Skip to content

OkObjectResult should be changed to support better type safety #32080

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
AgentEnder opened this issue Apr 22, 2021 · 1 comment
Closed

OkObjectResult should be changed to support better type safety #32080

AgentEnder opened this issue Apr 22, 2021 · 1 comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates design-proposal This issue represents a design proposal for a different issue, linked in the description ✔️ Resolution: Duplicate Resolved as a duplicate of another issue Status: Resolved

Comments

@AgentEnder
Copy link

Summary

OkObjectResult has an inner type of object?. This proposal is to make it generic, allowing better type safety when used in conjunction with ActionResult

Motivation and goals

Currently it is possible to break type safety in a method that returns an ActionResult. See the code sample below, found in a controller.

[HttpGet()]
public ActionResult<T1> MyControllerMethod {
    T2 myResult = repository.get();
    return Ok(myResult);
}

This code not only compiles, but creates API endpoints whose swagger documentation represents T1, when the returned type is actually T2. This is clearly unintended.

The goal here is to not change the signature of the helper functions like Ok(), but to use generics and type inference to ensure that types flow through the system properly.

Risks / unknowns

Some API's could theoretically build only because of this issue. While this would not be a hard thing for developers to fix, this would be a breaking change due to some projects not building after the types are locked down.

@AgentEnder AgentEnder added the design-proposal This issue represents a design proposal for a different issue, linked in the description label Apr 22, 2021
@pranavkm
Copy link
Contributor

Thanks @AgentEnder for your issue. We're considering tackling this issue as part of #8535 for 6.0. Feel free to upvote the other issue so we can prioritize it correctly.

@pranavkm pranavkm added the ✔️ Resolution: Duplicate Resolved as a duplicate of another issue label Apr 23, 2021
@ghost ghost added the Status: Resolved label Apr 23, 2021
@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 23, 2021
@javiercn javiercn closed this as completed May 7, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates design-proposal This issue represents a design proposal for a different issue, linked in the description ✔️ Resolution: Duplicate Resolved as a duplicate of another issue Status: Resolved
Projects
None yet
Development

No branches or pull requests

4 participants