Fix double variable coercion in QueryTraverser #2836
Conversation
dondonz
changed the title
| .document(executionContext.getDocument()) | ||
| .operationName(executionContext.getExecutionInput().getOperationName()) | ||
| .variables(executionContext.getVariables()) | ||
| .coercedVariables(executionContext.getCoercedVariables()) |
bbakerman
left a comment
bbakerman
left a comment
There was a problem hiding this comment.
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
| .document(executionContext.getDocument()) | ||
| .operationName(executionContext.getExecutionInput().getOperationName()) | ||
| .variables(executionContext.getVariables()) | ||
| .coercedVariables(executionContext.getCoercedVariables()) |
| Map<String, Object> variables) { | ||
| assertNotNull(document, () -> "document can't be null"); | ||
| CoercedVariables coercedVariables) { | ||
| this.schema = schema; |
There was a problem hiding this comment.
did you get rid of assertNotNull(document, () -> "document can't be null"); accidentilly here??
There was a problem hiding this comment.
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.
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.
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.
are the asserts removed because these invariants are no longer true?
There was a problem hiding this comment.
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() |
2 participants
This PR fixes double variable coercion in
QueryTraverserand 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
ZonedDateTimecustom scalar parser in the #2819 example acceptsStringandStringValue, but not itself, causing aIllegalArgumentExceptionduring the second coercion.The fix
This PR adds a
QueryTraverserthat handles already coerced variables, thus avoiding the problematic second coercion.In most cases, when a new
QueryTraverseris instantiated, the variables have already been coerced. Following #2773, traversal will usually take place after theExecutionContextis instantiated.