Add IApiResponseTypeMapper that allows configuring a ResponseType for values that also implement IConvertToActionResult

[pranavkm]

From https://github.com/aspnet/Mvc/issues/7036#issuecomment-371065915

@rynowak As far as I could see, while ActionResult<T> is nicely 'unwrapped' (i.e. Swagger picks T as the result model), inheriting from ActionResult<T> does not yield the same result. Consider a class that wraps a 'standardized' result object, used to shape the response of the API (Result<T>), and an ActionResult<T>-derived class used to simplify the generic definition (ApiResult<T>):

    public class Result<T>
    {
        public string Message { get; set; }
        public T Value { get; set; }
    }

   public class ApiResult<TValue> : ActionResult<Result<TValue>>
   {
        public static implicit operator ApiResult<TValue>(TValue value)
        {
            return new ApiResult<TValue>(value);
        }

        public static implicit operator ApiResult<TValue>(Result<TValue> value)
        {
            return new ApiResult<TValue>(value);
        }

        public static implicit operator ApiResult<TValue>(ActionResult result)
        {
            return new ApiResult<TValue>(result);
        }

        public ApiResult(TValue value)
                : base(new Result<TValue>
                       {
                               Value = value
                       })
        {
        }

        public ApiResult(Result<TValue> value)
                : base(value)
        {
        }

        public ApiResult(ActionResult result)
                : base(result)
        {
        }
    }

In such a case while an action defined as

        [HttpGet("Strings")]
        public async Task<ActionResult<Result<IEnumerable<string>>>> GetStringsAsync()
        {
            await Task.CompletedTask;
            return new Result<IEnumerable<string>>
                   {
                           Value = new[]
                                   {
                                           "a", "b"
                                   }
                   };
        }

yields the following swagger

an action defined using the ActionResult<T>-derived class does not unwrap (notice the result property and the actual Result<T> wrapped in a value property, both inherited from ActionResult<T>):

        [HttpGet("Strings2")]
        public async Task<ApiResult<IEnumerable<string>>> GetStrings2Async()
        {
            await Task.CompletedTask;
            return new Result<IEnumerable<string>>
                   {
                           Value = new[]
                                   {
                                           "a", "b"
                                   }
                   };
        }

Is this an unsupported scenario? Is it possible to create a custom class that acts like ActionResult<T> or extend the ActionResult<T> to other classes?

Edit: I just noticed that the result of the derived class is actually unwrapped, so it seems the issue is just with the way the action is interpreted by swagger, maybe...

[pranavkm]

Likely an issue wiith our unwrapping code here - https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs#L474. That said, deriving from ActionResult<> isn't a scenario that has much coverage or thought through, so we should make sure we understand any other fallout of supporting this behavior.

[rynowak]

We should seal ActionResult<T>

[BladeWise]

The only reason I was interested in deriving from ActionResult<T>, was to write code in a more convenient way, enforcing an 'envelope' for my API results, without having to write nested generic definitions, that tend to complicate readability. That said, I don't see the shortcomings in applying the unwrap policy to a little more generalized class of types. As I reported above, the functionality works as expected, it's just the metadata that are not coherent with the actual runtime behaviour.

[mkArtakMSFT]

@jbagga, can you please handle this? Thanks!

[rynowak]

@BladeWise - I haven't heard anything here that makes me want to reconsider sealing ActionResult<T>

Can you explain a little more about what you hope to achieve by writing your own envelope? How is it different from ActionResult<T>

[jbagga]

https://github.com/aspnet/Mvc/pull/7507 to seal ActionResultofT

[BladeWise]

@rynowak As far as I know, ActionResult<T> is a way to create strong-typed actions that can be interpreted by the Api explorer without further work from the developer (i.e. decorating every action with a proper ProducesResponseTypeAttribute). It does not add a new functionality (you could achieve the same with a bit more work), it is just convenience for the developer plus a nice compile-time checking that helps avoiding errors. If I am not wrong then, there is value (for a developer) in using a derived version of that class to avoid a verbose declaration like

public async Task<ActionResult<Result<IEnumerable<string>>>> GetStringsAsync()

in favor of something that better reflects the logic behind the API the developer is writing, like

public async Task<ApiResult<IEnumerable<string>>> GetStrings2Async()

To be honest, apart from this I cannot see any other critical scenario where inheriting is required.

That said, I consider a bug the fact that the action execution does not match the action description and while sealing the class would avoid the issue, I tend to think that the execution behavior is the correct one... what is the value of a sealed class in this case? And what is the purpose of the IConvertToActionResult interface if the actual test being performed is over ActionResult<T>?

[rynowak]

So, you're right ActionResult<T> gives you a particular runtime and API Explorer behavior. The runtime behavior is implemented by the IConvertToActionResult interface.

If we gave you an easy way to implement your desired API Explorer behavior for your custom result type, what would that look like? Can you make a suggestion?

[BladeWise]

I am honestly happy inheriting from a base class and ActionResult<T> fits perfectly; I don't think that there is the need for infrastructural changes or new code to handle this.

As @pranavkm pointed out, the issue lies in the implementation of ClosedGenericMatcher. That code tries to find the generic implementation of a class, but fails to find the relationship between a DerivedActionResult<T> and ActionResult<T>. I don't know if the purpose is finding the best matching generic implementation of a class, or finding the best generic implementation of an interface, but in the first case it does not work properly.

Check this method, the call to IsGenericInstantiation is called only on the initial tested type or the implemented interfaces (even implemented by its ancestors). After the first pass, only interfaces are unwrapped, while the base type is only tested directly. This means that the method fails to find types inheriting from a generic type definition.

A possible alternative would be a method like this

public static Type GetImplementationOfGenericOrDefault(Type type, Type genericTypeDefinition)
{
    if (!genericTypeDefinition.GetTypeInfo()
                                .IsGenericTypeDefinition)
        throw new ArgumentException("The type is not a generic type definition", nameof(genericTypeDefinition));

    var checkInterfaces = genericTypeDefinition.GetTypeInfo()
                                                .IsInterface;
    while (type != null && type != typeof(object))
    {
        var typeInfo = type.GetTypeInfo();
        var current = typeInfo.IsGenericType ? type.GetGenericTypeDefinition() : type;
        if (genericTypeDefinition == current)
            return type;

        if (checkInterfaces)
        {
            var genericInterfaceType = typeInfo.GetInterfaces()
                                                .FirstOrDefault(x =>
                                                {
                                                    var info = x.GetTypeInfo();
                                                    return info.IsGenericType && info.GetGenericTypeDefinition() == genericTypeDefinition;
                                                });
            if (genericInterfaceType != null)
                return genericInterfaceType;
        }
        type = typeInfo.BaseType;
    }
    return null;
}

which is missing the best match logic based on names, but could be added.

A (maybe) simpler alternative would be moving the call to IsGenericInstantiation at the beginning of the GetGenericInstantiation, so that every ancestor is checked just like the initial type.

I don't know if it is a bug or by design, yet it is counterintuitive that unwrapping a generic type definition fails for derived types.

[rynowak]

I am honestly happy inheriting from a base class and ActionResult fits perfectly; I don't think that there is the need for infrastructural changes or new code to handle this.

It's still our plan to make ActionResult<T> sealed. The main reason why we provided that type (as opposed to alternatives) is to support implicit conversions from ActionResult and T - which goes away if you inherit from it.

If you've defined your own type to act as an envelope that's fine, but we should look at really providing the support for that (including API Explorer).

If after all of this, you think you're "getting something out of inheriting ActionResult" that we can't provide in another way, I'd like to know what that is.

---

What if we provided something like:

public interface IApiResponseTypeMapper
{
    Type GetResponseDataType(Type returnType);
}

Where you're called during API Explorer execution to attempt to 'resolve' the declared return type of a method to a data type that you're like to treat it as? This would also allow us to remove some special cases from our code.

We would unwrap task-likes before calling this method.

Another alternative would be to provide something like IConvertToActionResult<T> where we do an inference based on T, but I don't like that as much because it seems less flexible. Providing a service where you can write code allows you to do a lot more.

---

ClosedGenericMatcher is designed to work on interfaces, and it's a little bit of a shame that it doesn't validate it's inputs so we would have caught this. I'm not really sure either if it's by design that this works or doesn't work, but that code wasn't designed to match classes - so it's a bug that we're using it this way.

[BladeWise]

I see your point and I agree that losing the implicit conversion inheriting from ActionResult<T> partially defeats the purpose of the class... even in my example I had to re-implement operators, so a bit more code implementing an interface would not pose a problem.

Regarding your proposals, I tend to think that having a single interface/service used to describe both the execution and API explorer behaviors makes sense, at least I cannot foresee a scenario where decoupling such functionalities can bring value.

If we consider IConvertToActionResult the proper way to define a type that wraps an IActionResult at execution time, it makes sense that the API explorer can infer from it the actual return type at design time, and isn't it possible only through a generic interface like IConvertToActionResult<T> where the type is determined at compile-time?

The same could be said for a service like IApiResponseTypeMapper, I am fine with implementing/extending a service to properly process my wrapped type, but find a little error prone the fact that unless I create the service and implement IConvertToActionResult in my wrapper class I have not a consistent behavior between execution and API explorer. Would it make sense, in your opinion, to use the same service to provide both behaviors, something like

public interface IApiResponseTypeMapper
{
    Type GetResponseDataType(Type returnType);
    object ConvertToResponseDataType(object returnObject);
}

In such a case the default implementation of the service could just target the sealed ActionResult<T>, IConvertToActionResult could be dropped and the logic moved to the implementation of the service itself...

[rynowak]

I think that sounds fine.

I want to keep IConvertToActionResult as well, but document that you should consider implementing IApiResponseTypeMapper if you implement it. The interface call for the ActionResult<T> case is much better than having to put a bunch of generic unwrapping code in this service that will run for every request.

[pranavkm]

Updated the title of the issue to reflect what we need to do here. ActionResult<T> was made sealed as part of https://github.com/aspnet/Mvc/commit/ed18a8f975e8f6df002994d2303fefb0bfc68b59

[rynowak]

c93c168