improve query traversal and make it public#1031
Conversation
c9b2c8e to
ac379fb
Compare
add tests for QueryTraversal
| /** | ||
| * @return a deep copy of this selection | ||
| */ | ||
| T deepCopy(); |
There was a problem hiding this comment.
because it is declared in the super interface already ... just a clean up :-)
|
Are you really going to put this in 9.0 Its marked as such |
| QueryVisitorEnvironment thisNodeAsParent = new QueryVisitorEnvironment(env.getField(), env.getFieldDefinition(), env.getParentType(), env.getParentEnvironment(), env.getArguments()); | ||
| if (valuesByParent.containsKey(thisNodeAsParent)) { | ||
| childsComplexity = valuesByParent.get(thisNodeAsParent).stream().mapToInt(Integer::intValue).sum(); | ||
| Map<QueryVisitorFieldEnvironment, List<Integer>> valuesByParent = new LinkedHashMap<>(); |
There was a problem hiding this comment.
Maybe put a builder on QueryVisitorFieldEnvironment. It will allow it to evolve more easily over time.
There was a problem hiding this comment.
I extracted an Interface instead.
|
|
||
| T reduceField(QueryVisitorEnvironment reducerEnvironment, T acc); | ||
| T reduceField(QueryVisitorFieldEnvironment reducerEnvironment, T acc); | ||
| } |
| private final ChildrenOfSelectionProvider childrenOfSelectionProvider; | ||
| private final GraphQLObjectType rootParentType; | ||
|
|
||
| public QueryTraversal(GraphQLSchema schema, |
There was a problem hiding this comment.
Put a Builder on this and make the constructor private. This allows for better API evolution over time
There was a problem hiding this comment.
mh ... I get your point but the arguments are mutually exclusive: for example operation is only valid with a Document, same with root and rooParentType etc.
Is it worth extracting a whole QueryTraversalBuilder? Other options?
There was a problem hiding this comment.
I think generally the graphql-java should use builders on PublicAPI elements because without it the constructor becomes public API and over loading is your only way to construct things
In this case I would do QueryTraversal.newQueryTraversal() and have an internal builder of them
|
|
||
| import graphql.PublicApi; | ||
|
|
||
| @PublicApi |
There was a problem hiding this comment.
Should you talk about order here?? pre order etc...??
| return "QueryVisitorFieldEnvironment{" + | ||
| "field=" + field + | ||
| ", fieldDefinition=" + fieldDefinition + | ||
| ", parentType=" + parentType + |
| return Objects.hash(fragmentSpread); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
extracted an Interface
| import graphql.PublicApi; | ||
|
|
||
| @PublicApi | ||
| public class QueryVisitorStub implements QueryVisitor { |
There was a problem hiding this comment.
Another way to do this in Java 8 is just use default methods on the interface.
I am still 50/50 on whether this is a good idea.
I do like your naming here. Better than NoopFoo
| import graphql.TestUtil | ||
| import graphql.language.Document | ||
| import graphql.language.* | ||
| import graphql.parser.Parser |
bbakerman
left a comment
There was a problem hiding this comment.
I think we should add Builders to the places where a consumer is expected to create an object
| this.rootParentType = getRootTypeFromOperation(getOperationResult.operationDefinition); | ||
| this.childrenOfSelectionProvider = new ChildrenOfSelectionProvider(fragmentsByName); | ||
| } | ||
|
|
There was a problem hiding this comment.
Without a builder the constructor is public SPI
| // {foo} | ||
| // """) | ||
| // def root = new Field() | ||
|
|
There was a problem hiding this comment.
Did you want this commented out??? I would remove it
This makes QueryTraversal public (it was internal before)
and improves it by having an arbitrary Node as root (not only the DocumentDefintion) and adds the
SelectionSetContainerto the environment.