Skip to content

Schema transformer fix#2078

Merged
bbakerman merged 12 commits into
masterfrom
schema-transformer-top-sort
Oct 25, 2020
Merged

Schema transformer fix#2078
bbakerman merged 12 commits into
masterfrom
schema-transformer-top-sort

Conversation

@andimarek

@andimarek andimarek commented Oct 19, 2020

Copy link
Copy Markdown
Member

This changes the Schema transformer implementation in order to overcome some limitations/fixes some bugs:

The main change is that the order of elements zipped up (moved up) is not depended on the depth (distance from dummy root), but the elements are sorted topologically (based on the reverse dependency of the normal graph).

It also changes the implementation of GraphQLList and GraphQLNonNull to not implement equals/hashcode because when we treat the GraphQLSchema as graph we rely fundamentally on the fact that every Node is unique and Nodes are equal only when it is the same instance.

@andimarek andimarek added the breaking change requires a new major version to be relased label Oct 19, 2020
@bbakerman

Copy link
Copy Markdown
Member

As discussed - let put an explicit .equals and hashCode in each SchemaElement such that they always called Object.equals

We can put doco on the SchemaElement interface in why

@bbakerman bbakerman self-requested a review October 20, 2020 02:17
@andimarek

andimarek commented Oct 20, 2020

Copy link
Copy Markdown
Member Author

@bbakerman I added a javadoc on the GraphQLSchemaElement so far.

*
* @return
*/
boolean equals(Object obj);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should also have hashCode() - HashSet relies on it not just equals

Set<GraphQLSchemaElement> notPermMarked = new LinkedHashSet<>(allNodes);
Set<GraphQLSchemaElement> tempMarked = new LinkedHashSet<>();
Set<GraphQLSchemaElement> permMarked = new LinkedHashSet<>();
/**

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We really have entered DAG academia havent we. Glad we have you!

permMarked.add(n);
notPermMarked.remove(n);
result.add(n);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This feels like it could be a generic utility. A DagUtil kinda class

@bbakerman bbakerman added this to the 16.0 milestone Oct 25, 2020
@bbakerman bbakerman merged commit a39aec8 into master Oct 25, 2020
@andimarek andimarek deleted the schema-transformer-top-sort branch May 4, 2021 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change requires a new major version to be relased

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants