SystemTextJsonSerializer causes unexpected behavior on serialization

[fabio-s-franco]

When initializing SystemTextJsonSerializer with a custom JsonSerializerOptions, it messes the options in ways that are very difficult to predict and very difficult to debug.

I am using .Net 5

I believe that SystemTextJsonSerializer should take the JsonSerializerOptions as is and not try to modify it. For example, passing the options below cause some very weird results:

JsonSerializerOptions options = new JsonSerializerOptions(JsonSerializerDefaults.Web);
options.Converters.Add(new JsonStringEnumConverter()); //This causes wrong text conversion
options.Converters.Add(new DateTimeConverter()); //This is a custom converter I have for ISO Dates, haven't tested this one
options.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull; //This is ignored

Example of serialization:

public enum Salutation {
      mr = 1, ms = 2
}

public class Sample {
      public Salutation salutation { get; set; }
      public string? optionalField { get; set; }
}

Results

With System.Text.JsonSerializer directly:

{
    "salutation": "mr"
}

With SystemTextJsonSerializer from GraphQLClient:

{
    "salutation": "MR",
    "optionalField": null
}

At the moment I have some ugly workarounds. But that's far from ideal and error prone. Is there any way to simply take the options I provide without doing anything to it?

[rose-a]

How are you initializing SystemTextJsonSerializer? If you directly pass a JsonSerializerOptions object via its constructor it invokes this constructor, which does not override your options class (but adds 2 converters which are vital for it to deserialize the received payloads properly).

By default the converter capitalizes enum names (because that's defined in the GraphQL spec) and uses a camel case property naming policy.

[fabio-s-franco]

_gqlClient = new(graphQlEndpoint, new SystemTextJsonSerializer(jsonOtions));

This is how I initialize it.

Now that I use these options elsewhere, I started getting

An exception of type 'System.InvalidOperationException' occurred in System.Text.Json.dll but was not handled in user code: 'Serializer options cannot be changed once serialization or deserialization has occurred.'

I only get this if another serialization happens before I initialize the GraphQLHttpClient with the options. So it seems adding these converters is considered something that changes these options.

And although I understand that the spec may enforce this practice, it doesn't mean that 3rd party providers follow that. At this moment workarounds are necessary because of this enforced serialization options through the converters. I cannot control how spec abiding providers are and I believe I should not be blocked due to spec violations I cannot control.

I believe this could be somewhat optional as a parameter (like, enforeceGraphQlSpecs) as passing on custom options is already a beg to do something differently than what the specs or defaults should be.

Right now I have no way to control this behavior, which I think partially defeats the point of having these options available to the consumer of the library. I strongly believe whatever is passed into the serializer, should not be altered in any way. It's making some assumptions that are difficult to predict and makes some trivial issues difficult to solve.

[rose-a]

Then I'd recommend to not reuse the same jsonOptions instance in multiple places but create a new instance for this.

This library simply won't work as people expect without these converters (they're necessary for deserialization of the errors and extensions fields of a GraphQL response).

[fabio-s-franco]

Then I'd recommend to not reuse the same jsonOptions instance in multiple places but create a new instance for this.

That was just another side effect, even if I use a new instance, the behavior that I need on serialization was still being modified. As a matter of fact, this was the hint that the source of the problem was not the serializer and I was looking at the wrong place for a long time.

This library simply won't work as people expect without these converters

I understand, and I agree, that by default, that is how the serializer should be initialized. However, when we are injecting a behavior (by passing in the json options), we are applying the IoC principle. And when there is a modification of the flow that should be in control of the caller, we are violating this principle. And we get exactly the same thing: That something is not working as expected, with an added twist: Instead of "the library", it becomes "something" as the source of the problem becomes misleading. As it indicates the serializer maybe it. Debugging the problem gets even more difficult, because the source of the problem is hidden in the library's source code.

I had many problems with serializers before and the first place I started to look was on the serializer, whilst the problem was actually on the library.

So in my case, instead of not working as expected, it wouldn't work at all. The only way I could make it work was by forking the repo and publishing as my own package. And it works quite well without the converters for my scenarios.

I'd say it would be a more elegant solution to enforce it on the model itself. If The library can define an abstract model or interface with the proper definitions, then no surprises there, it becomes evident to the consumer, what the expected json format is. The library could even allow as a separate overload or method call, allow the consumer to pass in any model as he wants (on the "at your own risk" style).

The library could also make use of its own model internally and use the passed in models as compositions instead of inheritance. I'd actually prefer this way. Could even make it with 3 (optionally) distinct models: <TData, TError, TExtension>

and the class that represent this composition is particular to each serializer, to ensure they are compatible. When you define this model for the SystemTextJson serializer you could:

class GqlModel<TData, TError, TExtension>
{
    [JsonPropertyName("data")]
    public TData data { get; set; }
}

Which already overrides any naming policy that may be passed in by the serializer options. And since that portion of the model is not defined by the consumer, any problem becomes much more evident.