Client-side Query Variable Verification

[nmck257]

(I spot-checked docs and other issues and couldn't find anything about this; apologies if I missed something)

Affects spring-graphql:1.4.1.

Consider a graphqlClient.documentName("...").variable(/*...*/).executeSync().toEntity(/*...*/) sequence. It's possible that the values supplied by variable calls will mismatch the set of variables defined for the query in the documentName, like providing a value of the wrong type (eg value null for a variable defined as non-null). Similarly, the query itself might be defined with an unused variable.

Those cases could be detected on the client-side, but, are currently sent over the network to the server anyway. This can yield exceptions like this:

org.springframework.graphql.client.FieldAccessException: Invalid field '', errors: [{message=Validation error (UnusedVariable) : Unused variable 'junk'}]
	at org.springframework.graphql.client.DefaultClientResponseField.toEntity(DefaultClientResponseField.java:106) ~[spring-graphql-1.4.1.jar:1.4.1]

It would be nice to have an option to run client-side variable validation before sending a query, to avoid consuming unnecessary network/server resources.

[bclozel]

Hey @nmck257 thanks for reaching out.

I'm wondering if this is a variant of #569 (comment)
We solved this on the tester side with a custom Jackson serializer to allow for present/not present/null values as variables.

I agree that right now both the client and tester do not check whether the GraphQL document being sent complies with the server schema. If we want to solve this we would need to at least parse the outgoing document (variables, fragments and more) but probably have the schema parsed in memory beforehand (as variables can be input types or enum values).

I think we're back to considering loading the schema when creating the GraphQL client. I think the client would lose its flexibility and the tradeoffs then might be in favor of a code generation solution like the DGS client.

What do you think?

[nmck257]

Thanks for the context @bclozel !

I think my main observation is that there's a class of query variable validations which shouldn't require the schema at all. Such as:

• Scalar variable values agreeing with scalar variable declarations ("don't pass an int to a bool")
• Absence/presence/nulls (across scalars, enums, and input types)
• Unused variable declarations (no references in the operation body)
• Unused variable values (supplied at runtime from the JVM code but not corresponding to anything in the query document)

You'd probably need a bit of cleverness in the (de)serialization to not stumble over the unknown types from the (absent) schema. But if that works, then I think we could get some healthy baseline validations without having to solve the tricky scenarios you've outlined with providing schemas vs DGS, etc.

[bclozel]

We discussed this issue as a team this morning and we decided to decline it.

This is indeed quite similar to #569 - to perform such checks we need to parse the actual request document. We could use the AST parser from graphql-java, but without the concrete schema we won't be able to act on it: input types, scalars, fragments). Even if we try and implement a "best effort" approach, this will be very brittle. This will come with a concrete performance penalty as well, for minimal ROI.

Taking a step back, we believe that the schema is very much a server concern and that there are two ways to go about this:

• testing the client
• generating types from the schema using the DGS client

Thanks for the enhancement idea, keep them coming!

[nmck257]

I think I follow the reasoning. Thanks for thinking this through!

Maybe this sort of verification functionality would make more sense in a (hypothetical) unit test construct, designed to assist with testing client-side GraphQl interactions.
In my apps, I typically use @RestClientTest for that -- but I've imagined that Spring GraphQl could provide some extensions for that pattern to optimize for GraphQl interactions. Not sure if that's been considered.