Json Serialiser doesn't handle structs well and ignores type converters about graphql-client HOT 11 OPEN

Even for the records, this doesn't work:

public readonly partial record struct ProductId
{
    public System.Guid Value { get; }

    public ProductId(System.Guid value)
    {
        Value = value;
    }
}

But this does work:

public readonly partial record struct ProductId(Guid Value) {}

from graphql-client.

Interestingly, it works if I use Newtonsoft.Json but not System.Text.Json

from graphql-client.

Hi, sorry for the late reply...

I do not quite understand why ImmutableConverter was chosen and not the specific one that I registered...

You could try to insert your custom converter at the beginning of the converters list (like here), this had to be done for the ErrorPathConverter and MapConverter, too.

from graphql-client.

@rose-a Thanks for the suggestion!
Indeed, it works with the insert method!

But I still think that it is a bug because annotating the type with

[System.Text.Json.Serialization.JsonConverter(typeof(ProductIdSystemTextJsonConverter))]

doesn't work.

But it does work for the Newtonsoft serialiser. So, something is still odd here.

from graphql-client.

Yeah, I'd expect that to work, too.

But I don't know how to fine-tune the System.Text.Json serializer for that... PRs welcome 😉

from graphql-client.

@rose-a I may try to find some time to look into it...
As a super quick and easy fix, would you accept this logic?

If the type is marked with JsonConverterAttribute (there is a converter for this type), ImmutableConverter should return false from CanConvert so that the registered converter can have its turn.

Also, could you please elaborate on the ImmutableConverter, why is it needed?

from graphql-client.

Also, could you please elaborate on the ImmutableConverter, why is it needed?

If I remember correctly it is needed for deserialization of anonymous types... If you're playing around, you could check what happens if you remove it (some tests should fail)

If the type is marked with JsonConverterAttribute (there is a converter for this type), ImmutableConverter should return false from CanConvert so that the registered converter can have its turn.

Sounds reasonable, although I have no idea on how this will impact performance... @sungam3r @Shane32 do you have an opinion on this?

from graphql-client.

If you're playing around, you could check what happens if you remove it (some tests should fail)

Yeah, that's the thing.
I already checked it out, all the tests still pass if we remove ImmutableConverter.

I tried commenting out this line

https://github.com/graphql-dotnet/graphql-client/blob/master/src/GraphQL.Client.Serializer.SystemTextJson/JsonSerializerOptionsExtensions.cs#L9

and even removing the whole ImmutableConverter.cs file, all the tests are happy.

Hence the question: why do we need it?

from graphql-client.

Hm... trying to read up on this, it seems like this might have become obsolete wit .NET 5.0...

from graphql-client.

If so, then it may fix the issue in a nicest way: fixing bugs by removing code :)

from graphql-client.