Use MVC action results for formatting viewmodel response

[markphillips100]

@mauroservienti I've been experimenting with how I can return MVC action results from handlers. I'll give you some background first.

Many of my handlers utilize the mediator pattern with pipelines that can generate validation errors. Up until now I've been throwing a custom ValidationException with any validation errors that may occur. This exception then gets handled higher up in the request pipeline using custom middleware to map the exception to a standardized problem details response.

I'd like to avoid throwing exceptions for validation but to do so requires 2 things:

1. an api in ServiceComposer to allow me to supply the required ActionResult (a BadRequestObjectResult that wraps a ValidationProblemDetails in this case)
2. a change in how the response is created in CompositionEndpointBuilder in this code branch

Basically I imagine a handler being able to tell SC that it would like a specific IActionResult to be used for the response rather than the viewmodel.

As for the response formatting a lot of code can be replaced (I think) by utilizing MVC result execution. The following extensions would support this.

    public static class HttpContextActionResultExtensions
    {
        private static readonly RouteData EmptyRouteData = new RouteData();

        private static readonly ActionDescriptor EmptyActionDescriptor = new ActionDescriptor();

        /// <summary>
        /// Write a model to the response.
        /// </summary>
        public static Task WriteModelAsync<TModel>(this HttpContext context, TModel model)
        {
            var result = new ObjectResult(model)
            {
                DeclaredType = typeof(TModel)
            };

            return context.ExecuteResultAsync(result);
        }

        /// <summary>
        /// Write any action result to the response.
        /// </summary>
        public static Task ExecuteResultAsync(this HttpContext context, IActionResult result)
        {
            if (context == null) throw new ArgumentNullException(nameof(context));
            if (result == null) throw new ArgumentNullException(nameof(result));

            var routeData = context.GetRouteData() ?? EmptyRouteData;
            var actionContext = new ActionContext(context, routeData, EmptyActionDescriptor);

            return result.ExecuteResultAsync(actionContext);
        }
    }

Then, the if (useOutputFormatters) { code block becomes:

  if (context.Items.ContainsKey(HttpRequestExtensions.ComposedActionResultKey))
  {
      await context.ExecuteResultAsync(context.Items[HttpRequestExtensions.ComposedActionResultKey] as IActionResult);
  }
  else
  {
      await context.WriteModelAsync(viewModel);
  }

I provided a SetActionResult extension to help supply the required action result to SC but not certain this is the best way.

        public static void SetActionResult(this HttpRequest request, ActionResult actionResult)
        {
            if (!request.HttpContext.Items.ContainsKey(HttpRequestExtensions.ComposedActionResultKey))
            {
                request.HttpContext.Items.Add(HttpRequestExtensions.ComposedActionResultKey, actionResult);
            }
        }

What do you think?

[mauroservienti]

@markphillips100 thanks for raising this issue. From what I have understood you'd expect to be able to do something like:

public class MyHandler : ICompositionRequestsHandler
{
    [HttpGet("/foo/bar")]
    public Task Handle(HttpRequest request)
    {
        request.HttpContext.BadRequest( new { /* bad request details model */ } );
        return Task.CompletedTask;
    }
}

The BadRequest extension is similar to the SetActionResult one. What happens if more than one handler sets the action result?

[markphillips100]

I'm assuming there's possibly other result types that would need to be catered for, not just bad request. E.g. NotFound, Conflict, Forbid, etc, hence why I only employed a generic interface to write the response.

That aside, yes indeed, what should happen if more than one handler for the same route sets an action result. I'm not sure to be honest unless only using subscribers - in that situation I'd say the single composition handler owns the results.

When there are multiple composition handlers for the same route I guess there would need to be some convention that all parties agree on. It's necessary to agree on the route anyway.

Would just allowing only one to be registered and drop any subsequent results perhaps work? In other words, allow any handler to register a result but ignore them if one has already been registered. This is kinda what I do with the extension now.

[mauroservienti]

Let's see a use case. We could have something like:

public static void BadRequest(this HttpRequest request, object model)
{
   //something here to store the action result for later usage
}

public static void NotFound(this HttpRequest request)
{
   //something here to store the action result for later usage
}

And then three handlers for the same route:

public class AHandler : ICompositionRequestsHandler
{
    [HttpGet("/foo/bar")]
    public Task Handle(HttpRequest request)
    {
        request.BadRequest( new { /* bad request details model */ } );
        return Task.CompletedTask;
    }
}

public class AnotherHandler : ICompositionRequestsHandler
{
    [HttpGet("/foo/bar")]
    public Task Handle(HttpRequest request)
    {
        request.NotFound();
        return Task.CompletedTask;
    }
}

public class AThirdHandler : ICompositionRequestsHandler
{
    [HttpGet("/foo/bar")]
    public Task Handle(HttpRequest request)
    {
        var vm = request.GetComposedResponseModel();
        vm.SomeData = "some data";

        return Task.CompletedTask;
    }
}

The third one is interesting because it would be very similar to the following:

public class AThirdHandler : ICompositionRequestsHandler
{
    [HttpGet("/foo/bar")]
    public Task Handle(HttpRequest request)
    {
        var vm = request.GetComposedResponseModel();
        vm.SomeData = "some data";

        request.ContentResult(vm); //assuming a default HTTP 200
        //or request.Ok(vm);

        return Task.CompletedTask;
    }
}

how does the resulting HTTP response look like? Is there any way we could express in a single response the "messages" all handlers are trying to convey?

[markphillips100]

Not really as the intent of individual responses are tightly coupled to the http status code. A not found is a 404, a content 200, bad request 400, etc.

This is I guess why I only imagined supporting any 4xx results but only 4xx types. 500 is an exception and 200 are resolved using normal objectresult semantics in the SC code using the composed VM dynamic. The first 4xx result wins. From the client perspective it doesn't matter which type of 4xx, they just know that something needs resolving their side

[markphillips100]

I should clarify. Of course the client cares about which 4xx they get, just not whether a notfound beat a bad request to the wire

[mauroservienti]

Everything makes sense, and honestly, I don't think there is any real issue with multiple handlers trying to set multiple action results. There is already a concurrency "issue" in setting the response code and documentation describes that. We can have a similar note on the documentation page for this feature.

One last question, hopefully 😄: The first 4xx result wins or the last?

The last would be way simpler to implement, the first requires some guards to be put in place to prevent the context.Items to be accessed multiple times for the same key, which is not really problematic, just wondering.

[markphillips100]

Last or first it doesn't matter I think. The only real requirement is if there is one it is returned instead of the VM.

Thanks for the discussion @mauroservienti

[mauroservienti]

@markphillips100 I dropped the ball on this, sorry about that. I should be able to raise a PR soon.

[mauroservienti]

@markphillips100 I raised #322. I'm not sure I understand how to change the CompositionEndpointBuilder to use the supplied action result. Your snippet is:

  if (context.Items.ContainsKey(HttpRequestExtensions.ComposedActionResultKey))
  {
      await context.ExecuteResultAsync(context.Items[HttpRequestExtensions.ComposedActionResultKey] as IActionResult);
  }
  else
  {
      await context.WriteModelAsync(viewModel);
  }

Can you please suggest how to use it?

[markphillips100]

Don't worry about "dropping the ball". We all lead busy lives at times hey.

I've been used an internal customized-for-my-needs version in the meantime. My CompositionEndpointBuilder is as follows but also note I understand it's up for discussion :-)

        public CompositionEndpointBuilder(RoutePattern routePattern, Type[] componentsTypes, int order, ResponseCasing defaultResponseCasing, bool useOutputFormatters)
        {
            this.useOutputFormatters = useOutputFormatters;
            Validate(routePattern, componentsTypes);

            casingToSettingsMappings.Add(ResponseCasing.PascalCase, new JsonSerializerSettings());
            casingToSettingsMappings.Add(ResponseCasing.CamelCase, new JsonSerializerSettings() {ContractResolver = new CamelCasePropertyNamesContractResolver()});

            RoutePattern = routePattern;
            Order = order;
            DefaultResponseCasing = defaultResponseCasing;
            RequestDelegate = async context =>
            {
                var viewModel = await CompositionHandler.HandleComposableRequest(context, componentsTypes);
                if (viewModel != null)
                {
                    if (useOutputFormatters)
                    {
                        if (context.Items.ContainsKey(HttpRequestExtensions.ComposedActionResultKey))
                        {
                            await context.ExecuteResultAsync(context.Items[HttpRequestExtensions.ComposedActionResultKey] as IActionResult);
                        }
                        else
                        {
                            await context.WriteModelAsync(viewModel);
                        }
                    }
                    else
                    {
                        var json = (string) JsonConvert.SerializeObject(viewModel, GetSettings(context));
                        context.Response.ContentType = "application/json; charset=utf-8";
                        await context.Response.WriteAsync(json);
                    }
                }
                else
                {
                    await context.Response.WriteAsync(string.Empty);
                }
            };
        }

This code is dependent on a new addition to your HttoRequestExtensions to add a SetActionResult extension to http request's httpcontext:

        public static void SetActionResult(this HttpRequest request, ActionResult actionResult)
        {
            if (!request.HttpContext.Items.ContainsKey(HttpRequestExtensions.ComposedActionResultKey))
            {
                request.HttpContext.Items.Add(HttpRequestExtensions.ComposedActionResultKey, actionResult);
            }
        }

Note, the ComposedActionResultKey is just another static string:

        internal static readonly string ComposedActionResultKey = "composed-action-result";

Not really relevant except to just show how I use the extension but I'll explain my use case. My handlers/subscribers use mediator queries returning "result" types. These types are not mvc results, i.e. the command query layer isn't coupled to mvc. Obviously then I need to map them to relevant action results and I do this via a pattern match extension:

        public static OneOf<ActionResult, TValue> HandleResultForController<TValue>(
            this FluentResults.Result<TValue> result,
            ModelStateDictionary modelState,
            string propertyPrefix = null,
            bool usePropertyNames = false)
        {
            modelState.Update(result, propertyPrefix, usePropertyNames);

            return result switch
            {
                { } res when res.HasError<NotFoundError>() => new NotFoundResult(),
                { } res when res.HasError<ConflictError>() => new ConflictResult(),
                { } res when res.HasError<NotAuthorizedError>() => new ForbidResult(),
                { } res when res.HasError<RequestValidationError>() => new BadRequestObjectResult(new ValidationProblemDetails(modelState) { Type = ProblemDetailsType }),
                { } res when res.IsFailed => new BadRequestResult(),
                _ => result.Value
            };
        }

Notice the ValidationProblemDetails there surfaced as a 400 BadRequest. This supports the ideal of not throwing an exception for 400 errors.

My OneOf ActionResult match handlers then get processed by calling the new SetActionResult extension.

        public static async Task<OneOf<ActionResult, None>> HandleResultForCompositionRequestsAsync(
            this Result result,
            HttpContext httpContext)
        {
            var modelState = new ModelStateDictionary();

            return await result
                .HandleResultForController(modelState)
                .Match<Task<OneOf<ActionResult, None>>>(
                    async actionResult => await httpContext.ProcessActionResultAsync(actionResult),
                    async _ => await Task.FromResult(new None())
                );
        }

        public static Task<ActionResult> ProcessActionResultAsync(this HttpContext httpContext, ActionResult actionResult)
        {
            httpContext.Request.SetActionResult(actionResult);
            return Task.FromResult(actionResult);
        }

And from a handler this looks like this, where the only real logic is what to do with "ok" data, e.g. add it to viewmodel, raise composition event, etc:

        [HttpGet("carts/{orderId:guid}")]
        public async Task Handle(HttpRequest request)
        {
            var requestModel = await request.Bind<Cart.Query>();
            var result = await _mediator.Send(requestModel);

            var resultHandler = await result.HandleResultForCompositionRequestsAsync(request.HttpContext);

            await resultHandler
                .Match(
                    async _ => await Task.CompletedTask,
                    async value => await ProcessValueAsync(request, requestModel, value)
                );
        }

        private static async Task ProcessValueAsync(HttpRequest request, Cart.Query requestModel, CartViewModel value)
        {
            request.GetComposedResponseModel().sales = value;

            var orderItemIds = value.OrderItemDetails
                .SelectMany(orderItemDetail => orderItemDetail.OrderItem.GetAllOrderItems())
                .Select(orderItem => orderItem.Id)
                .Distinct()
                .ToArray();

            await request
                .GetCompositionContext()
                .RaiseEvent(new CartGetRequested
                {
                    OrderId = requestModel.OrderId,
                    OrderItemIds = orderItemIds
                });
        }

[mauroservienti]

Thanks @markphillips100. I just realized that when going through the context all the formatters stuff is handled automatically.

Do you think it's worth raising an exception if someone is trying to use this new features and output formatters are not enabled? (They are disabled by default)

Second question, do you see any issue with enabling formatters by default in v2, and allowing disabling them merely for backward compatibility?

[markphillips100]

Raising an exception would be useful for sure, and I was going to suggest making output formatters the default too :-)

As an aside, is there any follow up on System.Text.Json support for dynamic (expando) serialization? Would be nice to have the option of which to use from the MVC perspective, although I typically am using Json.Net anyway due to NServiceBus in process so no big problem having both serializers.

[mauroservienti]

Raising an exception would be useful for sure, and I was going to suggest making output formatters the default too :-)

Raised #323 to track this. I'll add the exception to the PR.

As an aside, is there any follow up on System.Text.Json support for dynamic (expando) serialization? Would be nice to have the option of which to use from the MVC perspective, although I typically am using Json.Net anyway due to NServiceBus in process so no big problem having both serializer.

I'm having a hard time keeping track of what they are doing. Based on this comment dotnet/runtime#31175 (comment) it seems that .NET 6 comes with support for JsonNode but they won't support DynamicOjbect out of the box by default, but at least we could implement support for it. Also some more info in dotnet/runtime#29690 and dotnet/runtime#53195.

[markphillips100]

I guess I understand them not wanting to add a dependency from the tree shaking perspective. And perhaps creating our own might be the way. A task for another time.

[mauroservienti]

@markphillips100, it's available on Nuget. Many thanks for your patience.

[markphillips100]

Wonderful @mauroservienti . I shall update asap. Many thanks again.