Skip to content

OkObjectResult with generic type parameter #48324

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

Open
1 task done
lus opened this issue May 19, 2023 · 5 comments
Open
1 task done

OkObjectResult with generic type parameter #48324

lus opened this issue May 19, 2023 · 5 comments
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@lus
Copy link

lus commented May 19, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

I am a bit perplexed that issue #31396 was closed with the simple response "there is nothing we can do about it" as it seems like it is an issue that is very messy to work around. Please correct me if I am missing something.

The root of the problem described there is that ASP.NET uses JsonSerializer.Serialize<object> for serializing OkObjectResults.
EF Core returns proxies of some types with some additional properties that are unwanted in the resulting JSON.
Because Serialize<object> is used instead of Serialize<wanted_type>, these properties are not ignored as they lack the [JsonIgnore] attribute (which seemingly cannot be added either as an EF Core maintainer acknowledged this issue without bringing this up as a possible solution).

Describe the solution you'd like

As long as I am not missing something: would it technically be possible to add an additional OkObjectResult<T> class that uses T rather than object (and subsequently JsonSerializer.Serialize<T> rather than JsonSerializer.Serialize<object>)?

We could then use

MyType proxiedValue = dbSet.Find(...);

return Ok<MyType>(proxiedValue);

in the controller.

Additional context

I know that there were other issues suggesting this, but they were closed because multiple people think #8535 would be a better solution.
As I am not sure about whether this would solve this issue as well, I am going to bring this up again.

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 19, 2023
@javiercn
Copy link
Member

@lus thanks for contacting us.

I think what @mitchdenny mentions is right. The only thing I can suggest is authoring an https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.metadata.ijsontypeinforesolver?view=net-7.0 to take over the proxy serialization yourself.

But this is a case where ASP.NET does not know about EF and EF does not know about ASP.NET, and is not a boundary we want to break, so there is some additional integration work.

MVC does a lot more internally than just calling Json.Serialize, so I don't think the solution you are proposing is trivial to implement, and potentially not even possible.

If I had to solve this, I would try the type info resolver to try and detect the proxy types and return the contract for the base instance instead. As for how to do that exactly, I will defer to the docs on config for system.text.json and mvc.

@halter73
Copy link
Member

halter73 commented May 22, 2023

Another option would be to write your own generic IResult or ActionResult that calls JsonSerializer.SerializeAsync<T>(HttpContext.Response.Body, proxiedValue) yourself.

We could consider new API that does essentially this, but we'd have to think about consistency with TypedResults.Ok<T>() and TypedResults.Json<T>() which also always serialize as object regardless of the T, so always serializing as object is something we do pretty consistently in ASP.NET Core. Changing this for a new set of Results could make things more confusing even if it's consistent with the lower level JsonSerializer API it's calling.

@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label May 23, 2023
@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone May 23, 2023
@ghost
Copy link

ghost commented May 23, 2023

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.

@lus
Copy link
Author

lus commented May 24, 2023

@halter73 thank you for your detailed reply. I do understand now that making this change to the API is not that straightforward as I may have thought, so thanks for clearing this up.
I think I will try the workaround you mentioned then, that seems pretty reasonable.

@randyshoopman
Copy link

randyshoopman commented Jan 22, 2024

In case anyone else runs into this here's a JsonConverter that helps solve it:

/// <summary>
/// Used to serialize EF Proxy objects using the non proxy class.  This solves a number of issues
/// including not serializing added proxy properties and respecting Json Attributes on the non
/// proxy class (like JsonIgnore or JsonPropertyName)
/// </summary>
public class UnwrapEFProxyJsonConverter : JsonConverter<object>
{
    public override bool CanConvert(Type typeToConvert)
    {
        var entityTypeFromProxy = ObjectContext.GetObjectType(typeToConvert);
        // types won't match if typeToConvert is an EF proxy
        return entityTypeFromProxy != typeToConvert;
    }
    public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        throw new NotImplementedException("UnwrapEFProxyJsonConverter converter is not for deserialization");
    }

    public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options)
    {
        var entityTypeFromProxy = ObjectContext.GetObjectType(value.GetType());
        JsonSerializer.Serialize(writer, value, entityTypeFromProxy, options);
    }
}

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

7 participants
@halter73 @javiercn @randyshoopman @wtgodbe @mkArtakMSFT @lus and others