Make all Nodes immutable but still easy to "modify" #1335

andimarek · 2018-12-01T09:52:26Z

The goal of this PR to make all Nodes immutable, but "modifying" the Ast should be still easy to.

This means that you can easily get a new tree when you modify a Node, even if the Node is deep inside the tree.

contains contribution from @felipe-gdr :-)

add NodePosition to general Traverser AstTransformer WIP

renamed ChildContainer to NodeChildContainer general named children in Traverser

andimarek · 2018-12-02T02:46:54Z

There still might be some improvements, but I expect that all major parts for that change are in place:

Every Node is now immutable.

It introduces the concept of a Zipper: basically a pointer to a specific Node in a immutable Tree which also keeps track of where where the current Node is in respect to the parent. (see here )

Some background details: the problem with an immutable Ast is, that it is very easy to change a Node and all the children, but is very hard then also to change the parents referencing the node you just changed, because a Node just knows about its children, nothing about it parent. Zipper is a general concept which helps with that. I can recommend http://learnyouahaskell.com/zippers as a more detailed explanation. I also took inspiration in implementing it from this book. (Warning: it actually talks about Haskell and not Java)

Additionally there is the class AstMultiZipper which is a list of Zippers and allows keeping track of multiple changes in a Tree at the same time.

AstTransformer now allows for modifying the Ast (Document) and getting a new Tree back in a very convenient way by leveraging AstMultiZipper.

It will look like this:

graphql-java/src/test/groovy/graphql/language/AstTransformerTest.groovy

Lines 13 to 33 in 5558e3d

    
           def document = TestUtil.parseQuery("{ root { foo { midA { leafA } midB { leafB } } bar { midC { leafC } midD { leafD } } } }") 
        
           AstTransformer astTransformer = new AstTransformer() 
        
           def visitor = new NodeVisitorStub() { 
        
               @Override 
        
               TraversalControl visitField(Field node, TraverserContext<Node> context) { 
        
                   if (!node.name.startsWith("mid")) { 
        
                       return TraversalControl.CONTINUE 
        
                   } 
        
                   String newName = node.name + "-modified" 
        
                   Field changedField = node.transform({ builder -> builder.name(newName) }) 
        
                   changeNode(context, changedField) 
        
                   return TraversalControl.CONTINUE 
        
               } 
        
           } 
        
           when: 
        
           def newDocument = astTransformer.transform(document, visitor)

Feedback welcome.

Also: Now that every Node is immutable you can't change a query anymore at any point. If you have some use cases which requires changing the query after it was create, please comment here. We wanna offer specific hooks where a query can be modified.

andimarek · 2018-12-02T02:50:01Z

Ping @tinnou for feedback. thanks

bbakerman · 2018-12-03T06:45:47Z

src/main/java/graphql/execution2/result/ExecutionResultMultiZipper.java

        }
        ExecutionResultNode newNode = parent.withNewChildren(newChildren);
        return new ExecutionResultZipper(newNode, restBreadcrumbs);
    }


bbakerman · 2018-12-03T06:46:29Z

src/main/java/graphql/execution2/result/ExecutionResultMultiZipper.java

    private Map<ExecutionResultNode, List<ExecutionResultZipper>> zipperWithSameParent(List<ExecutionResultZipper> zippers) {
+        //todo: change to LinkedHashMap
        return zippers.stream().collect(Collectors.groupingBy(ExecutionResultZipper::getParent));
    }


TODO ^

I think you put in a supplier of the Map

Yeah I reckon create its own helper method

bbakerman · 2018-12-03T06:48:59Z

src/main/java/graphql/language/AstMultiZipper.java

+            curZippers.removeAll(deepestZippers);
+            curZippers.addAll(newZippers);
+        }
+        assertTrue(curZippers.size() == 1, "illegal state");


better assertion message - eg state the precondition better or have no message

bbakerman · 2018-12-03T06:51:25Z

src/main/java/graphql/language/Directive.java

    private final String name;
    private final List<Argument> arguments = new ArrayList<>();

+    private static final String CHILD_ARGUMENTS = "arguments";


not sure on the CHILD_ smurfing

The thing that represents "arguments" could be called arguments

bbakerman · 2018-12-03T06:56:29Z

src/main/java/graphql/language/FloatValue.java

+            throw new IllegalArgumentException("Cannot pass non-empty newChildren to Node that doesn't hold children");
+        }
+
+        return this;


This one does not seem to be finished.

Is this a BUG?

well: you can't add children to a FloatValue, so we only allow empty children here

I would create a helper method for these checks

@Override public FloatValue withNewChildren(NodeChildrenContainer newChildren) { NodeChildrenContainer.assertEmpty(newChildren); return this; }

done via helper method

bbakerman · 2018-12-03T06:58:58Z

src/main/java/graphql/language/NullValue.java

+        }
+
+        return this;
+    }


Yeah - defintely common method if this is needed

@Override public NullValue withNewChildren(NodeChildrenContainer newChildren) { return NodeChildrenContainer.assertNotEmpty(newChildren); }

refactored it

bbakerman · 2018-12-03T07:00:14Z

src/main/java/graphql/util/DefaultTraverserContext.java

+    /**
+     * PRIVATE: Used by {@link Traverser}
+     */
+    void setCurAccValue(Object curAccValue) {


bad Javadoc - its going to lead to warnings and set of my OCD on having a clean command line compile / build

changed it no java doc

src/main/java/graphql/util/NodePosition.java

+        }
+        NodePosition that = (NodePosition) o;
+        return index == that.index &&
+                Objects.equals(name, that.name);


bbakerman · 2018-12-03T07:04:14Z

src/test/groovy/graphql/language/AstMultiZipperTest.groovy

+        def midA = rootField.selectionSet.children[0] as Field
+        def leafA = midA.selectionSet.children[0] as Field
+        def leafB = midA.selectionSet.children[0] as Field
+


I think the indices here are wrong.

def leafB = midA.selectionSet.children[1] as Field

here and below

bbakerman · 2018-12-03T07:04:34Z

src/test/groovy/graphql/language/AstMultiZipperTest.groovy

+
+        def midB = rootField.selectionSet.children[1] as Field
+        def leafC = midB.selectionSet.children[0] as Field
+        def leafD = midB.selectionSet.children[0] as Field


indices are wrong I think

bbakerman · 2018-12-11T21:27:22Z

src/main/java/graphql/language/Argument.java

+        return NodeChildrenContainer.newNodeChildrenContainer()
+                .child(CHILD_VALUE, value)
+                .build();
+    }


I would do static import in all the places to reduce the boiler plate

@Override public NodeChildrenContainer getNamedChildren() { return newNodeChildrenContainer() .child(CHILD_VALUE, value) .build(); }

bbakerman · 2018-12-11T21:28:24Z

src/main/java/graphql/language/AstBreadcrumb.java

+import java.util.Objects;
+
+@PublicApi
+public class AstBreadcrumb {


I woukld still love to see JavaDoc at the top to outline the class.

bbakerman · 2018-12-11T21:28:57Z

src/main/java/graphql/language/AstMultiZipper.java

+import static java.util.stream.Collectors.toList;
+
+@PublicApi
+public class AstMultiZipper {


bbakerman · 2018-12-11T21:34:01Z

src/main/java/graphql/language/AstZipper.java

@@ -0,0 +1,85 @@
+package graphql.language;


Should all this transformation code be in graphql.language or one down in say graphql.language.transformation ?

I think the latter

AstZipper and AstMultiZipper is actually not coupled to transformations: it is a general concept.

the only two transformation classes are AstTransformer and AstTransformerUtil. Extra package for them?

bbakerman · 2018-12-11T21:35:22Z

src/main/java/graphql/language/AstZipper.java

+    }
+
+    public AstZipper moveUp() {
+        return null;


is this not done???

and hence not tested??

I removed all not used methods for now.

bbakerman · 2018-12-11T21:39:10Z

src/main/java/graphql/language/IntValue.java

+
+    @Override
+    public IntValue withNewChildren(NodeChildrenContainer newChildren) {
+        assertNewChildrenAreEmpty(newChildren);


Are you have one. Do it on FLoatValue

done ... I forgot FloatValue somehow

bbakerman · 2018-12-11T21:39:34Z

src/main/java/graphql/language/Node.java

     */
    List<Node> getChildren();

+    NodeChildrenContainer getNamedChildren();


bbakerman · 2018-12-11T21:40:20Z

src/main/java/graphql/language/NodeChildrenContainer.java

+import static graphql.Assert.assertNotNull;
+
+@PublicApi
+public class NodeChildrenContainer {


JavaDoc?

Maybe a seperate PR for the JavaDoc say

bbakerman · 2018-12-11T22:05:41Z

src/test/groovy/graphql/util/TraverserTest.groovy

+                leave: { TraverserContext context ->
+                    TraversalControl.CONTINUE
+                }
+        ] as TraverserVisitor


what is this crazy syntax?? I didnt know you could do this! :)

a groovy map with closures/functions as value ... not so crazy, or?

TraverserState

AstMultiZipper fix for changing the structure in multiple nodes more AstTransformer tests

kaqqao · 2018-12-19T14:55:03Z

Now that every Node is immutable you can't change a query anymore at any point. If you have some use cases which requires changing the query after it was create, please comment here.

Perhaps I'm too late, but I'm aware of users that allow queries for objects without a subselection, by modifying the query on the fly to include all the direct fields in such cases. This was done via an instrumentation.
Is this use case still possible?

kaqqao · 2018-12-20T01:54:08Z

Ah, just seen #1345 which caters exactly to this use-case :)

andimarek added 2 commits December 1, 2018 20:42

add ast zipper, all Nodes immutable

1f67b2c

naming, cleanup

05fb8cc

andimarek added this to the 12.0 milestone Dec 1, 2018

andimarek mentioned this pull request Dec 1, 2018

Spike: Add a zipper to mutate Ast (WIP) #1148

Closed

andimarek added 5 commits December 1, 2018 22:05

add AstMultiZipper

5c575f0

AstMultiZipper tests

9a145f4

refactor Traversal

1cc1222

add NodePosition to general Traverser AstTransformer WIP

AstTransformer first test green

40a4f7b

renamed ChildContainer to NodeChildContainer general named children in Traverser

make test nicer

5f11a73

andimarek added the breaking change requires a new major version to be relased label Dec 2, 2018

andimarek requested review from bbakerman and felipe-gdr December 2, 2018 03:06

bbakerman reviewed Dec 3, 2018

View reviewed changes

src/main/java/graphql/util/NodePosition.java

}

NodePosition that = (NodePosition) o;

return index == that.index &&

Objects.equals(name, that.name);

This comment was marked as outdated.

Sign in to view

bbakerman reviewed Dec 3, 2018

View reviewed changes

andimarek added 6 commits December 11, 2018 16:59

use linkedHashMap for groupBy

7a9f6cf

better error message

7b86cee

non javadoc

8000977

cleanup

e7639a8

make all child String constants public

ee8c62d

add static helper method

cb72a31

bbakerman reviewed Dec 11, 2018

View reviewed changes

andimarek and others added 17 commits December 12, 2018 13:23

move assertion into helper method

7952846

remove not needed methods

3a345de

javadoc

49525f4

javadoc

8436b61

use static import NodeChildrenContainer

ce19b7c

improve traverser: add getParentNodes and isRootContext to

3833d72

TraverserState

improve Traverser by allowing the node to be changed

0e8b4f6

AstMultiZipper fix for changing the structure in multiple nodes more AstTransformer tests

handle unchanged nodes correctly

1f29524

Added AstSorter to test zipper code

84fdbc1

Added better testing

e801153

More code for SDL - not yet tested

007567d

more ast transformer test

7215491

Added SDL sorting with tests

85da93c

JavaDoc on AST Sorter

2ae091b

Merge remote-tracking branch 'upstream/zipper2' into zipper2

0d35906

Now added signature work - not finished but close

cca5172

Removed copyright

a0463cd

andimarek merged commit 44627e4 into master Dec 15, 2018

andimarek deleted the zipper2 branch January 14, 2019 04:14

Make all Nodes immutable but still easy to "modify" #1335

Make all Nodes immutable but still easy to "modify" #1335

Uh oh!

Conversation

andimarek commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andimarek commented Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andimarek commented Dec 2, 2018

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!

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!

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!

andimarek commented Dec 1, 2018 •

edited

Loading

andimarek commented Dec 2, 2018 •

edited

Loading