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

Null dereference bug during deserialization #103

Open
WesToleman opened this issue Jul 16, 2020 · 0 comments
Open

Null dereference bug during deserialization #103

WesToleman opened this issue Jul 16, 2020 · 0 comments

Comments

@WesToleman
Copy link

I have found a scenario that causes a null dereference in GraphQLDeserilization.cs:L92.

This response causes the error when a GraphQLUnionOrInterface is involved yet works fine when there is no inheritance.

{
  "data": {
    "foo": null
  }
}

E.g. FooQuery works fine and Foo is null. FooBarQuery causes an issue in the deserializer.

public class FooQuery
{
    [GraphQLArguments("id", "String!", "fooId", isRequired: true, inlineArgument: false)]
    public Foo Foo { get; set; }
}

public class FooBarQuery
{
    [GraphQLArguments("id", "String!", "fooId", isRequired: true, inlineArgument: false)]
    [GraphQLUnionOrInterface("FooBar", typeof(FooBar))]
    public Foo Foo { get; set; }
}

public class Foo
{
    public string Id { get; set; }
}

public class FooBar : Foo
{
    public string Bar { get; set; }
}

Client fix

It's possible to fix the client by annotating the potentially null value with [JsonProperty(NullValueHandling = NullValueHandling.Ignore)].

public class FooBarQuery
{
    [JsonProperty(NullValueHandling = NullValueHandling.Ignore)]
    [GraphQLArguments("id", "String!", "fooId", isRequired: true, inlineArgument: false)]
    [GraphQLUnionOrInterface("FooBar", typeof(FooBar))]
    public Foo Foo { get; set; }
}

Possible library fix

It's possible to globally specify how nulls and missing values should be handled by the deserializer. I'm not sure of the full implications of this but I believe ignoring nulls may be safe considering default nullability of GraphQL types. Missing members may mask errors though.

--- GraphQLDeserilization.cs
+++ GraphQLDeserilization.cs
@@ -22,7 +22,11 @@
             var converters = GetFieldConverters(fields);

             // Get JsonSerilizerSettings
-            var settings = new JsonSerializerSettings();
+            var settings = new JsonSerializerSettings
+            {
+                NullValueHandling = NullValueHandling.Ignore,
+                MissingMemberHandling = MissingMemberHandling.Ignore,
+            };

             foreach (var converter in converters)
             {
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

No branches or pull requests

1 participant