Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graphql-java-support: Bump graphql-java to v14 #51

Merged
merged 15 commits into from
Mar 27, 2020

Conversation

sachindshinde
Copy link
Contributor

@sachindshinde sachindshinde commented Mar 16, 2020

This PR is an iteration of the changes suggested in #41 and #46 (many thanks to the authors and commenters).

Summary of changes:

Note this is a backwards-incompatible change.

Also note that FederationSdlPrinter.Options.includeDirectives() will filter all directive definitions and usages indicated with the exception of directive usages of @deprecated, which is conveniently the behavior we want. We've added directive definition filtering in #53 , so we don't need to rely on FederationSdlPrinter.Options.includeDirectives() to filter the standard directive definitions.

It'll be easier in this PR to go commit-by-commit. For FederationSdlPrinter, it'll help to compare FederationSdlPrinter.java in the class-copy-and-rename commit to the v14 SchemaPrinter, located at https://raw.githubusercontent.com/graphql-java/graphql-java/v14.0/src/main/java/graphql/schema/idl/SchemaPrinter.java . For the FederationSdlPrinter changes that fix the issues in #52 and #53 , it'll help to compare against the respective changes in those PRs.

@mcohen75
Copy link
Contributor

mcohen75 commented Mar 17, 2020

Also note that SchemaPrinter.Options.includeDirectives() will filter all directive definitions and usages indicated with the exception of directive usages of @deprecated, which is conveniently the behavior we want.

Is it worth adding a test to ensure that the @skip and @include directives are filtered out? This would catch problems here in the future if this behavior changes in graphql-java. It would also serve as convenient documentation when we all forget about this PR.

@sachindshinde
Copy link
Contributor Author

@mcohen75
@include and @skip can only be used on executable locations (as per spec), and not type system locations. The only way we could test is by including executable definitions, but SchemaParser.parse() will naturally throw if it sees those.

@sachindshinde sachindshinde changed the base branch from master to sachin/remove-empty-query-type-from-sdl March 18, 2020 20:05
Copy link

@zionts zionts left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed PR description! I think we should try to use this ourselves in our own federated service before merging this, but I'll take a look at the FederationSdlPrinter changes more thoroughly now. Everything else looks 👍

@sachindshinde sachindshinde force-pushed the sachin/graphql-java-v14 branch 2 times, most recently from 315f17a to a910096 Compare March 19, 2020 01:43
@sachindshinde sachindshinde changed the base branch from sachin/remove-empty-query-type-from-sdl to master March 19, 2020 01:43
@sachindshinde sachindshinde force-pushed the sachin/graphql-java-v14 branch 3 times, most recently from 95c77af to 4e80998 Compare March 26, 2020 02:36
@sachindshinde sachindshinde changed the base branch from master to sachin/better-schema-processing March 26, 2020 02:37
@sachindshinde
Copy link
Contributor Author

So after some testing in our own infra, I've opened up #53 to address some further issues, and rebased this branch on top of that.

The main consequences for this PR are that we can just use the directive definition filtering from #53 to hide standard directive definitions.

@sachindshinde sachindshinde changed the base branch from sachin/better-schema-processing to master March 27, 2020 01:41
@sachindshinde sachindshinde merged commit da23efa into master Mar 27, 2020
@sachindshinde sachindshinde deleted the sachin/graphql-java-v14 branch March 27, 2020 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants