Skip to content
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

Cherry pick raw and coerced variable refactor for 18.x branch #2861

Merged
merged 1 commit into from Jun 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 25 additions & 10 deletions src/main/java/graphql/ExecutionInput.java
Expand Up @@ -2,6 +2,7 @@

import graphql.cachecontrol.CacheControl;
import graphql.execution.ExecutionId;
import graphql.execution.RawVariables;
import graphql.execution.instrumentation.dataloader.DataLoaderDispatcherInstrumentationState;
import org.dataloader.DataLoaderRegistry;

Expand All @@ -24,7 +25,7 @@ public class ExecutionInput {
private final GraphQLContext graphQLContext;
private final Object localContext;
private final Object root;
private final Map<String, Object> variables;
private final RawVariables rawVariables;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending how aggressive you are in "sem ver" one could argue this is not a bug fix dues to the new API constructs. But since its backwards compat in all the places. Cool

Copy link
Member Author

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. This bugfix is tricky because it depended on the variables refactor. I figured it would be easier to cherry pick the refactor as well, rather than try crowbar the bugfix in alone

private final Map<String, Object> extensions;
private final DataLoaderRegistry dataLoaderRegistry;
private final CacheControl cacheControl;
Expand All @@ -39,7 +40,7 @@ private ExecutionInput(Builder builder) {
this.context = builder.context;
this.graphQLContext = assertNotNull(builder.graphQLContext);
this.root = builder.root;
this.variables = builder.variables;
this.rawVariables = builder.rawVariables;
this.dataLoaderRegistry = builder.dataLoaderRegistry;
this.cacheControl = builder.cacheControl;
this.executionId = builder.executionId;
Expand Down Expand Up @@ -97,10 +98,17 @@ public Object getRoot() {
}

/**
* @return a map of variables that can be referenced via $syntax in the query
* @return a map of raw variables that can be referenced via $syntax in the query.
*/
public Map<String, Object> getVariables() {
return variables;
return rawVariables.toMap();
}

/**
* @return a map of raw variables that can be referenced via $syntax in the query.
*/
public RawVariables getRawVariables() {
return rawVariables;
}

/**
Expand Down Expand Up @@ -158,7 +166,7 @@ public ExecutionInput transform(Consumer<Builder> builderConsumer) {
.root(this.root)
.dataLoaderRegistry(this.dataLoaderRegistry)
.cacheControl(this.cacheControl)
.variables(this.variables)
.variables(this.rawVariables.toMap())
.extensions(this.extensions)
.executionId(this.executionId)
.locale(this.locale);
Expand All @@ -176,7 +184,7 @@ public String toString() {
", context=" + context +
", graphQLContext=" + graphQLContext +
", root=" + root +
", variables=" + variables +
", rawVariables=" + rawVariables +
", dataLoaderRegistry=" + dataLoaderRegistry +
", executionId= " + executionId +
", locale= " + locale +
Expand Down Expand Up @@ -209,7 +217,7 @@ public static class Builder {
private Object context = graphQLContext; // we make these the same object on purpose - legacy code will get the same object if this change nothing
private Object localContext;
private Object root;
private Map<String, Object> variables = Collections.emptyMap();
private RawVariables rawVariables = RawVariables.emptyVariables();
public Map<String, Object> extensions = Collections.emptyMap();
//
// this is important - it allows code to later known if we never really set a dataloader and hence it can optimize
Expand Down Expand Up @@ -242,7 +250,6 @@ public Builder executionId(ExecutionId executionId) {
return this;
}


/**
* Sets the locale to use for this operation
*
Expand Down Expand Up @@ -351,8 +358,16 @@ public Builder root(Object root) {
return this;
}

public Builder variables(Map<String, Object> variables) {
this.variables = assertNotNull(variables, () -> "variables map can't be null");
/**
* Adds raw (not coerced) variables
*
* @param rawVariables the map of raw variables
*
* @return this builder
*/
public Builder variables(Map<String, Object> rawVariables) {
assertNotNull(rawVariables, () -> "variables map can't be null");
this.rawVariables = new RawVariables(rawVariables);
return this;
}

Expand Down
@@ -1,6 +1,7 @@
package graphql.analysis;

import graphql.Internal;
import graphql.execution.CoercedVariables;
import graphql.execution.ConditionalNodes;
import graphql.execution.ValuesResolver;
import graphql.introspection.Introspection;
Expand Down Expand Up @@ -154,7 +155,7 @@ public TraversalControl visitField(Field field, TraverserContext<Node> context)
boolean isTypeNameIntrospectionField = fieldDefinition == schema.getIntrospectionTypenameFieldDefinition();
GraphQLFieldsContainer fieldsContainer = !isTypeNameIntrospectionField ? (GraphQLFieldsContainer) unwrapAll(parentEnv.getOutputType()) : null;
GraphQLCodeRegistry codeRegistry = schema.getCodeRegistry();
Map<String, Object> argumentValues = valuesResolver.getArgumentValues(codeRegistry, fieldDefinition.getArguments(), field.getArguments(), variables);
Map<String, Object> argumentValues = valuesResolver.getArgumentValues(codeRegistry, fieldDefinition.getArguments(), field.getArguments(), new CoercedVariables(variables));
QueryVisitorFieldEnvironment environment = new QueryVisitorFieldEnvironmentImpl(isTypeNameIntrospectionField,
field,
fieldDefinition,
Expand Down
16 changes: 9 additions & 7 deletions src/main/java/graphql/analysis/QueryTraverser.java
@@ -1,6 +1,8 @@
package graphql.analysis;

import graphql.PublicApi;
import graphql.execution.CoercedVariables;
import graphql.execution.RawVariables;
import graphql.execution.ValuesResolver;
import graphql.language.Document;
import graphql.language.FragmentDefinition;
Expand Down Expand Up @@ -42,26 +44,26 @@ public class QueryTraverser {
private final Collection<? extends Node> roots;
private final GraphQLSchema schema;
private final Map<String, FragmentDefinition> fragmentsByName;
private final Map<String, Object> variables;
private CoercedVariables coercedVariables;

private final GraphQLCompositeType rootParentType;

private QueryTraverser(GraphQLSchema schema,
Document document,
String operation,
Map<String, Object> variables) {
assertNotNull(document, () -> "document can't be null");
assertNotNull(document, () -> "document can't be null");
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.variables = coerceVariables(assertNotNull(variables, () -> "variables can't be null"), variableDefinitions);
this.coercedVariables = coerceVariables(assertNotNull(variables, () -> "variables can't be null"), variableDefinitions);
}

private Map<String, Object> coerceVariables(Map<String, Object> rawVariables, List<VariableDefinition> variableDefinitions) {
return new ValuesResolver().coerceVariableValues(schema, variableDefinitions, rawVariables);
private CoercedVariables coerceVariables(Map<String, Object> rawVariables, List<VariableDefinition> variableDefinitions) {
return new ValuesResolver().coerceVariableValues(schema, variableDefinitions, new RawVariables(rawVariables));
}

private QueryTraverser(GraphQLSchema schema,
Expand All @@ -70,11 +72,11 @@ private QueryTraverser(GraphQLSchema schema,
Map<String, FragmentDefinition> fragmentsByName,
Map<String, Object> variables) {
this.schema = assertNotNull(schema, () -> "schema can't be null");
this.variables = assertNotNull(variables, () -> "variables can't be null");
assertNotNull(root, () -> "root can't be null");
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"));
}

public Object visitDepthFirst(QueryVisitor queryVisitor) {
Expand Down Expand Up @@ -181,7 +183,7 @@ private Object visitImpl(QueryVisitor visitFieldCallback, Boolean preOrder) {
}

NodeTraverser nodeTraverser = new NodeTraverser(rootVars, this::childrenOf);
NodeVisitorWithTypeTracking nodeVisitorWithTypeTracking = new NodeVisitorWithTypeTracking(preOrderCallback, postOrderCallback, variables, schema, fragmentsByName);
NodeVisitorWithTypeTracking nodeVisitorWithTypeTracking = new NodeVisitorWithTypeTracking(preOrderCallback, postOrderCallback, coercedVariables.toMap(), schema, fragmentsByName);
return nodeTraverser.depthFirst(nodeVisitorWithTypeTracking, roots);
}

Expand Down
35 changes: 35 additions & 0 deletions src/main/java/graphql/execution/CoercedVariables.java
@@ -0,0 +1,35 @@
package graphql.execution;

import graphql.Internal;
import graphql.collect.ImmutableKit;
import graphql.collect.ImmutableMapWithNullValues;

import java.util.Map;

/**
* Holds coerced variables
*/
@Internal
public class CoercedVariables {
private final ImmutableMapWithNullValues<String, Object> coercedVariables;

public CoercedVariables(Map<String, Object> coercedVariables) {
this.coercedVariables = ImmutableMapWithNullValues.copyOf(coercedVariables);
}

public Map<String, Object> toMap() {
return coercedVariables;
}

public boolean containsKey(String key) {
return coercedVariables.containsKey(key);
}

public Object get(String key) {
return coercedVariables.get(key);
}

public static CoercedVariables emptyVariables() {
return new CoercedVariables(ImmutableKit.emptyMap());
}
}
2 changes: 1 addition & 1 deletion src/main/java/graphql/execution/ConditionalNodes.java
Expand Up @@ -34,7 +34,7 @@ public boolean shouldInclude(Map<String, Object> variables, List<Directive> dire
private boolean getDirectiveResult(Map<String, Object> variables, List<Directive> directives, String directiveName, boolean defaultValue) {
Directive foundDirective = NodeUtil.findNodeByName(directives, directiveName);
if (foundDirective != null) {
Map<String, Object> argumentValues = valuesResolver.getArgumentValues(SkipDirective.getArguments(), foundDirective.getArguments(), variables);
Map<String, Object> argumentValues = valuesResolver.getArgumentValues(SkipDirective.getArguments(), foundDirective.getArguments(), new CoercedVariables(variables));
Object flag = argumentValues.get("if");
Assert.assertTrue(flag instanceof Boolean, () -> String.format("The '%s' directive MUST have a value for the 'if' argument", directiveName));
return (Boolean) flag;
Expand Down
7 changes: 3 additions & 4 deletions src/main/java/graphql/execution/Execution.java
Expand Up @@ -30,7 +30,6 @@
import static graphql.execution.ExecutionContextBuilder.newExecutionContextBuilder;
import static graphql.execution.ExecutionStepInfo.newExecutionStepInfo;
import static graphql.execution.ExecutionStrategyParameters.newParameters;
import static graphql.execution.nextgen.Common.getOperationRootType;
import static graphql.language.OperationDefinition.Operation.MUTATION;
import static graphql.language.OperationDefinition.Operation.QUERY;
import static graphql.language.OperationDefinition.Operation.SUBSCRIPTION;
Expand Down Expand Up @@ -62,10 +61,10 @@ public CompletableFuture<ExecutionResult> execute(Document document, GraphQLSche
Map<String, FragmentDefinition> fragmentsByName = getOperationResult.fragmentsByName;
OperationDefinition operationDefinition = getOperationResult.operationDefinition;

Map<String, Object> inputVariables = executionInput.getVariables();
RawVariables inputVariables = executionInput.getRawVariables();
List<VariableDefinition> variableDefinitions = operationDefinition.getVariableDefinitions();

Map<String, Object> coercedVariables;
CoercedVariables coercedVariables;
try {
coercedVariables = valuesResolver.coerceVariableValues(graphQLSchema, variableDefinitions, inputVariables);
} catch (RuntimeException rte) {
Expand All @@ -88,7 +87,7 @@ public CompletableFuture<ExecutionResult> execute(Document document, GraphQLSche
.localContext(executionInput.getLocalContext())
.root(executionInput.getRoot())
.fragmentsByName(fragmentsByName)
.variables(coercedVariables)
.coercedVariables(coercedVariables)
.document(document)
.operationDefinition(operationDefinition)
.dataLoaderRegistry(executionInput.getDataLoaderRegistry())
Expand Down
17 changes: 12 additions & 5 deletions src/main/java/graphql/execution/ExecutionContext.java
Expand Up @@ -9,7 +9,6 @@
import graphql.PublicApi;
import graphql.cachecontrol.CacheControl;
import graphql.collect.ImmutableKit;
import graphql.collect.ImmutableMapWithNullValues;
import graphql.execution.instrumentation.Instrumentation;
import graphql.execution.instrumentation.InstrumentationState;
import graphql.language.Document;
Expand Down Expand Up @@ -43,7 +42,7 @@ public class ExecutionContext {
private final ImmutableMap<String, FragmentDefinition> fragmentsByName;
private final OperationDefinition operationDefinition;
private final Document document;
private final ImmutableMapWithNullValues<String, Object> variables;
private final CoercedVariables coercedVariables;
private final Object root;
private final Object context;
private final GraphQLContext graphQLContext;
Expand All @@ -66,7 +65,7 @@ public class ExecutionContext {
this.mutationStrategy = builder.mutationStrategy;
this.subscriptionStrategy = builder.subscriptionStrategy;
this.fragmentsByName = builder.fragmentsByName;
this.variables = ImmutableMapWithNullValues.copyOf(builder.variables);
this.coercedVariables = builder.coercedVariables;
this.document = builder.document;
this.operationDefinition = builder.operationDefinition;
this.context = builder.context;
Expand All @@ -80,7 +79,7 @@ public class ExecutionContext {
this.errors.set(builder.errors);
this.localContext = builder.localContext;
this.executionInput = builder.executionInput;
queryTree = FpKit.interThreadMemoize(() -> ExecutableNormalizedOperationFactory.createExecutableNormalizedOperation(graphQLSchema, operationDefinition, fragmentsByName, variables));
queryTree = FpKit.interThreadMemoize(() -> ExecutableNormalizedOperationFactory.createExecutableNormalizedOperation(graphQLSchema, operationDefinition, fragmentsByName, coercedVariables));
}


Expand Down Expand Up @@ -116,8 +115,16 @@ public OperationDefinition getOperationDefinition() {
return operationDefinition;
}

/**
* @deprecated use {@link #getCoercedVariables()} instead
*/
@Deprecated
public Map<String, Object> getVariables() {
return variables;
return coercedVariables.toMap();
}

public CoercedVariables getCoercedVariables() {
return coercedVariables;
}

/**
Expand Down
16 changes: 12 additions & 4 deletions src/main/java/graphql/execution/ExecutionContextBuilder.java
Expand Up @@ -9,7 +9,6 @@
import graphql.PublicApi;
import graphql.cachecontrol.CacheControl;
import graphql.collect.ImmutableKit;
import graphql.collect.ImmutableMapWithNullValues;
import graphql.execution.instrumentation.Instrumentation;
import graphql.execution.instrumentation.InstrumentationState;
import graphql.language.Document;
Expand Down Expand Up @@ -39,7 +38,7 @@ public class ExecutionContextBuilder {
Object root;
Document document;
OperationDefinition operationDefinition;
ImmutableMapWithNullValues<String, Object> variables = ImmutableMapWithNullValues.emptyMap();
CoercedVariables coercedVariables = CoercedVariables.emptyVariables();
ImmutableMap<String, FragmentDefinition> fragmentsByName = ImmutableKit.emptyMap();
DataLoaderRegistry dataLoaderRegistry;
CacheControl cacheControl;
Expand Down Expand Up @@ -86,7 +85,7 @@ public ExecutionContextBuilder() {
root = other.getRoot();
document = other.getDocument();
operationDefinition = other.getOperationDefinition();
variables = ImmutableMapWithNullValues.copyOf(other.getVariables());
coercedVariables = other.getCoercedVariables();
fragmentsByName = ImmutableMap.copyOf(other.getFragmentsByName());
dataLoaderRegistry = other.getDataLoaderRegistry();
cacheControl = other.getCacheControl();
Expand Down Expand Up @@ -151,8 +150,17 @@ public ExecutionContextBuilder root(Object root) {
return this;
}

/**
* @deprecated use {@link #coercedVariables(CoercedVariables)} instead
*/
@Deprecated
public ExecutionContextBuilder variables(Map<String, Object> variables) {
this.variables = ImmutableMapWithNullValues.copyOf(variables);
this.coercedVariables = new CoercedVariables(variables);
return this;
}

public ExecutionContextBuilder coercedVariables(CoercedVariables coercedVariables) {
this.coercedVariables = coercedVariables;
return this;
}

Expand Down
Expand Up @@ -27,7 +27,7 @@ public ExecutionStepInfo newExecutionStepInfoForSubField(ExecutionContext execut
GraphQLOutputType fieldType = fieldDefinition.getType();
List<Argument> fieldArgs = mergedField.getArguments();
GraphQLCodeRegistry codeRegistry = executionContext.getGraphQLSchema().getCodeRegistry();
Supplier<Map<String, Object>> argumentValues = FpKit.intraThreadMemoize(() -> valuesResolver.getArgumentValues(codeRegistry, fieldDefinition.getArguments(), fieldArgs, executionContext.getVariables()));
Supplier<Map<String, Object>> argumentValues = FpKit.intraThreadMemoize(() -> valuesResolver.getArgumentValues(codeRegistry, fieldDefinition.getArguments(), fieldArgs, executionContext.getCoercedVariables()));

ResultPath newPath = parentInfo.getPath().segment(mergedField.getResultKey());

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/graphql/execution/ExecutionStrategy.java
Expand Up @@ -817,7 +817,7 @@ protected ExecutionStepInfo createExecutionStepInfo(ExecutionContext executionCo
if (!fieldArgDefs.isEmpty()) {
List<Argument> fieldArgs = field.getArguments();
GraphQLCodeRegistry codeRegistry = executionContext.getGraphQLSchema().getCodeRegistry();
argumentValues = FpKit.intraThreadMemoize(() -> valuesResolver.getArgumentValues(codeRegistry, fieldArgDefs, fieldArgs, executionContext.getVariables()));
argumentValues = FpKit.intraThreadMemoize(() -> valuesResolver.getArgumentValues(codeRegistry, fieldArgDefs, fieldArgs, executionContext.getCoercedVariables()));
}


Expand Down