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

Add PolymorphicTypenameConverter class to handle __typename deserialization #343

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lanwin
Copy link

@lanwin lanwin commented Apr 26, 2021

Attached a helper class I found very useful to handle fragments and __typename deserialization:

Usage:

            [JsonConverter(typeof(DiscriminateEvent))]
            public Event Event { get; set; }

            class DiscriminateEvent : PolymorphicTypenameConverter<Event>
            {
                protected override Type Descriminator(string typename) =>
                    typename switch
                    {
                        "EventA" => typeof(EventA),
                        "EventB" => typeof(EventB),
                        _ => typeof(Event)
                    };
            }

I think there should be also a example within the readme cause its not obvious how to use it.

But for now I want to know what you think first and if its a nice addition.


public override T Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
var clone = reader; // cause its a struct
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not obvious for me too. But the Utf8JsonReader is a struct and many contains pointers to data to read. So to get the __typename we need to read ahead the data and then move back and deserialize the object. But the reader can only read forward. So we make use of the struct and clone it by assign it to another variable. So both clone and reader and read to differenc positions from each others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Better to put a comment into sources.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By itself, this class without descendants carries little benefit. You can make at least one descendant, which discovers all the classes in the declaring assembly.

Comment on lines 18 to 20
if(clone.TokenType == JsonTokenType.StartObject)
clone.Read();
if(clone.TokenType != JsonTokenType.PropertyName || clone.GetString() != "__typename")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it works only if I query __typename meta-field.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. its designed to help with using Graphql Fragments. No other purpose.

…meConverter.cs

Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
@lanwin lanwin marked this pull request as ready for review April 27, 2021 12:43
@lanwin lanwin marked this pull request as draft April 27, 2021 12:45
Copy link
Collaborator

@rose-a rose-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lanwin, thanks for your contribution!

I think this can be very handy if you need to work with GraphQL interfaces and union types.

We'd need a similar converter for Newtonsoft.Json, too.


namespace GraphQL.Client.Serializer.SystemTextJson
{
public abstract class PolymorphicTypenameConverter<T> : JsonConverter<T>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can find a class name which makes the use case for this converter a little bit more obvious (in that it's tied to the builtin GraphQL __typename field).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FromTypenameConverter ?

Copy link
Collaborator

@rose-a rose-a Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GraphQLTypenameConverter? 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is already inside GraphQL.* namespace.

@sungam3r
Copy link
Member

@lanwin Would you like to continue working on it?

…meConverter.cs

Co-authored-by: Alexander Rose <alex@rose-a.de>
@lanwin
Copy link
Author

lanwin commented Aug 29, 2022

@lanwin Would you like to continue working on it?

Yes but I am not sure yet whats expected from here.

@sungam3r
Copy link
Member

@rose-a ?

@StevenDewey
Copy link

It would be great to see this or something like this supported! It is a great idea, though it looks like it has lost some momentum over the past year...

@Keyslam
Copy link

Keyslam commented Jan 15, 2024

Works like a charm when using unions for error handling. Would like to see it merged into the library. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants