Add visitor for types and use generic traversal by exbe · Pull Request #1084 · graphql-java/graphql-java

exbe · 2018-06-15T01:05:25Z

? extends GraphQLType can be traversed using TypeTraverser
SchemaUtil implementation uses visitor/traverser to fulfill its contract

bbakerman · 2018-06-16T11:40:24Z

src/main/java/graphql/schema/GraphQLType.java

JavaDoc on key types would be needed

bbakerman · 2018-06-16T11:46:57Z

src/main/java/graphql/schema/SchemaUtil.java

I find this one a real stretch in terms of saying this is "better"...There are 2 conditions -scalar and enum and there is some unwrapping to be done.

If I look at this code is an indirection that is low value.

That said the others have more value

Unwrapping is using recursion, which is based on a fixed stack.

Large and deep graphs have a chance to overflow stack. VERY large graphql type system will overflow it for sure.

Recursion is also application-dependent at runtime. If this lib will be called after web server, IoC framework, other application layers .... this all eats up stack leaving less and less room. Not to mention hypothetical cycles in graphql type system...

Another hypothetical case is there application developer would run graphql in a thread with custom (and small) execution stack.

Traverser is free from all above issues. It relies on a true stack, which would grow in size if needed and tracks visited types as it goes.

In theory - perhaps

but in practice, given a graphql type it can only be 1 of 6 different types (enum, scalar, object, input, interface and union) or the wrappers like (list, null, type reference)

So in order to blow the stack given a single graphql type, then you would need megabytes of wrapping

eg [![![![![![![![![![![![![![SomeTypeHere]]]]]]]]]]]

Given a simple graphql type there is no infinite tree.

Now if you include graphql.schema.GraphQLFieldDefinition (which does not derive from GraphqlType) then yes you can have a large tree.

My point is that not ALL uses case of recursion are problematic. And in this case where we have a single GraphqlType, not at all.

My point is that not ALL uses case of recursion are problematic.

Agree on 100%. Do you think that traverser gives too much of overhead?

This case might be trivial, but it is still type traversal. Traverser looks good enough to cover this case too. Would you prefer to have custom algorithm here instead?

bbakerman · 2018-06-16T11:49:52Z

src/main/java/graphql/schema/TypeTraverser.java

Java Doc and @internal of @publicapi would be needed

TypeTraverser is similar to NodeTraverser. Shall I update both?

I consider NodeTraverser/TypeTraverser as @internal for now.

yes, please make it @Internal for now.

bbakerman · 2018-06-18T07:33:35Z

The build is failing with

A problem occurred evaluating root project 'graphql-java'.
git hash could not be determined

I am not sure what they means however. Can you re-pull master onto this branch so as to perhaps fix this.

I did this on other branches and it seem to work. Like I said I am not sure what this is??

bbakerman · 2018-06-18T07:37:49Z

src/main/java/graphql/schema/SchemaUtil.java

There is enough visitor code here that it deserves its own file.

Perhaps not one class per visitor so I would create a package protected holder of these classes called say "TypeVisitors" that contained these classes

bbakerman · 2018-06-18T07:38:18Z

src/main/java/graphql/schema/SchemaUtil.java

please put these at the top of the file. Bit more idiomatic

bbakerman · 2018-06-18T07:39:30Z

src/main/java/graphql/schema/TypeVisitorStub.java

Simple JavaDoc please.

This probably deserves to be @publicapi eventually (as a help to write a visitor)

Also naming as mentioned above GraphqlTypeVisitorStub

bbakerman · 2018-06-18T07:41:02Z

src/main/java/graphql/schema/TypeVisitor.java

Naming is hard but I would call this (and the others) GraphqlTypeVisitor

My reasoning is that we have runtime types and AST types and its already confusing

So we have GraphqlType / GraphqlObjectType and so on to help with that

Hence I visitor of GraphqlType thigns should have the GraphqlTypeVisitor naming. Same with the stubb and so on

bbakerman

I would like to see more unit tests of the specific traversal on all types.

I know existing tests around SchemaUtil probably test this by accident

But I think we need a test that demonstrates that given all the GraphqlType derived things, that a visitor will in fact see them all.

exbe · 2018-06-18T20:41:01Z

@bbakerman Build failure is coming from gradle function (here), but it seems execution of the git command itself was ok. I think it has something to do with gradle thread... not sure.

I will try to re-base/re-sync with latest master and see what happens.

From travis log.

current git hash:
git rev-parse --short HEAD
df45cb3

Thank you for review, I will follow up with comments.

exbe · 2018-06-18T22:14:47Z

@bbakerman I feel confused with usage of @PublicAPI/@internal. Is there guide anywhere besides javadoc? Basically I would love to wait few release before switching it to public. Probably we could od same thing for NodeVisitorStub/NodeVisitor - they are @internal, but I believe they should be @public as well.

Let me ping you about test cases on gitter. I might need to clarify some use cases.

exbe · 2018-06-18T22:15:50Z

PS close-reopen PR helps to "fix" build failure on travis CI

andimarek · 2018-06-19T01:45:31Z

@exbe yes: the current build (getting the git hash part) is sometimes flaky.

andimarek · 2018-06-19T01:56:20Z

src/main/java/graphql/schema/GraphQLArgument.java

why is this deprecated now?

@andimarek This method was part of older recursive reference resolution algorithm.
This PR uses getChildren() to unwrap actual type and replaceType(GraphQLInputType type) to set it.

I am not sure if anyone else is using it (e.g. clients), so I have marked it as @deprecated.
Ideally, if it is not part of some use case, this can be removed in future versions.

If I miss some use case, I would be happy to learn about it and add support.

please remove them: they are not used anywhere else and were only for one purpose

exbe · 2018-06-20T01:03:33Z

@bbakerman @andimarek I think I responded to all comments.
Please take a look

andimarek

Thanks for this PR.
In general I think it is a real improvement compared to the manual traversing. 👍

Please see my other comments for what to change.

Also: Please use the graphql-java formatter https://github.com/graphql-java/graphql-java/blob/master/graphql-java-code-style.xml and format your code with that.

Thanks

andimarek · 2018-06-23T06:49:34Z

src/main/java/graphql/schema/GraphqlTypeVisitor.java

Class name: 'GraphQLTypeVisitor' not 'GrapqhlTypeVisitor'

andimarek · 2018-06-23T06:51:21Z

src/main/java/graphql/schema/GraphqlTypeVisitor.java

why the difference here between Interfaces and concrete types?

For convince, which is based on assumption that in most cases developers won't need to visit marker interface very often (even stub doesn't care).

Not a big deal - I can move default implementation under GraphQLTypeVisitorStub.

What is your preference @bbakerman and @andimarek?

andimarek · 2018-06-23T06:51:48Z

src/main/java/graphql/schema/GraphqlTypeVisitorStub.java

naming: GraphQL...

unit testing is missing: have a look at NodeVisitorStubTest for inspiration.

It is getting tested via TypeTraverserTest :) Is it good enough?

andimarek · 2018-06-23T06:56:14Z

src/main/java/graphql/schema/GraphqlTypeVisitors.java

naming: GraphQL...

please make every class public and extract in a extra file. Don't make them package protected, but mark them as @Internal

andimarek · 2018-06-23T07:03:52Z

src/main/java/graphql/schema/SchemaUtil.java

please remove it

andimarek · 2018-06-23T07:04:55Z

src/main/java/graphql/schema/TypeTraverser.java

yes, please make it @Internal for now.

andimarek · 2018-06-23T07:11:59Z

src/main/java/graphql/schema/TypeTraverser.java

why protected? I would say make it private. We don't really design for inheritance: if there is something that should be customizable it should be done in another way.

andimarek · 2018-06-23T07:12:13Z

src/main/java/graphql/schema/TypeTraverser.java

private .. see above

andimarek · 2018-06-23T07:12:38Z

src/main/java/graphql/schema/TypeTraverser.java

private static please if we keep it, but more important: simplify it please by only caring about enter, because we only support depthFirst atm.

andimarek · 2018-06-23T07:31:55Z

src/main/java/graphql/schema/SchemaUtil.java

I agree with @bbakerman: the original code for isLeafType and isInputType is so much simpler and I don't really see a non theoretical argument to replace that. So please revert it to the original code and delete the LeafVisitor.

andimarek · 2018-06-23T09:36:02Z

We will probably make the NodeTraverser public, this includes TraverserContext. See #1097. So I don't see a problem to make this visitor also public. @exbe

exbe · 2018-07-16T21:47:59Z

@bbakerman @andimarek do you see anything else, which I should change in a scope of this PR?
Please let me know.

exbe · 2018-08-26T08:49:59Z

@bbakerman @andimarek any updates on this one?

andimarek · 2018-09-03T00:06:51Z

I am really sorry for the delay @exbe. Will try to review it soon.

exbe · 2018-09-03T05:15:54Z

@andimarek no worries, take your time :)

andimarek

Looks good, see my comments for some final thoughts.

andimarek · 2018-09-03T05:37:27Z

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

+import graphql.util.TraversalControl;
+import graphql.util.TraverserContext;
+
+@Internal


This should be public: we recently made the general Traverser also public, so nothing prevents us from making this public too

andimarek · 2018-09-03T05:38:41Z

src/main/java/graphql/schema/GraphQLTypeResolvingVisitor.java

+    public TraversalControl visitGraphQLTypeReference(GraphQLTypeReference node, TraverserContext<GraphQLType> context) {
+
+        final GraphQLType resolvedType = typeMap.get(node.getName());
+        Assert.assertTrue(resolvedType != null, "type %s not found in schema", node.getName());


just as a hint: we also have assertNoNull

andimarek · 2018-09-03T05:48:30Z

src/main/java/graphql/schema/GraphQLTypeRefResolvingVisitor.java

+import graphql.util.TraverserContext;
+
+@Internal
+public class GraphQLTypeRefResolvingVisitor extends GraphQLTypeVisitorStub {


what do you think about putting this class into the other one GraphQLTypeResolvingVisitor? it is a very hard dependency and it would make it more readable imho

…lver - type visitor promoted to public api - better code style for assertion

exbe · 2018-09-05T03:42:48Z

@andimarek please check

andimarek · 2018-09-09T01:48:28Z

@exbe thanks for the patience ... looks good

bbakerman reviewed Jun 16, 2018

View reviewed changes

src/main/java/graphql/schema/GraphQLType.java Outdated

Copy link

Member

bbakerman Jun 16, 2018

Choose a reason for hiding this comment

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

JavaDoc on key types would be needed

bbakerman reviewed Jun 16, 2018

View reviewed changes

bbakerman reviewed Jun 18, 2018

View reviewed changes

src/main/java/graphql/schema/SchemaUtil.java Outdated

Copy link

Member

bbakerman Jun 18, 2018

Choose a reason for hiding this comment

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

please put these at the top of the file. Bit more idiomatic

bbakerman reviewed Jun 18, 2018

View reviewed changes

bbakerman requested changes Jun 18, 2018

View reviewed changes

exbe force-pushed the typesvisitor branch from 7832e87 to 9845980 Compare June 18, 2018 20:57

exbe closed this Jun 18, 2018

exbe reopened this Jun 18, 2018

andimarek reviewed Jun 19, 2018

View reviewed changes

andimarek requested changes Jun 23, 2018

View reviewed changes

exbe force-pushed the typesvisitor branch from cd429e7 to 857f03c Compare June 25, 2018 21:50

amatiushkin added 8 commits July 27, 2018 11:02

Add visitor for types and use generic traversal

4d9b642

Add javadoc

9c96a2c

Code review comments

7cfc8a7

fix few bugs and add tests

847ab4b

Review comments

b864afb

Clear deprecated methods

0afdea2

Remove unsused statement

925e343

Extract visitors to stand-alone classes

f989e92

amatiushkin added 3 commits July 27, 2018 11:02

Use project codestyle

4f6488e

Make TypeTraverser public with respect to graphql-java#1097

126f3df

Add a test for GraphQLTypeVisitorStub

58925da

exbe force-pushed the typesvisitor branch from b68b955 to 58925da Compare July 27, 2018 18:04

andimarek requested changes Sep 3, 2018

View reviewed changes

- reference resolving visitor is inner type of more general type reso…

1252851

…lver - type visitor promoted to public api - better code style for assertion

fix assertNotNull argument

a4850c0

andimarek added this to the 11.0 milestone Sep 9, 2018

andimarek merged commit 4c5aca5 into graphql-java:master Sep 9, 2018

kaqqao mentioned this pull request Sep 11, 2018

Reusing the same TypeReference instance breaks reference resolution in the current master #1216

Closed

Conversation

exbe commented Jun 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exbe Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exbe Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bbakerman commented Jun 18, 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!

bbakerman left a comment

Choose a reason for hiding this comment

Uh oh!

exbe commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

exbe commented Jun 18, 2018

Uh oh!

exbe commented Jun 18, 2018

Uh oh!

andimarek commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exbe Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exbe commented Jun 20, 2018

Uh oh!

andimarek left a comment

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!

exbe Jun 16, 2018 •

edited

Loading

exbe Jun 19, 2018 •

edited

Loading

exbe commented Jun 18, 2018 •

edited

Loading

exbe Jun 19, 2018 •

edited

Loading