Zaid-Ajaj / Snowflaqe

A dotnet CLI to generate type-safe GraphQL clients for F# and Fable with automatic deserialization, static query verification and type checking
MIT License
154 stars 25 forks source link

Schema with unions throws NullReferenceException #70

Closed Yakimych closed 2 years ago

Yakimych commented 2 years ago

Hi! Looks like it is currently not possible to generate a client from a schema that contains Union types. In Snowflaqe we get a Object reference not set to an instance of an object, but we (@margaretkrutikova and @danielstarck) tracked it down to the following line in graphql-dotnet. The problem is that when called like this, both union.ResolveType and union.IsTypeOf are null.

Not sure if we should open an issue there, since it's not entirely clear to me whether the way it is called from Snowflaqe is the intended usage. The examples in graphql-dotnet docs provide the actual types when calling Schema.For, while Snowflaqe only provides the shcema string. Please advice.

I've opened a PR with a simple repro here: https://github.com/Zaid-Ajaj/Snowflaqe/pull/69

Zaid-Ajaj commented 2 years ago

Hi @Yakimych thanks for opening the issue! This is definitely a regression, I am pretty sure both unions and interfaces are supported in Snowflaqe but it could be that only the schema parser is broken in latest graphql-dotnet.

Any chance you could use a schema from a JSON file as a result of the introspection query rather using the SDL directly?

Yakimych commented 2 years ago

Hi @Zaid-Ajaj, and thanks for the quick reply! Unfortunately it might not be a regression (at least not in the latest version), as I've tried both 1.36.0 as well as 1.34.0, and both return the same error message.

I've tried running both versions against a JSON-schema as you suggested, and both work smoothly. Not sure if it helps though, since the operation that fails is the one that is trying to convert the SDL to JSON itself.

MargaretKrutikova commented 2 years ago

@Zaid-Ajaj unfortunately we need to use our SDL since we don't expose introspection query in our environment.

Me and @danielstarck have tested fromSchemaDefinition more now and it doesn't seem to work with either interfaces or unions. The reason behind this is that graphql-dotnet expects to see either ResolveType on the parent type (union or interface) or IsTypeOf on the union cases/interface implementations. And when doing GraphQL.Types.Schema.For(definition) both ResolveType and IsTypeOf end up being null which later makes the introspection query fail here:

graphqlServer.ExecuteAsync(GraphQLSerializer(), fun options -> options.Query <- IntrospectionQuery)

The validation rules in graphql-dotnet SchemaTypes fail with InvalidOperationException. Here is where it throws exception for union types and for interface types.

In a real world scenario when querying real data that includes unions or interfaces, you would need to know which union case or interface implementation to use when mapping data to your graph types, that's why you need to have either ResolveType or IsTypeOf. In the case with Snowflaqe however, we don't care (🤪) about the validation rules when running the introspection query since there is no mapping of data involved. We also figured that we can set a dummy ResolveType on the SchemaBuilder to make graphql-dotnet not complain about the validation. So this can be a fix:

let configure (schemaBuilder: SchemaBuilder) =
    schemaBuilder.Types.ForAll(fun typeConfig -> typeConfig.ResolveType <- fun _ -> null) |> ignore

let graphqlServer =
    GraphQL.Types.Schema.For(definition, configure)

let schemaJson = ...

All the tests will pass with this fix. What do you think about this, @Zaid-Ajaj ?

Zaid-Ajaj commented 2 years ago

@MargaretKrutikova thanks a lot for digging into this! It saves me a lot of time debugging 🙏 I understand the problem better now and the fix looks great to me, if you include it in the PR #69, I would be happy to merge and publish a new version of Snowflaqe

MargaretKrutikova commented 2 years ago

I created a different PR since I don't have access to that fork. Any feedback is greatly appreciated 🙂