Smarter schema visitor by bbakerman · Pull Request #3087 · graphql-java/graphql-java

bbakerman · 2023-02-06T04:23:16Z

In short I want to schema visitors to easier to use. eg if I am on a field def, can I get its containing type. Not context.getParentObject() and cast it but a type safe mechanism for that.

This is more like the graphql.analysis.QueryVisitor#visitField that exists which gives out stuff like

default TraversalControl visitFieldWithControl(QueryVisitorFieldEnvironment queryVisitorFieldEnvironment) {
        visitField(queryVisitorFieldEnvironment);
        return TraversalControl.CONTINUE;
    }

    void visitInlineFragment(QueryVisitorInlineFragmentEnvironment queryVisitorInlineFragmentEnvironment);

    void visitFragmentSpread(QueryVisitorFragmentSpreadEnvironment queryVisitorFragmentSpreadEnvironment);

    default void visitFragmentDefinition(QueryVisitorFragmentDefinitionEnvironment queryVisitorFragmentDefinitionEnvironment) {

    }

So this has now progressed and I have more environments available - all of the major types plus one catch all GraphqlSchemaElement one

I dont visit Non nulls or Lists - I dont think this is useful

If you really want to do that - then use a plain old TypeVisitor - all the power without the nice ness

bbakerman · 2023-02-06T04:35:42Z

src/main/java/graphql/schema/visitor/GraphQLSmartTypeVisitor.java

+     */
+    interface GraphQLFieldDefinitionVisitorEnvironment extends SmartTypeVisitorEnvironment {
+
+        GraphQLFieldsContainer getFieldsContainer();


One other option here is to NOT have a per method interface but rather put this into method signature

visitGraphQLFieldDefinition(GraphQLFieldDefinition fieldDefinition, GraphQLFieldsContainer container, SmartTypeVisitorEnvironment environment) {

What else would go here to make it smarter ?

What about some callback methods for the intermediate interfaces in the class hierarchy?
Like, I often find myself having to do stuff on all elements that can have directives.
Maybe a callback like visitGraphQLDirectiveContainer(GraphQLDirectiveContainer directiveContainer.....)? GraphQLFieldsContainer is another very common one.
Although that can quickly become convoluted - do we need one for each of those intermediate interfaces (GraphQLNamedInputType, GraphQLNullableType, etc...)? Probably not.

maybe like a dynamic visitor method<T extends GraphQLSchemaElement> visitThing(Class<T> thingClass, BiConsumer<T, Context>) , and in the visitor we could check thingClass.isAssignableFrom(element.getClass()). Something like that...
Not sure if there's already something like that in the API, though.

src/main/java/graphql/schema/visitor/GraphQLSmartTypeVisitor.java

+        return TraversalControl.CONTINUE;
+    }
+
+


bbakerman · 2023-03-06T04:23:32Z

src/main/java/graphql/schema/GraphQLTypeVisitor.java

+     *
     * @return how to control the visitation processing
     */
-    TraversalControl visitGraphQLDirective(GraphQLDirective node, TraverserContext<GraphQLSchemaElement> context);


I ordered all the methods here in alphabetic order so I can know what's what

bbakerman · 2023-03-06T04:24:15Z

src/main/java/graphql/schema/SchemaTransformer.java

        }
+        if (schema != null) {
+            traverser.rootVar(GraphQLSchema.class, schema);
+        }


We now add the source schema as a root variable - which visitors can now access

bbakerman · 2023-03-06T04:24:36Z

src/main/java/graphql/schema/SchemaTraverser.java

        TraverserDelegateListVisitor traverserDelegateListVisitor = new TraverserDelegateListVisitor(typeVisitors);
-        return initTraverser().rootVars(rootVars).traverse(roots, traverserDelegateListVisitor);
+        Traverser<GraphQLSchemaElement> traverser = initTraverser().rootVars(rootVars).rootVar(GraphQLSchema.class, schema);
+        return traverser.traverse(roots, traverserDelegateListVisitor);


Same here on SchemaTraverser - the schema being traversed is now a root variable

bbakerman · 2023-03-06T04:25:12Z

src/main/java/graphql/schema/SchemaTraverser.java

+                TraversalControl control = context.thisNode().accept(context, graphQLTypeVisitor);
+                if (control != CONTINUE) {
+                    return control;
+                }


This was broken. I fixed it and now tests broke

So I wrote a new test for this case - when NOT CONTINUE is returned

bbakerman · 2023-03-06T04:27:43Z

src/main/java/graphql/schema/visitor/GraphQLSchemaTraversalControl.java

@@ -0,0 +1,81 @@
+package graphql.schema.visitor;


We have a different "control" that SchemaTraversalControl - so we can have better methods and "hide" the changeNode malarky we must do.

The only way to get a GraphQLSchemaTraversalControl (which is package level on creation) is by the helper methods and hence this simplifies the call API for consumers

bbakerman · 2023-03-06T04:28:53Z

src/main/java/graphql/schema/visitor/GraphQLSchemaTraversalControl.java

+        }
+        return TraversalControl.CONTINUE;
+    }
+}


See how this hodes the TreeTransformerUtil malarky AND also we can detect intention from the GraphQLSchemaTraversalControl as a value and not a side effect

WIP - Smarter type visitor

f9ce2b3

bbakerman commented Feb 6, 2023

View reviewed changes

src/main/java/graphql/schema/visitor/GraphQLSmartTypeVisitor.java Outdated

return TraversalControl.CONTINUE;

}

This comment was marked as outdated.

Sign in to view

bbakerman added 5 commits February 11, 2023 10:05

Renamed a few things and added per callback envs

c92b102

Now with control methods in the env

b869dce

More work on smarter schema visitors

07def39

Added schema to root vars

33cea4a

Test for changing schema

18bf953

bbakerman changed the title ~~WIP - Smarter type visitor~~ Smarter schema visitor Feb 27, 2023

bbakerman added 2 commits March 6, 2023 12:11

Added a new control type so we can know when thigns have changed

792f1cb

Fixed SchemaTraverser so it can QUIT

9f00d25

bbakerman commented Mar 6, 2023

View reviewed changes

Made class public

1e28acc

bbakerman commented Mar 6, 2023

View reviewed changes

dondonz added this to the 20.1 milestone Mar 14, 2023

andimarek modified the milestones: April 2023, July 2023 Mar 21, 2023

bbakerman added this pull request to the merge queue May 29, 2023

Merged via the queue into master with commit 8649ea6 May 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Smarter schema visitor#3087

Smarter schema visitor#3087
bbakerman merged 9 commits intomasterfrom
smarter_schema_visitors

bbakerman commented Feb 6, 2023 •

edited

Loading

Uh oh!

bbakerman Feb 6, 2023

Uh oh!

bbakerman Feb 6, 2023

Uh oh!

felipe-gdr Feb 6, 2023

Uh oh!

felipe-gdr Feb 6, 2023

Uh oh!

This comment was marked as outdated.

Uh oh!

bbakerman Mar 6, 2023

Uh oh!

bbakerman Mar 6, 2023

Uh oh!

bbakerman Mar 6, 2023

Uh oh!

bbakerman Mar 6, 2023

Uh oh!

bbakerman Mar 6, 2023

Uh oh!

bbakerman Mar 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

bbakerman commented Feb 6, 2023 • 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!

This comment was marked as outdated.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bbakerman commented Feb 6, 2023 •

edited

Loading