Skip to content
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

Microsoft.AspNetCore.Mvc.JsonOptions uses default instead of OC JOptions settings #16831

Closed
wAsnk opened this issue Oct 4, 2024 · 8 comments · Fixed by #16837
Closed

Microsoft.AspNetCore.Mvc.JsonOptions uses default instead of OC JOptions settings #16831

wAsnk opened this issue Oct 4, 2024 · 8 comments · Fixed by #16837
Assignees
Labels

Comments

@wAsnk
Copy link
Contributor

wAsnk commented Oct 4, 2024

Describe the bug

Only Microsoft.AspNetCore.Http.Json.JsonOptions is configured here with the OC Json settings, while Microsoft.AspNetCore.Mvc.JsonOptions is kept as default, causing not minimal Apis using the default Microsoft.AspNetCore.Mvc.JsonOptions during model binding deserialization.

Expected behavior

Both JsonOptions should use the same settings, a similar configuration merge should be done for Microsoft.AspNetCore.Mvc.JsonOptions to ensure same behavior.

@wAsnk wAsnk added the bug 🐛 label Oct 4, 2024
@hishamco
Copy link
Member

hishamco commented Oct 4, 2024

@wAsnk feel free to submit a PR

@wAsnk
Copy link
Contributor Author

wAsnk commented Oct 4, 2024

@wAsnk feel free to submit a PR

If I'll have spare time I'll do it and if someone else won't do it in the meantime.

@hishamco hishamco self-assigned this Oct 4, 2024
@hishamco
Copy link
Member

hishamco commented Oct 4, 2024

I will submit a PR tonight

@MikeAlhayek
Copy link
Member

MikeAlhayek commented Oct 4, 2024

I am not sure it is the best idea to change the global JsonOptions for OC project as it could impact users negatively.

In the minimal API where you are returning content item, I would just return the result differently.

For example, https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Modules/OrchardCore.Contents/Endpoints/Api/GetEndpoint.cs#L45

instead of returning return TypedResults.Ok(contentItem);, we can return Results.Json(contentItem, options.Value.SerializerOptions); while IOptions<DocumentJsonSerializerOptions> options would be injected into the HandleAsync method.

Same goes to here: https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Modules/OrchardCore.Contents/Endpoints/Api/DeleteEndpoint.cs#L47

and

https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Modules/OrchardCore.Contents/Endpoints/Api/CreateEndpoint.cs#L126

Also, the JsonOptionsConfigurations should be removed so we don't expose global app settings that would override .NET core framework by default. @sebastienros thoughts?

@MikeAlhayek
Copy link
Member

@wAsnk can you please share with us a content item that caused an exception in the minimalApi?

It would be great if you have a repo to ensure it is properly fixed.

@wAsnk
Copy link
Contributor Author

wAsnk commented Oct 8, 2024

@MikeAlhayek
Sorry it might've been misunderstandable. I was experiencing this with NOT minimal APIs, so with ApiControllers. In ApiControllers, SystemTextJsonInputFormatter and SystemTextJsonOutputFormatter is used during model binding, and the serializer there uses settings from Microsoft.AspNetCore.Mvc.JsonOptions.

I think setting globally the JsonOptions should be either for both or neither. But neither looks like the better solution.

Btw this wasn't my root problem, it was just a sidequest for me apparently.... My original problem was that I had a Collection with a private setter in a generated class, so while the OC defined settings where the JsonObjectCreationHandling was set to Populate the collection was populated. But with the default settings of JsonOptions the JsonObjectCreationHandling is set to Replace, thus a private setter won't allow the collection to be set. So during deserialization I was missing data.
While I didn't know the exact problem I just copied the Http.JsonOptions to the Mvc.JsonOptions and used that everywhere.
My final solution now is that I removed the Http.JsonOptions like you did in your PR. And also now I generate the class with a public setter for the collection. Now I can serialize-deserialize back and forth with the same result, unlike before.

@MikeAlhayek
Copy link
Member

Thanks @wAsnk I also added a default output formatter to provide a way to deserialize Document and Entity by default.

@Piedone
Copy link
Member

Piedone commented Oct 9, 2024

Fixed by #16837.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment