octokit / octokit.graphql.net

A GitHub GraphQL client library for .NET
MIT License
143 stars 50 forks source link

Selecting a null GraphQL element throws an unfriendly exception #97

Open jcansdale opened 6 years ago

jcansdale commented 6 years ago

For example, the following:

var query =
    from x in new Query().Repository(owner: "grokys", name: "PullRequestSandbox")
    select new { x.Name, x.Description, ForkedFrom = x.Parent.NameWithOwner };
var result = await Connection.Run(query);

will throw:

System.InvalidOperationException: Cannot access child value on Newtonsoft.Json.Linq.JValue.
    at Newtonsoft.Json.Linq.JToken.get_Item(Object key)
    at x => new <>f__AnonymousType3`3(Name = x.Name, Description = x.Description, ForkedFrom = x.Parent.NameWithOwner)(Closure , JToken )
    at Octokit.GraphQL.Core.Builders.Rewritten.Value.Select[TResult](JToken source, Func`2 selector)
    at lambda_method(Closure , JObject )
    at Octokit.GraphQL.Core.Deserializers.ResponseDeserializer.Deserialize[TResult](Func`2 deserialize, JObject data)
    at Octokit.GraphQL.Core.Deserializers.ResponseDeserializer.Deserialize[TResult](Func`2 deserialize, String data)
    at Octokit.GraphQL.Core.SimpleQuery`1.Runner.<RunPage>d__10.MoveNext()

Unfortunately ?. operators aren't currently supported in expressions trees:

image

So the user would need to do the following to get the expected result:

var query =
    from x in new Query().Repository(owner: "grokys", name: "PullRequestSandbox")
    select new { x.Name, x.Description, ForkedFrom = x.Parent != null ? x.Parent.NameWithOwner : null };
var result = await Connection.Run(query);
Console.WriteLine(result);
{ Name = PullRequestSandbox, Description = , ForkedFrom =  }

Problems with current situation

  1. It isn't obvious from the exception that the issue is with x.Parent.NameWithOwner
  2. The obvious x.Parent?.NameWithOwner fix isn't supported
  3. After seeing the compile error, it wasn't obvious that x.Parent != null ? ... would be supported
  4. I didn't see the exception immediately because by first queries didn't return any null elements

Possible solutions

Might it be possible to expose nullable elements as a custom nullable type (similar to nullable value types)? This could support .Value and .GetValueOrDefault(...) like the existing Nullable type.

This nullable type could also support a .SelectOrDefault(x => x....) method, for when a user wants to continue theSelect` operation.

It would then be possible to do this:

var query =
    from x in new Query().Repository(owner: "grokys", name: "PullRequestSandbox")
    select new { x.Name, x.Description, ForkedFrom = x.Parent.SelectOrDefault(y => y.NameWithOwner) };

This would make it easy to discover and handle nullable elements and avoid surprises when an element comes back null.

StanleyGoldman commented 6 years ago

I've now encountered this error myself.

StanleyGoldman commented 6 years ago

Here is a ResponseDeserializerTest in #122 that demonstrates @jcansdale's initial issue

https://github.com/octokit/octokit.graphql.net/blob/987969483687240bf24b6901459e7031ad12ecbb/Octokit.GraphQL.Core.UnitTests/ResponseDeserializerTests.cs#L421-L437