-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. nice |
||
.build(); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,32 +51,38 @@ public class QueryTraverser { | |
private QueryTraverser(GraphQLSchema schema, | ||
Document document, | ||
String operation, | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. did you get rid of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
NodeUtil.GetOperationResult getOperationResult = NodeUtil.getOperation(document, operation); | ||
List<VariableDefinition> variableDefinitions = getOperationResult.operationDefinition.getVariableDefinitions(); | ||
this.schema = assertNotNull(schema, () -> "schema can't be null"); | ||
this.fragmentsByName = getOperationResult.fragmentsByName; | ||
this.roots = singletonList(getOperationResult.operationDefinition); | ||
this.rootParentType = getRootTypeFromOperation(getOperationResult.operationDefinition); | ||
this.coercedVariables = coerceVariables(assertNotNull(variables, () -> "variables can't be null"), variableDefinitions); | ||
this.coercedVariables = coercedVariables; | ||
} | ||
|
||
private CoercedVariables coerceVariables(Map<String, Object> rawVariables, List<VariableDefinition> variableDefinitions) { | ||
return new ValuesResolver().coerceVariableValues(schema, variableDefinitions, new RawVariables(rawVariables)); | ||
private QueryTraverser(GraphQLSchema schema, | ||
Document document, | ||
String operation, | ||
RawVariables rawVariables) { | ||
this.schema = schema; | ||
NodeUtil.GetOperationResult getOperationResult = NodeUtil.getOperation(document, operation); | ||
List<VariableDefinition> variableDefinitions = getOperationResult.operationDefinition.getVariableDefinitions(); | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This is one for Andi omre yhan you - but since There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
private QueryTraverser(GraphQLSchema schema, | ||
Node root, | ||
GraphQLCompositeType rootParentType, | ||
Map<String, FragmentDefinition> fragmentsByName, | ||
Map<String, Object> variables) { | ||
this.schema = assertNotNull(schema, () -> "schema can't be null"); | ||
assertNotNull(root, () -> "root can't be null"); | ||
CoercedVariables coercedVariables) { | ||
this.schema = schema; | ||
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 commentThe 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 commentThe 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 |
||
this.rootParentType = rootParentType; | ||
this.fragmentsByName = fragmentsByName; | ||
this.coercedVariables = coercedVariables; | ||
} | ||
|
||
public Object visitDepthFirst(QueryVisitor queryVisitor) { | ||
|
@@ -196,7 +202,8 @@ public static class Builder { | |
private GraphQLSchema schema; | ||
private Document document; | ||
private String operation; | ||
private Map<String, Object> variables; | ||
private CoercedVariables coercedVariables = CoercedVariables.emptyVariables(); | ||
private RawVariables rawVariables; | ||
|
||
private Node root; | ||
private GraphQLCompositeType rootParentType; | ||
|
@@ -211,7 +218,7 @@ public static class Builder { | |
* @return this builder | ||
*/ | ||
public Builder schema(GraphQLSchema schema) { | ||
this.schema = schema; | ||
this.schema = assertNotNull(schema, () -> "schema can't be null"); | ||
return this; | ||
} | ||
|
||
|
@@ -237,19 +244,33 @@ public Builder operationName(String operationName) { | |
* @return this builder | ||
*/ | ||
public Builder document(Document document) { | ||
this.document = document; | ||
this.document = assertNotNull(document, () -> "document can't be null"); | ||
return this; | ||
} | ||
|
||
/** | ||
* Variables used in the query. | ||
* Raw variables used in the query. | ||
* | ||
* @param variables the variables to use | ||
* | ||
* @return this builder | ||
*/ | ||
public Builder variables(Map<String, Object> variables) { | ||
this.variables = variables; | ||
assertNotNull(variables, () -> "variables can't be null"); | ||
this.rawVariables = new RawVariables(variables); | ||
return this; | ||
} | ||
|
||
/** | ||
* Variables (already coerced) used in the query. | ||
* | ||
* @param coercedVariables the variables to use | ||
* | ||
* @return this builder | ||
*/ | ||
public Builder coercedVariables(CoercedVariables coercedVariables) { | ||
assertNotNull(coercedVariables, () -> "coercedVariables can't be null"); | ||
this.coercedVariables = coercedVariables; | ||
return this; | ||
} | ||
|
||
|
@@ -262,7 +283,7 @@ public Builder variables(Map<String, Object> variables) { | |
* @return this builder | ||
*/ | ||
public Builder root(Node root) { | ||
this.root = root; | ||
this.root = assertNotNull(root, () -> "root can't be null"); | ||
return this; | ||
} | ||
|
||
|
@@ -274,7 +295,7 @@ public Builder root(Node root) { | |
* @return this builder | ||
*/ | ||
public Builder rootParentType(GraphQLCompositeType rootParentType) { | ||
this.rootParentType = rootParentType; | ||
this.rootParentType = assertNotNull(rootParentType, () -> "rootParentType can't be null"); | ||
return this; | ||
} | ||
|
||
|
@@ -286,7 +307,7 @@ public Builder rootParentType(GraphQLCompositeType rootParentType) { | |
* @return this builder | ||
*/ | ||
public Builder fragmentsByName(Map<String, FragmentDefinition> fragmentsByName) { | ||
this.fragmentsByName = fragmentsByName; | ||
this.fragmentsByName = assertNotNull(fragmentsByName, () -> "fragmentsByName can't be null"); | ||
return this; | ||
} | ||
|
||
|
@@ -296,9 +317,18 @@ public Builder fragmentsByName(Map<String, FragmentDefinition> fragmentsByName) | |
public QueryTraverser build() { | ||
checkState(); | ||
if (document != null) { | ||
return new QueryTraverser(schema, document, operation, variables); | ||
if (rawVariables != null) { | ||
return new QueryTraverser(schema, document, operation, rawVariables); | ||
} | ||
return new QueryTraverser(schema, document, operation, coercedVariables); | ||
} else { | ||
return new QueryTraverser(schema, root, rootParentType, fragmentsByName, variables); | ||
if (rawVariables != null) { | ||
// When traversing with an arbitrary root, there is no variable definition context available | ||
// Thus, the variables must have already been coerced | ||
// Retaining this builder for backwards compatibility | ||
return new QueryTraverser(schema, root, rootParentType, fragmentsByName, new CoercedVariables(rawVariables.toMap())); | ||
} | ||
return new QueryTraverser(schema, root, rootParentType, fragmentsByName, coercedVariables); | ||
} | ||
} | ||
|
||
|
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!