graphql-dotnet / graphql-client

A GraphQL Client for .NET Standard
MIT License
623 stars 134 forks source link

Proper enum deserialization #129

Closed felpel closed 4 years ago

felpel commented 5 years ago

Hi,

I'm not sure if I'm missing something with my GraphQL client configuration, given I use the version 2.0.0-alpha.3 of this package.

I have a C# enum on one of our servers that is returned as a property from the GraphQL endpoint.

Given our conventions, a value following the Pascal case notation such as EnumValue is serialized as ENUM_VALUE by graphql-dotnet/graphql-dotnet.

It seems that this is the expected behavior, documented in GraphQL's specs.

However, if I'm trying to use the same enum on the client, I'm unable to deserialize the data from GraphQLResponse.GetDataFieldAs<T> method.

I can reproduce the behavior with Digitransit's GraphQL public endpoint with the following code run with LINQPad:

async Task Main()
{
    var request = new GraphQLRequest
    {
        Query = @"query {
            alerts {
                alertEffect
            }
        }"
    };

    var httpClient = new HttpClient();

    var options = new GraphQLHttpClientOptions {
        EndPoint = new Uri("https://dev-api.digitransit.fi/routing/v1/routers/finland/index/graphql")
    };

    GraphQLResponse response;
    using (var client = httpClient.AsGraphQLClient(options)) {
        response = await client.SendQueryAsync(request);
    }

    if (response.Data != null)
    {
        var @event = response.GetDataFieldAs<List<Alert>>("alerts");

        @event.Dump();
    }
}

class Alert { 
    public AlertEffect AlertEffect {get;set;}
}

enum AlertEffect {
    NoService,
    ReducedService,
    SignificantDelays,
    Detour,
    AdditionalService,
    ModifiedService,
    OtherEffect,
    UnknownEffect,
    StopMoved, 
    NoEffect
}

StringEnumConverter doesn't seem to help either if I add it to GraphQLHttpClientOptions serializer settings' converters.

felpel commented 5 years ago

I think we might have found a workaround, but I'm not sure if it would be the proper solution. Given we had the EnumMemberAttribute on top of individual enum values like this:

enum AlertEffect 
{
    [EnumMember(Value = "NO_SERVICE")]
    NoService,

    // Other possible values
}

The enum can be properly deserialized, but if the enum would be shared inside the codebase, it would also be serialized as "NO_SERVICE" for other contexts and that's a no-go.

sungam3r commented 5 years ago

@felpel This error actually occurs due to the fact that by default GraphQL server implementation returns enumerations in a slightly different notation - CONSTANT_CASE. On our projects we widely use GraphQL servers and .NET generated clients. Although these are our own generated clients, they are based on this package, so I have spent some time to investigate posted issue. Indeed, now GetDataFieldAs does not support serializer settings (I don’t use GetDataFieldAs method at all).

You can use following workaround with custom json converter:

client.Options.JsonSerializerSettings.Converters.Add(new GraphQLEnumConverter());
 public sealed class GraphQLEnumConverter : StringEnumConverter
    {
        public GraphQLEnumConverter()
            : base(new ConstantCaseNamingStrategy())
        {
        }
    }
 internal sealed class ConstantCaseNamingStrategy : NamingStrategy
    {
        protected override string ResolvePropertyName(string name) => StringUtils.ToConstantCase(name);
    }
 /// <summary> Copied from https://github.com/graphql-dotnet/graphql-dotnet/blob/master/src/GraphQL/Utilities/StringUtils.cs </summary>
    internal static class StringUtils
    {
        private static readonly Regex ReWords = new Regex(@"[A-Z\xc0-\xd6\xd8-\xde]?[a-z\xdf-\xf6\xf8-\xff]+(?:['’](?:d|ll|m|re|s|t|ve))?(?=[\xac\xb1\xd7\xf7\x00-\x2f\x3a-\x40\x5b-\x60\x7b-\xbf\u2000-\u206f \t\x0b\f\xa0\ufeff\n\r\u2028\u2029\u1680\u180e\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u202f\u205f\u3000]|[A-Z\xc0-\xd6\xd8-\xde]|$)|(?:[A-Z\xc0-\xd6\xd8-\xde]|[^\ud800-\udfff\xac\xb1\xd7\xf7\x00-\x2f\x3a-\x40\x5b-\x60\x7b-\xbf\u2000-\u206f \t\x0b\f\xa0\ufeff\n\r\u2028\u2029\u1680\u180e\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u202f\u205f\u3000\d+\u2700-\u27bfa-z\xdf-\xf6\xf8-\xffA-Z\xc0-\xd6\xd8-\xde])+(?:['’](?:D|LL|M|RE|S|T|VE))?(?=[\xac\xb1\xd7\xf7\x00-\x2f\x3a-\x40\x5b-\x60\x7b-\xbf\u2000-\u206f \t\x0b\f\xa0\ufeff\n\r\u2028\u2029\u1680\u180e\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u202f\u205f\u3000]|[A-Z\xc0-\xd6\xd8-\xde](?:[a-z\xdf-\xf6\xf8-\xff]|[^\ud800-\udfff\xac\xb1\xd7\xf7\x00-\x2f\x3a-\x40\x5b-\x60\x7b-\xbf\u2000-\u206f \t\x0b\f\xa0\ufeff\n\r\u2028\u2029\u1680\u180e\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u202f\u205f\u3000\d+\u2700-\u27bfa-z\xdf-\xf6\xf8-\xffA-Z\xc0-\xd6\xd8-\xde])|$)|[A-Z\xc0-\xd6\xd8-\xde]?(?:[a-z\xdf-\xf6\xf8-\xff]|[^\ud800-\udfff\xac\xb1\xd7\xf7\x00-\x2f\x3a-\x40\x5b-\x60\x7b-\xbf\u2000-\u206f \t\x0b\f\xa0\ufeff\n\r\u2028\u2029\u1680\u180e\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u202f\u205f\u3000\d+\u2700-\u27bfa-z\xdf-\xf6\xf8-\xffA-Z\xc0-\xd6\xd8-\xde])+(?:['’](?:d|ll|m|re|s|t|ve))?|[A-Z\xc0-\xd6\xd8-\xde]+(?:['’](?:D|LL|M|RE|S|T|VE))?|\d+|(?:[\u2700-\u27bf]|(?:\ud83c[\udde6-\uddff]){2}|[\ud800-\udbff][\udc00-\udfff])[\ufe0e\ufe0f]?(?:[\u0300-\u036f\ufe20-\ufe23\u20d0-\u20f0]|\ud83c[\udffb-\udfff])?(?:\u200d(?:[^\ud800-\udfff]|(?:\ud83c[\udde6-\uddff]){2}|[\ud800-\udbff][\udc00-\udfff])[\ufe0e\ufe0f]?(?:[\u0300-\u036f\ufe20-\ufe23\u20d0-\u20f0]|\ud83c[\udffb-\udfff])?)*");

        /// <summary>
        /// Split a cased string into a series of "words" excluding the separator if applicable.
        /// </summary>
        public static IEnumerable<string> ToWords(string str)
        {
            foreach (Match match in ReWords.Matches(str))
            {
                yield return match.Value;
            }
        }

        public static string Capitalize(string str)
        {
            switch (str.Length)
            {
                case 0: return str;
                case 1: return str.ToUpperInvariant();
                default:
                    str = str.ToLowerInvariant();
                    return char.ToUpperInvariant(str[0]) + str.Substring(1);
            }
        }

        public static string ToCamelCase(string str)
        {
            return ChangeCase(str, "", (word, index) => index == 0 ? word.ToLowerInvariant() : Capitalize(word));
        }

        public static string ToConstantCase(string str)
        {
            return ChangeCase(str, "_", w => w.ToUpperInvariant());
        }

        public static string ToPascalCase(string str) => ChangeCase(str, "", Capitalize);

        public static string ChangeCase(string str, string sep, Func<string, string> composer)
        {
            return ChangeCase(str, sep, (w, i) => composer(w));
        }

        public static string ChangeCase(string str, string sep, Func<string, int, string> composer)
        {
            string result = "";
            int index = 0;

            foreach (string word in ToWords(str))
            {
                result += (index == 0 ? "" : sep) + composer(word, index++);
            }

            return result;
        }
    }

Also note that you should use version of Newtonsoft.Json which supports NamingStrategy (it seems this is version 12, not 11 from GraphQL client dependencies). 2.0.0-alpha.3 was published 9 months ago and since then the master branch has gone forward and already has latest Newtonsoft dependency.

Second option (if you own the server) - you can change server side logic to return NormalCase instead of CONSTANT_CASE - just override this method to return original string value.

sungam3r commented 5 years ago

I understood a simple thing - you use wrong enum. Where did you get it? This enum should be provided by the server through the schema. All the above tricks are completely unnecessary. There are the correct names from https://api.digitransit.fi/graphiql/hsl image

felpel commented 5 years ago

@sungam3r is the converter (i.e. GraphQLEnumConverter) available publicly in one of graphql-dotnet's NuGet packages? I think it would be very useful to make it available, given it would resolve this issue very easily and could be verified by the rest of the community if it documented properly.

Regarding your latest comment, it was just an example to illustrate an example involving an enum with multiple words broken down by underscores!

We're using GraphQL in server-to-server communication and I think I'd rather stick to what is defined in GraphQL's specifications for the naming of our enumerations.

sungam3r commented 5 years ago

No, Graphqlenumconverter was written manually to test this issue. Understand that you should use only what the service schema provides, and not some secret internal knowledge about what it use undercover. There should not be any shared server-to-server enums for you.

But you can make it work by simply stopping returning enum values from the server in CONSTANT_CASE notation by overriding the method in EnumerationGraphType and returning enum values as is. Then you can use shared enums.

maximebousquet commented 5 years ago

Hey @sungam3r!

I've gotten the same kind of problem recently. I understand that there is an easy workaround to serialize an ENUM in a different way. However, what I don't understand, is why the default behaviors of the graphql-client and the graphql-dotnet (server) do not work out-of-the-box together (i.e. the server serializes the ENUM as CONSTANT_CASE but the GetDataFieldAs does not work with this behavior).

You mentioned that you never use GetDataFieldAs, which is causing the problem of deserializing the ENUM. What do you use instead? Do you use custom graphql clients?

Thanks!

sungam3r commented 5 years ago

 why the default behaviors ... do not work out-of-the-box together 

Because .NET world != GraphQL world. Where did you get your enums? Understand that you should use only what the service schema provides.

Do you use custom graphql clients?

Exactly. I use self written generator which build full-featured type safe .net client (netstandard2.0) from graphql url or sdl file. At work we extensively use it for dozens of our backend services. Out CI has triggers for build new package for graphql .net client every time appropriate service schema changes.

deinok commented 5 years ago

Basicly this issue comes from the fact on how the Enums are converted in C# that don't match GraphQL standards.

As @sungam3r has pointed the best way to solve this is have some kind of autogenerated Schema / client. (something like OpenAPI / Swagger / Blueprint)

deinok commented 5 years ago

@maximebousquet @felpel At the moment, I try to follow the GraphQL RFC and this library will always try to follow the RFC, priorizing this over the .NET GraphQL Server.

So, for now I will let this open while I find a way to make interoperability easier with some common GraphQL servers

sungam3r commented 4 years ago

@rose-a It seems to me that this issue can be closed since the answer is given.

adamjones1 commented 3 years ago

Because .NET world != GraphQL world. Where did you get your enums? Understand that you should use only what the service schema provides.

But isn't entire point of a GraphQL client library to serve as a bridge between those two worlds? I'm not sure I understand the logic in this thread (from either direction) - yes absolutely the client should be unaware of how a server is implemented and only concern itself with the schema in its published spec, but if it's a given that the convention in GraphQL specs is to case things one way and in .Net another way, surely the .Net client should provide some (optional) lubricant for mapping between the two? Otherwise details of GraphQL leak into the consuming client's domain model. (And the client has to disable pesky compiler warnings about the naming convention breach.)

sungam3r commented 3 years ago

But isn't entire point of a GraphQL client library to serve as a bridge between those two worlds?

No. .NET client can work with any server written in .NET/Java/Go/Haskell/etc.

surely the .Net client should provide some (optional) lubricant for mapping between the two?

I'm sure the answer is no. Just think carefully about the source of the problem again. It is that you are trying to apply knowledge about the internals of the server to the client using some server-side code. The API hides implementation details and, in this case, modifies the (internal) enumeration names. If you so want to use rigid conventions, then just don't rename the enumeration values and return them as they are i.e. MyEnumValue and not MY_ENUM_VALUE, then they will deserialize normally.

adamjones1 commented 3 years ago

I think I didn't articulate myself clearly. Agreed, if enumeration names in a GraphQL spec so happen to come from a .Net application which so happens to produce them from C# names on an enum type, this is irrelevant to a client. The "bridging two worlds" I'm talking about here isn't bidirectional to/from .Net, ie. ".Net server -> GraphQL -> .Net client" - it is only "GraphQL -> .Net client".

As a .Net client I have no interest in whatever the names for enums might have been on the server-side - I just know that in my app, I want to write them in Pascal case because that is the native convention in my client app's environment (whether or not they were originally Pascal case on the server), and that the GraphQL spec writes them in upper case, because that is the native convention in GraphQL. This is true whether or not I actually have ownership of the server, so breaking GraphQL conventions on the server side isn't a fix.

sungam3r commented 3 years ago

OK. So I suggest you to post a new issue about custom deserialization setting for enum. But it seems to me that even this is not required and you can already do all the necessary settings. See https://github.com/graphql-dotnet/graphql-client/blob/master/src/GraphQL.Client.Serializer.Newtonsoft/NewtonsoftJsonSerializer.cs and https://github.com/graphql-dotnet/graphql-client/blob/master/src/GraphQL.Client.Serializer.SystemTextJson/SystemTextJsonSerializer.cs

adamjones1 commented 3 years ago

Hi - apologies for the radio silence here. I think there's no issue in the GraphQL.Net client per se after all - I'm using the System.Text.Json transport and the JsonStringEnumConverter you use in that linked file is all I'm after really, so that should solve my problem. I think the reason it's apparently not solving my problem, having just run into this problem testing something unrelated, is thanks to this nugget of fun: https://github.com/dotnet/runtime/issues/31619. So it looks like the code doesn't do what it intends after all due to a bug (or very misleading API at best) in the BCL.

sungam3r commented 3 years ago

What if use two converters - one for reading and another for writing?

adamjones1 commented 3 years ago

I think that would work but the read converter would have to be manually implemented rather than using JsonStringEnumConverter...

sungam3r commented 3 years ago

Of course.

ViRuSTriNiTy commented 2 years ago

In case of the Newtonsoft part, I think the underlying issue here is that ConstantCaseEnumConverter does not override ReadJson which by default uses code that is not compatible with CONSTANT_CASE values. I don't know why this is the case but we can remove and add a custom converter like mentioned here but with the SnakeCaseNamingStrategy provided by Newtonsoft:

var enumNamingStrategy = new SnakeCaseNamingStrategy();
var enumConverter = new StringEnumConverter(enumNamingStrategy);

var constantCaseEnumConverters = settings.Converters.OfType<ConstantCaseEnumConverter>().ToList();
foreach (var constantCaseEnumConverter in constantCaseEnumConverters)
{
    settings.Converters.Remove(constantCaseEnumConverter);
}

settings.Converters.Add(enumConverter);
EvrenJG commented 3 months ago

There's a simpler way, you can inherit ConstantCaseEnumConverter and override ReadJson yourself.

I've got this working by creating this class:

private class GraphQLEnumConverter : ConstantCaseEnumConverter
{
    public override object? ReadJson(JsonReader reader, Type objectType, object? existingValue, JsonSerializer serializer)
    {
        return
            reader.Value is string value
            && Enum.TryParse(objectType, value.ToPascalCase(), out var enumValue)
                ? enumValue
                : base.ReadJson(reader, objectType, existingValue, serializer);
    }
}

and using it like this:

var serializer = new NewtonsoftJsonSerializer(
    new JsonSerializerSettings
    {
        Converters = [new GraphQLEnumConverter()]
    });
var client = new GraphQLHttpClient(options, serializer, httpClient);
winnative commented 1 month ago

You see this problem because GraphQL returns the name of enum members as result while json needs the values of them to deserialize.

You can use Resolve method in your GraphQL Server-Side and return the value of each enum members like int or byte.