Modify JsonSerializationOptions instead of creating a new instance #2651
Conversation
mmsmits
changed the title
mmsmits
changed the title
There was a problem hiding this comment.
There is a ForFhir<F> just below that has the same problem.
More in general, we create & return new options, which does not work well with the ASP.NET service configuration pattern, e.g. see here: https://www.meziantou.net/configuring-json-options-in-asp-net-core.htm
Pretty() and Compact() will have the same problems. Thinking about this a bit more, changing this could be considered a breaking change, since we're suddenly changing the input parameter, which is probably unexpected.
What to do about that?
…instead-of-creating-new' into bugfix/2583-modify-json-options-instead-of-creating-new
…n or deserialization has occurred.
| @@ -41,7 +41,8 @@ public void RoundtripEdgeCases() | |||
| { | |||
| var (poco, expected) = getEdgecases(); | |||
|
|
|||
| var options = BaseOptions.Pretty(); | |||
| var options = new JsonSerializerOptions().ForFhir(typeof(TestPatient).Assembly).Pretty(); | |||
There was a problem hiding this comment.
This is exactly why this could be considered a breaking change, as we discussed on our call. Still, the benefits for the use in service initialization outweigh the risk I think. And I guess many people even assume we return the same instance (but then modified). But we cannot be sure.
There was a problem hiding this comment.
I'll add something to the release notes
ewoutkramer
deleted the
bugfix/2583-modify-json-options-instead-of-creating-new
branch
fixes: #2583