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
Fix double variable coercion in QueryTraverser #2836
Fix double variable coercion in QueryTraverser #2836
Conversation
@@ -147,7 +147,7 @@ QueryTraverser newQueryTraverser(ExecutionContext executionContext) { | |||
.schema(executionContext.getGraphQLSchema()) | |||
.document(executionContext.getDocument()) | |||
.operationName(executionContext.getExecutionInput().getOperationName()) | |||
.variables(executionContext.getVariables()) | |||
.coercedVariables(executionContext.getCoercedVariables()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing that you have multiple entry points into the constructor and hence the asserts of nbull ness got moved up into the builder
@@ -84,7 +84,7 @@ QueryTraverser newQueryTraverser(ExecutionContext executionContext) { | |||
.schema(executionContext.getGraphQLSchema()) | |||
.document(executionContext.getDocument()) | |||
.operationName(executionContext.getExecutionInput().getOperationName()) | |||
.variables(executionContext.getVariables()) | |||
.coercedVariables(executionContext.getCoercedVariables()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
Map<String, Object> variables) { | ||
assertNotNull(document, () -> "document can't be null"); | ||
CoercedVariables coercedVariables) { | ||
this.schema = schema; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you get rid of assertNotNull(document, () -> "document can't be null");
accidentilly here??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably saw it later - the null assert is inside the builder now
this.fragmentsByName = getOperationResult.fragmentsByName; | ||
this.roots = singletonList(getOperationResult.operationDefinition); | ||
this.rootParentType = getRootTypeFromOperation(getOperationResult.operationDefinition); | ||
this.coercedVariables = new ValuesResolver().coerceVariableValues(schema, variableDefinitions, rawVariables); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one for Andi omre yhan you - but since ValuesResolver
is a stateless class - we should move to static entry points. Object allocation is cheap - but statics are even cheaper AND it says its a singleton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, doesn't make sense to have an instance here.
I'll have a chat to Andi and make this change in a separate PR
this.roots = Collections.singleton(root); | ||
this.rootParentType = assertNotNull(rootParentType, () -> "rootParentType can't be null"); | ||
this.fragmentsByName = assertNotNull(fragmentsByName, () -> "fragmentsByName can't be null"); | ||
this.coercedVariables = new CoercedVariables(assertNotNull(variables, () -> "variables can't be null")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the asserts removed because these invariants are no longer true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding 2 new constructors so to keep the code tidier I thought I would move all the null checks into the builder, rather than having to remember 4 lots of asserts
return input | ||
} | ||
}) | ||
.build() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one - great repo!
This PR fixes double variable coercion in
QueryTraverser
and classes using traversal such asMaxQueryComplexityInstrumentation
,MaxQueryDepthInstrumentation
, andFieldValidationSupport
. Fixes #2819.The double coercion problem
Previously, raw and coerced variables were represented as plain maps
Map<String, Object>
. Using the same type made it hard to distinguish raw from coerced variables and in some places, such asMaxQueryDepthInstrumentation
, we unintentionally coerced variables twice. The double coercion was there for some time (e.g. #2458), but became a problem in v18.1 (see #2819)The
ZonedDateTime
custom scalar parser in the #2819 example acceptsString
andStringValue
, but not itself, causing aIllegalArgumentException
during the second coercion.The fix
This PR adds a
QueryTraverser
that handles already coerced variables, thus avoiding the problematic second coercion.In most cases, when a new
QueryTraverser
is instantiated, the variables have already been coerced. Following #2773, traversal will usually take place after theExecutionContext
is instantiated.