Create and include an analyzer that validates action method return types match the declared ActionResult<T>

[DamianEdwards]

Add an analyzer that validates the return types from an action method returning ActionResult<T>, e.g. don't let it return ObjectResult with a different T value, e.g. the following is invalid:

[HttpGet]
public ActionResult<IEnumerable<string>> Get()
{
    return new ObjectResult(new { Not = "Valid" });
}

[benmccallum]

I'm currently facing a similar dilemma. This seems like a nice place to chat about it without creating a new issue.

Essentially the existing codebase I'm working in has a BaseResponse that looks like so:

public abstract class BaseResponse
{
    public virtual int HttpStatusCode { get; set; } = 200;
}

The nice thing about this is that we can keep our controllers super light, route the message off with MediatR to a handler and all our handlers return an implementation of BaseResponse and can set a HttpStatusCode if they need to based on their own business logic. None of that has to happen in the Action method.

The existing codebase currently has an extension method on ControllerBase that does controller.StatusCode(baseResponse.HttpStatusCode, baseResponse); returning IActionResult and then the Action methods also return IActionResult.

I didn't really like that much, as I'd like my Action methods to have definite return types, so I started refactoring by creating an ApiControllerBase like so and moving that extension method in there for consistency with base Mvc:

[ApiController, Produces("application/json")]
public abstract class ApiControllerBase : ControllerBase 
{
    protected ObjectResult StatusCode(BaseResponse response) 
        => StatusCode(response.HttpStatusCode, response);
}

but then when I started specifying all the Action methods return types explicitly, I realised that returning StatusCode(xBaseResponse) that xBaseResponse could be any T of BaseResponse and not specifically the T that the action method should return. This is the same issue you've found and raised this ticket for, albeit in your case you instantiate ObjectResult but in my case I do it indirectly by calling StatusCode; as soon as any T is wrapped in ObjectResult type checking goes out the window.

Trying to think of other options, I've considered:

1. Can I extend ActionResult<T> with an implict cast operator for my BaseResponse? But obviously that's not possible on a type I don't own.
2. Can I inherit ActionResult<T> so I can have my own type with my own implicit case operator? Nope, it's sealed as the framework probably needs some certainty of the type here.
3. Can I write an ActionFilter that on the way out inspects the result, determines if BaseResponse and sets the status code off that? I think this is a yes, but the real downside I see is that the Api Analyzer now won't give me the useful hints about any missing ProducesResponseType attributes for my Action.
4. Can I do anything with the IConvertToActionResult interface? I haven't explored this, but could I implement this with my BaseResponse and it just work?
5. What if my BaseResponse had an implicit case operator to an ActionResult<T>? Would it be used over the operator on the other side?

Problem with the last two solutions is that this requires me to add a dependency to the Mvc package but in our solution that's not going to work as we have "Client" projects (essentially a client SDK for our service) with the Response models and then "Service" projects which ref the Client for the models and are our MVC projects. I wouldn't want our Client projects to have to reference all of Mvc, and indeed it'd actually be impossible as some of our consumers are net47 projects. So for those last two options to be viable I'd need to have IConvertToActionResult and ActionResult<T> in an abstracted package outside of Mvc which I'm guessing isn't something that'd be do-able your end?

All in all, I guess my best options right now are:

• Wait for this analyzer, stick with that StatusCode overload on my BaseApiController. Drawback of this over the filter though is you always need to do that wrapping call rather than just return TBaseResponse and be done with it.
• Option 3 above. Perhaps there's an interface that be shoved somewhere that my BaseResponse could inherit from and then the Api Analyzer could understand that it also needs to look in there for status codes and missing ProducesResponseType attrs?

// Example of such an action filter (not tested yet)
public class SetResponseStatusCodeFromBaseResponseActionFilter : ResultFilterAttribute
{
    public override void OnResultExecuted(ResultExecutedContext context)
    {
        if (context.Result is ObjectResult objectResult &&
            objectResult.Value is BaseResponse baseResponse)
        {
            objectResult.StatusCode = baseResponse.HttpStatusCode;
        }

        base.OnResultExecuted(context);
    }
}

Would love to hear any other suggestions!

[benmccallum]

I guess the other approach might be to have my MediatR handlers all return ActionResult<T>, kind of couples them with Mvc a bit...

Would still have the same problem you've highlight @DamianEdwards that there's desperate need for Analyzer validation there, but that'll be solved eventually.

[benmccallum]

I think this is something we all struggle with, "how can we have a business layer that can return a response that easily adjusts the status code?".

I think a BaseResponse approach works well, as long as we can solve this final hand-off to the ActionResult in a way that:

1. Has type checking
2. Has ProducesResponseType analyzer support

I also just discovered that the ProducesResponseType analyzer can only handle const status codes, so neither of my 2 best options look like they won't benefit much from this unless it can be updated to dig deeper into the user's code. As such, I'll go for the ActionFilter solution atm as it reduces the boilerplate and provides type safety.

TODO: Investigate an ActionFilter that in development mode could throw or log errors if response type != attributed response types.

[syndicatedshannon]

Shouldn't it be feasible to return implicitly return ExceptionalResult for ObjectResult<T>, while still disallowing unintentional return of ObjectResult<T2>, and simultaneously allow implicit conversion from T to ObjectResult<T>; all through standard compiler mechanisms, and thus avoiding analyzers and the associated baggage of development, maintenance, tool dependency, and derivation limitations?

I agree with @benmccallum that ultimately my chief issue is returning T from my Web API, while still occasionally handling a one-off ExceptionalResult outside of a filter. My preference would really be a robust and performant pattern to return raw DTO T; especially one that simplifies tests of both the expected and exceptional result. Currently our options are:

• Build a filter for a narrow use, and inspect for filter-specialized information during test
• Re-implement StatusCode results and return null or Default<T> on response
• Implement what I've described above, also discussed in ActionResult<T> class shouldn't be sealed #6729 (comment) , and then unwrap the result in tests.

I apologize for second-guessing someone's work I'm on the outside of, looking in to; I'm sure it looks easier from here than it is.

[syndicatedshannon]

Or perhaps I misunderstood @pranavkm 's comment and the analyzer is not a substitute for addressing this pattern issue long-term, but rather in addition to it, to help developers find problems in older code.

[ghost]

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.