ActionResult<T> class shouldn't be sealed

[scott-wojan]

Describe the bug

I was going to make an inherited class ActionListResult : ActionResult<List> to help avoid repeating List everywhere in controllers but I can't do this because this class is marked as sealed. Why mark this as sealed?

Expected behavior

Be able to use inheritance.

[blowdart]

Without the class name we can't really talk about why it's sealed. Could you please supply it?

[gfoidl]

The issue got edited (see there) -> ActionResult<List>

[syndicatedshannon]

I, too, had to discard the ActionResult<T> implementation because it was sealed.
I ended up re-implementing in AspNetCore the same solution I'd previously used in AspNet.
I didn't duplicate all functionality of ActionResult<T>, as I didn't implement IConvertible<T> or IConvertToActionResults.
However, I recall understanding why it had been sealed when I investigated. I feel like perhaps it was just the issue of IConvertible symmetry?
It wasn't a big deal to re-implement, but it concerns me that other bits of AspNetCore leverage ActionResult<T>, and we can't derive. Sorry, the only bit I currently remember is something about Swagger.

[syndicatedshannon]

I should probably also mention why I found ActionResult<T> insufficient.
One issue I had is that any ActionResult<T> can be implicitly converted to ActionResult and any ActionResult can be used to construct any ActionResult<T>. That leaves a lot of room to defeat useful compiler checks.
Additionally, other useful IActionResult implementations can return a specific type, such as CreatedAtRoute (I need to extend that as well, so additional sealed types won't help here).
Lastly (that I can remember) various useful Controller methods expect types that won't conform. I don't remember what I found when I tried to drive ActionResult<T> through an Ok<T>() overload, but something was unpleasant.

[syndicatedshannon]

My personal thinking at the time was that ActionResult<T> would probably get a pretty significant redesign in v3 due to the above, and I would just not worry about it until then.

[davidfowl]

That leaves a lot of room to defeat useful compiler checks.

Can you elaborate?

[syndicatedshannon]

Hey David, sure. A crude example to start. If we are generating proxies or documentation from our return types, of course we'd like the compiler to ensure our proxy/client doesn't throw RT (de)serialization errors. Otherwise attribute-based documentation performs the same function.

However, the following compiles:

public ActionResult<int> GetId() {
    return Ok(Guid.Create());
}

Of course, in most cases, we don't call Guid.Create() in our controller, we call a DL or BL. Now spot the defect:

public ActionResult<int> GetId() {
    return Ok(Data.GetId());
}

[scott-wojan]

In my case, I just don't want to write this over and over in conrollers:
public async Task<ActionResult<List<InventoryBookingsGetModel>>> GetAvailabilityAsync(DateTime date)

Task<ActionResult<List<InventoryBookingsGetModel>>> that is a ton of noise that I was hoping to simplify as :
Task<ActionListResult<InventoryBookingsGetModel>>
or even
ActionListResultTask<InventoryBookingsGetModel>

[syndicatedshannon]

just don't want to write this over and over

I hear you and appreciate your intuitive desire to reduce repetition. Of course reducing code shouldn't be the only goal. Readability is important too, and not always correlated with fewer characters. e.g. if we just want to reduce code we could simply state the return type as IActionResult in all cases.

By merging the content type with the platform type, we make it harder for consumers (both humans and tooling) to understand the data type that will need to be (de)serialized. We can't apply a simple logical rule and say "our web methods return whatever is inside the ActionResult angle braces".

[syndicatedshannon]

I don't know how ActionListResultTask would affect Swagger and client proxy generation tools, but I wouldn't be surprised if it broke them.

[pranavkm]

That leaves a lot of room to defeat useful compiler checks.

We're tracking addressing this using an analyzer in #8535.

Is there a reason you weren't able to use IConvertToActionResult? It's the contract the rest of MVC uses to produce an instance of IActionResult from ActionResult<T>.

[syndicatedshannon]

Is there a reason you weren't able to use IConvertToActionResult?

@pranavkm I'm not sure. As I noted above, I reviewed it as an option at the time I selected an approach, and chose against it. Looking at what I did, I probably chose to derive from OkObjectResult for the following reasons:

• to avoid re-implementing StatusCode and researching associated domain knowledge (e.g interactions with serializers, filters, other Response properties, short-circuits, etc), for which ASP.NET Core has pre-existing expertise
• provide a migration path, as my API surfaces have grown large; C# interfaces cannot supply implicit conversions
• I would have naturally preferred deriving a well-supported existing type over implementing conversion, out of concerns for quality gaps due to my lack of familiarity with ASP.NET internals and common issues with conversion
• less code; see below:

	public class WebOkResult<T> : OkObjectResult, IWebResult<T> {
		public WebOkResult(T content) : base(content) { }
	}

We're tracking addressing this using an analyzer in #8535.

@pranavkm Thanks for the heads-up. I wish it weren't necessary. See my comment in 8535

Personally, for most of the work I do in the middle tier, what I really desire are raw DTO return values. I could ignore all of this business if there were a better way to return T1 and T2 directly, while still allowing a high-performance exceptional response branch.

[ghost]

Thank you for contacting us. Due to no activity on this issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.