Allowing document compiler to handles directives and values down in the field tree #3757

bbakerman · 2024-11-28T00:25:06Z

Previously the ENO compiler would not respect "immediate" directives on a field.

While it has the QueryDirectives object - this doe not have "Normalised" values for directive arguments.

This PR adds that. This PR is more extensive that I had hoped - but adding Normalised values in multiple places will set us up for betterness later

bbakerman · 2024-11-28T00:25:30Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

        when:
        def result = localCompileToDocument(schema, QUERY, null, fields, noVariables)
-        def printed = AstPrinter.printAst(new AstSorter().sort(result.document))
+        def printed = printDoc(result.document)


just to clean up the tests a little

bbakerman · 2024-11-28T00:26:08Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy



-    def "test a combination of plain objects and interfaces will be all variables"() {
+    def "test a combination of plain objects and interfaces with all variables and no variables"() {


I added two test states here - with all variables and with no variables

bbakerman · 2024-11-28T00:26:28Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

                 v3: [arg1: "barFragArg"],
                 v4: [arg1: "barArg"],
                 v5: [arg1: "fooArg"]]
+


This is the extra test state. Using no variables

bbakerman · 2024-11-28T00:26:53Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

+  listField1(arg: [1, 2, 3])
+  listField2(arg: [{arg1 : "v1", arg2 : [{arg1 : "v1.1"}]}, {arg1 : "v2"}, {arg1 : "v3"}])
+}
+'''


This works - all the values are inlined as expected

bbakerman · 2024-11-28T00:27:19Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

    }

-    private ExecutableNormalizedOperation createNormalizedTree(GraphQLSchema schema, String query, Map<String, Object> variables = [:]) {
+    def "can extract variables or inline values for directives on the query"() {


Added this test for directives - it does NOT work - directives are ignored and removed

github-actions · 2024-11-28T00:27:41Z

Test Results

311 files 311 suites 47s ⏱️
3 568 tests 3 562 ✅ 6 💤 0 ❌
3 657 runs 3 651 ✅ 6 💤 0 ❌

Results for commit 053aac4.

♻️ This comment has been updated with latest results.

bbakerman · 2024-11-28T00:27:55Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

+    bar(barArg: "barArgVar") @optIn(to: ["optToX"])
+  }
+}
+'''


This currently produces

query named { foo(fooArg: "fooArgVar") { bar(barArg: "barArgVar") } }

Which is wrong!!

bbakerman · 2024-11-28T00:28:23Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

+    bar(barArg: $v0)
+  }
+}
+'''


The above is also wrong but I made the assertion pass for now - so I can run both tests. But I know this to be wrong

…or not

…o print them back out - we need NormalisedValues for this

bbakerman · 2024-12-04T03:45:03Z

src/main/java/graphql/execution/directives/QueryDirectivesImpl.java

+                    Directive directive = directives.get(i);
+                    GraphQLDirective graphQLDirective = resolvedDirectives.get(i);
+                    directiveCounterParts.put(graphQLDirective, directive);
+                }


Question - this is mapping the AST directive to its GraphQLDirective counter part by index - does directivesResolver return the list in the same order and size.

bbakerman · 2024-12-05T03:44:51Z

src/main/java/graphql/normalized/ArgumentMaker.java

+ * argument value making.
+ */
+@Internal
+class ArgumentMaker {


as the comment says - breaking out code into its own class

I like the class name, reminds me of this Monty Python sketch https://www.youtube.com/watch?v=uLlv_aZjHXc

I'd like to have an argument please

bbakerman · 2024-12-05T03:47:53Z

src/main/java/graphql/normalized/VariablePredicate.java

+                                       String argName,
+                                       NormalizedInputValue normalizedInputValue) {
+        return false;
+    }


since this is a SPI - we cant require them to implement this. Or we could make it a breaking change say

bbakerman · 2024-12-05T06:23:56Z

src/main/java/graphql/execution/Execution.java

+            normalizedVariableValues = ValuesResolver.getNormalizedVariableValues(graphQLSchema,
+                    variableDefinitions,
+                    inputVariables,
+                    executionInput.getGraphQLContext(), executionInput.getLocale());


This is new - we will coerce variables into rutime form AND Normalised literal form so that the ExecutionContext can have them AND the QueryDirectives can have them in the DFE

bbakerman · 2024-12-05T06:24:15Z

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

+    public NormalizedVariables getNormalizedVariables() {
+        return normalizedVariables;
+    }
+


just passing on NormalizedVariables

bbakerman · 2024-12-05T06:24:52Z

src/main/java/graphql/execution/ExecutionStrategy.java

            QueryDirectives queryDirectives = new QueryDirectivesImpl(field,
                    executionContext.getGraphQLSchema(),
                    executionContext.getCoercedVariables().toMap(),
+                    executionContext.getNormalizedVariables().toMap(),


needed inside to make normalized values where they refeference operation variables

bbakerman · 2024-12-05T06:25:28Z

src/main/java/graphql/execution/NormalizedVariables.java

+    public String toString() {
+        return normalisedVariables.toString();
+    }
+}


We have Raw, Coerced and NormalizedVariables now as distinct classes

bbakerman · 2024-12-05T06:25:46Z

src/main/java/graphql/execution/ValuesResolver.java

-
-        return result;
-
+        return NormalizedVariables.of(result);


distinct class now used

bbakerman · 2024-12-05T06:26:27Z

src/main/java/graphql/execution/directives/DirectivesResolver.java

    }

-    public Map<String, List<GraphQLDirective>> resolveDirectives(List<Directive> directives, GraphQLSchema schema, Map<String, Object> variables, GraphQLContext graphQLContext, Locale locale) {
+    public BiMap<GraphQLDirective, Directive> resolveDirectives(List<Directive> directives, GraphQLSchema schema, Map<String, Object> variables, GraphQLContext graphQLContext, Locale locale) {


The only use of this internal class is QueryDirectivesImpl - so we can change this with impunity

Map<String, List> was a pointless modelling - BiMap<GraphQLDirective, Directive is what we need

bbakerman · 2024-12-05T06:27:46Z

src/main/java/graphql/execution/directives/QueryDirectives.java

+     *
+     * @return a map of applied directive to named argument values
+     */
+    Map<QueryAppliedDirective, Map<String, NormalizedInputValue>> getNormalizedInputValueByImmediateAppliedDirectives();


for a given QueryAppliedDirective - we can get a normalised view of its argument values

bbakerman · 2024-12-05T06:29:55Z

src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java

    private static SelectionSet selectionSet(List<Field> fields) {
        return newSelectionSet().selections(fields).build();
    }



The code below got moved into ArgumentMaker

bbakerman · 2024-12-05T06:31:37Z

src/test/groovy/graphql/execution/values/InputInterceptorTest.groovy

                .graphQLContext({ it.put(InputInterceptor.class, interceptor) })
                .variables(
-                        "booleanArg": "truthy",
+                        "booleanArg": false,


This was never a valid boolean value

bbakerman · 2024-12-05T06:33:01Z

src/test/groovy/graphql/schema/CoercingTest.groovy

+        @Override
+        StringValue valueToLiteral(@NotNull Object input, @NotNull GraphQLContext graphQLContext, @NotNull Locale locale) {
+            return new StringValue(input.toString())
+        }


This is now needed because during "execution" we create normalised values.

This is something contentious. If a SCALAR did not previously have this, they require it now to work.

So we want this. We need to discuss but I think so otherwise I would not have put it in place

That's very interesting! I want to chat about this too

bbakerman · 2024-12-05T06:33:30Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

 import static graphql.normalized.ExecutableNormalizedOperationToAstCompiler.compileToDocumentWithDeferSupport

-abstract class ExecutableNormalizedOperationToAstCompilerTest extends Specification {
+abstract class ENOToAstCompilerTestBase extends Specification {


made a common base - this class is getting WAY to big

bbakerman · 2024-12-05T06:33:57Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

+    static printDoc(Document doc) {
+        return AstPrinter.printAst(sortDoc(doc))
    }
+}


most of the above is moving common test helper code around

bbakerman · 2024-12-05T06:34:35Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

-    name @optIn(to: "devOps")
+  foo1(arg: {arg1 : "Subscription"}) @optIn(to: ["foo"]) {
+    name @optIn(to: ["devOps"])
  }


was NOT correct before

bbakerman · 2024-12-07T03:03:38Z

src/main/java/graphql/execution/Execution.java

+                    executionInput.getGraphQLContext(),
+                    executionInput.getLocale());
+
+            normalizedVariableValues = FpKit.intraThreadMemoize(() ->


I made this a supplier - if not one asks for normalised values of directives - then no work will be done.

QueryDirectivesIpl is also lazy by design -so this wont be asked for until some one asks for QueryDirectives

bbakerman · 2024-12-07T03:06:34Z

src/main/java/graphql/normalized/VariableAccumulator.java

+            return variablePredicate != null && variablePredicate.shouldMakeVariable(executableNormalizedField, queryAppliedDirective, argName, normalizedInputValue);
+        } else {
+            return variablePredicate != null && variablePredicate.shouldMakeVariable(executableNormalizedField, argName, normalizedInputValue);
+        }


this internal call back is the same - when queryAppliedDirective is null - it must be just for a field

This could be added as a comment in the code, that the else branch is only for a field

bbakerman · 2024-12-07T03:07:46Z

src/test/groovy/graphql/normalized/ENOToAstCompilerDirectivesTest.groovy

+                }
+            }
+        }
+    '''


notice all the fragment directives

fragment InnerDetails on Book @timeout(afterMillis: 27)

well they dont get picked up as "immediate" query directives. This is NOT new behavior

Only immediate field directives are captured

bbakerman · 2024-12-09T02:21:36Z

...st/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerDirectivesTest.groovy

 * Test related to ENO and directives
 */
-class ENOToAstCompilerDirectivesTest extends ENOToAstCompilerTestBase {
+class ExecutableNormalizedOperationToAstCompilerDirectivesTest extends ENOToAstCompilerTestBase {


name line up better in IDEA - even though its TOO long a class name

dondonz · 2024-12-12T00:52:13Z

...st/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerDirectivesTest.groovy

+        vars == [v0: "barArgVar", v1: ["optToX"], v2: ["optToY"], v3: "fooArgVar", v4: false]
+        //
+        // the below is what ir currently produces but its WRONG
+        // fix up when the other tests starts to work


Is this test working as expected now?

…ument-compiler # Conflicts: # src/main/java/graphql/execution/Execution.java

…ument-compiler # Conflicts: # src/main/java/graphql/execution/Execution.java # src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java

bbakerman added 2 commits November 28, 2024 10:33

Extra tests to show that the document compiler can inline everything

8f998e2

Extra tests to show that the document compiler can inline everything

32802f2

bbakerman added this to the 23.x breaking changes milestone Nov 28, 2024

bbakerman commented Nov 28, 2024

View reviewed changes

bbakerman added 2 commits November 28, 2024 14:36

Extra tests to show that the document compiler can inline everything …

08c6bb8

…or not

This adds more support for Query directives in ENFs and the ability t…

3e23fea

…o print them back out - we need NormalisedValues for this

bbakerman commented Dec 4, 2024

View reviewed changes

Tweaked code and fixed test

641bea4

bbakerman changed the title ~~Extra tests for document compiler - handles directives and values down in the tree~~ Allowing document compiler to handles directives and values down in the field tree Dec 5, 2024

Added more tests cases to QueryDirectivesIntegrationTest.groovy

f4939e3

bbakerman commented Dec 5, 2024

View reviewed changes

Fixed up tests and made a new test class

6563ef9

bbakerman commented Dec 5, 2024

View reviewed changes

made a supplier of normalised variables

fe13e10

bbakerman commented Dec 7, 2024

View reviewed changes

bbakerman added 2 commits December 9, 2024 10:44

Added more tests

4760249

Added compiler tst for merged fields

883ba63

bbakerman commented Dec 9, 2024

View reviewed changes

Extra comment

e09e6b6

dondonz reviewed Dec 12, 2024

View reviewed changes

bbakerman added 2 commits February 27, 2025 11:36

Merge remote-tracking branch 'origin/master' into extra-tests-for-doc…

4887066

…ument-compiler # Conflicts: # src/main/java/graphql/execution/Execution.java

Merged in master and tweaked Execution.java

bfe691a

dondonz approved these changes Feb 27, 2025

View reviewed changes

bbakerman added 2 commits March 20, 2025 13:12

Merge remote-tracking branch 'origin/master' into extra-tests-for-doc…

95654b5

…ument-compiler # Conflicts: # src/main/java/graphql/execution/Execution.java # src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java

Merged in master

053aac4

bbakerman merged commit 472bb92 into master Mar 20, 2025
2 checks passed



		def "test a combination of plain objects and interfaces will be all variables"() {
		def "test a combination of plain objects and interfaces with all variables and no variables"() {

Allowing document compiler to handles directives and values down in the field tree #3757

Allowing document compiler to handles directives and values down in the field tree #3757

Uh oh!

Conversation

bbakerman commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bbakerman commented Nov 28, 2024 •

edited

Loading

github-actions bot commented Nov 28, 2024 •

edited

Loading