Raw and Coerced Variables refactor #2802

dondonz · 2022-04-18T06:06:38Z

Why refactor? Identifying double coercion

Variables were previously represented as a Map<String, Object>, which made it difficult to determine if variables were raw or had been already coerced.

Example: double coercion in `FieldValidationInstrumentation`

Let's start from this line in Execution#executeOperation. At this point, variables have already been coerced. The ExecutionContext can only contain coerced variables.

InstrumentationContext<ExecutionResult> executeOperationCtx = instrumentation.beginExecuteOperation(instrumentationParams);

Then inside FieldValidationInstrumentation#beginExecuteOperation we have

List<GraphQLError> errors = FieldValidationSupport.validateFieldsAndArguments(fieldValidation, parameters.getExecutionContext());

Within validateFieldsAndArguments, we create a QueryTraverser, which coerces variables a second time.

Example: double coercion in WIP PR to shift max query depth after validation

https://github.com/graphql-java/graphql-java/pull/2773/files#diff-2696e88b9e4d095eea4d0e47b1c98ea872bfce3642c5aaee7317b1c3c2c478a5R149

This is a WIP PR so I don't want to be harsh, nevertheless it's a good illustration of how confusing raw and coerced variables are. The ExecutionContext can only contain coerced variables. The linked QueryTraverser will coerce variables a second time.

This PR adds `RawVariables` and `CoercedVariables`

This PR adds RawVariables and CoercedVariables to make the distinction clearer and avoid accidental double coercion.

This PR addresses #2764, and sets the foundation for #2773.

This is a large PR! To make it easier to see the changes, please read commit by commit (or alternatively, I can split this work into multiple PRs).

What did change

I refactored variables to RawVariables or CoercedVariables in crucial classes related to Execution and classes required for the upcoming traverser changes in #2773.

What didn't change

There are other classes containing a map of variables such as FieldCollectionParameters. These classes have been left unchanged primarily to keep the scope of this PR manageable. In these unchanged classes, there was far less ambiguity about coercion. I can extend RawVariables and CoercedVariables to all other classes in another PR.

What is in the next PR

After this PR is merged in I can help finish off #2773 and fix both double coercion examples I outlined above.

… calls

dondonz · 2022-04-18T10:54:22Z

src/main/java/graphql/ExecutionInput.java

@@ -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;


Variables in ExecutionInput are always raw

dondonz · 2022-04-18T11:00:05Z

src/main/java/graphql/analysis/QueryTraverser.java

        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);


For now, keep functionality similar to before

In the next PR (#2773), QueryTraverser will change to accept already coerced variables

dondonz · 2022-04-18T11:00:48Z

src/main/java/graphql/execution/CoercedVariables.java

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


Using this special map as variable values can be null

dondonz · 2022-04-18T11:02:40Z

src/main/java/graphql/execution/ExecutionContext.java

@@ -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;


ExecutionContext can only contain coerced variables. This change makes it clearer

dondonz · 2022-04-18T11:03:28Z

src/main/java/graphql/execution/RawVariables.java

+ */
+@Internal
+public class RawVariables {
+    private final ImmutableMapWithNullValues<String, Object> rawVariables;


Using this special map as values can be null

dondonz · 2022-04-18T11:07:56Z

src/test/groovy/graphql/execution/ConditionalNodesTest.groovy


        def argument = Argument.newArgument("if", new BooleanValue(true)).build()
        def directives = [Directive.newDirective().name("skip").arguments([argument]).build()]

-        conditionalNodes.valuesResolver.getArgumentValues(Directives.SkipDirective.getArguments(), [argument], variables) >> ["if": true]
-


This mock wasn't necessary

dondonz · 2022-04-18T11:08:34Z

src/test/groovy/graphql/execution/ValuesResolverTest.groovy

        then:
-        resolvedValues['variable'] == outputValue
+        resolvedValues.get('variable') == outputValue


Most API changes in this test file

bbakerman · 2022-05-10T05:17:28Z

src/main/java/graphql/execution/ExecutionContextBuilder.java

@@ -39,7 +39,7 @@ public class ExecutionContextBuilder {
    Object root;
    Document document;
    OperationDefinition operationDefinition;
-    ImmutableMapWithNullValues<String, Object> variables = ImmutableMapWithNullValues.emptyMap();
+    CoercedVariables coercedVariables = new CoercedVariables(Collections.emptyMap());


how about CoercedVariables.emptyVariables() - reads nicer - not essential

bbakerman · 2022-05-10T05:19:10Z

src/main/java/graphql/execution/CoercedVariables.java

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


map name it toMap that is you are converting it from a X -> Y - from a CoercedVariables to a map representtion

bbakerman · 2022-05-10T05:20:32Z

src/test/groovy/graphql/ExecutionInputTest.groovy

+        executionInput.locale == Locale.GERMAN
+        executionInput.extensions == [some: "map"]
+        executionInput.query == "new query"
+    }


What happens if we set both

.rawVariables(x).variables(y)

Should we have a test for this. Do they go into the same place?

I'll add a test.

Yes, in ExecutionInput both rawVariables and variables set the same field. If both get set, the last one would be used (in your example y).

Is it worthwhile preventing both .rawVariables(x).variables(y) to be used? I don't mind adding another check if it'll be clearer

bbakerman

I generally appove strongly of this

See my specific comments on naming.

…toMap

bbakerman

As we discussed in person - lets remove the RawVariables fron ExecutionInput signature but use it on the inside

# Conflicts: # src/main/java/graphql/ExecutionInput.java # src/main/java/graphql/execution/ValuesResolver.java # src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java

bbakerman

Merge ! Merge ! Merge!!

dondonz added 10 commits April 17, 2022 19:01

Add raw and coerced variables

1f7f58f

Add RawVariables to ExecutionInput

5ff6353

Add more ExecutionInput tests

2614e4b

Add coerced variables to ExecutionContext

0c51d2e

Add ExecutionContext transformer test and tidy up

c98a615

Add coerced and raw variables to Execution

409c6bb

Add coerced and raw variables to ExecutableNormalizedOperationFactory

7c38af4

Add raw and coerced variables to ValuesResolver and update all method…

86af871

… calls

Update tests after ValuesResolver refactor

f1fe14a

Add coerced variables to Query Traverser

61198b5

dondonz commented Apr 18, 2022

View reviewed changes

dondonz added 2 commits May 4, 2022 08:52

Merge branch 'master' into 2764-coerced-variables-refactor

6240769

Merge branch 'master' into 2764-coerced-variables-refactor

88a69c2

dondonz requested review from andimarek and bbakerman May 7, 2022 06:42

bbakerman reviewed May 10, 2022

View reviewed changes

bbakerman approved these changes May 10, 2022

View reviewed changes

dondonz added 2 commits May 14, 2022 18:19

Create empty versions of variables for convenience, rename getMap to …

d87ea6d

…toMap

Add tests where both map and boxed variables are set

c0e10be

bbakerman requested changes May 19, 2022

View reviewed changes

dondonz added 2 commits May 24, 2022 12:38

Merge branch 'master' into 2764-coerced-variables-refactor

bdd907f

Revert to one variable builder for ExecutionInput and tidy up

7bf2bd5

Merge branch 'master' into 2764-coerced-variables-refactor

84ae2d3

# Conflicts: # src/main/java/graphql/ExecutionInput.java # src/main/java/graphql/execution/ValuesResolver.java # src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java

bbakerman approved these changes May 24, 2022

View reviewed changes

bbakerman merged commit 86d5443 into graphql-java:master May 24, 2022

bbakerman added this to the 19.0 milestone May 24, 2022

dondonz mentioned this pull request Jun 16, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raw and Coerced Variables refactor #2802

Raw and Coerced Variables refactor #2802

dondonz commented Apr 18, 2022 •

edited

dondonz Apr 18, 2022

dondonz Apr 18, 2022

dondonz Apr 18, 2022

dondonz Apr 18, 2022

dondonz Apr 18, 2022

dondonz Apr 18, 2022

dondonz Apr 18, 2022

bbakerman May 10, 2022

bbakerman May 10, 2022

bbakerman May 10, 2022

dondonz May 14, 2022

bbakerman left a comment

bbakerman left a comment

bbakerman left a comment

Raw and Coerced Variables refactor #2802

Raw and Coerced Variables refactor #2802

Conversation

dondonz commented Apr 18, 2022 • edited

Why refactor? Identifying double coercion

Example: double coercion in FieldValidationInstrumentation

Example: double coercion in WIP PR to shift max query depth after validation

This PR adds RawVariables and CoercedVariables

What did change

What didn't change

What is in the next PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbakerman left a comment

Choose a reason for hiding this comment

bbakerman left a comment

Choose a reason for hiding this comment

bbakerman left a comment

Choose a reason for hiding this comment

dondonz commented Apr 18, 2022 •

edited

Example: double coercion in `FieldValidationInstrumentation`

This PR adds `RawVariables` and `CoercedVariables`