Skip to content

Add DataLoaderRegistry to GraphQLContext #83

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jul 25, 2018
Merged

Add DataLoaderRegistry to GraphQLContext #83

merged 7 commits into from
Jul 25, 2018

Conversation

ashu-walmart
Copy link
Contributor

@ashu-walmart ashu-walmart commented Jun 15, 2018 •
edited
Loading

And return instrumentation in SimpleGraphQLServlet accordingly.

@ashu-walmart ashu-walmart mentioned this pull request Jun 18, 2018
Closed
@ashu-walmart ashu-walmart changed the title Add DataLoaderRegistry to GraphQLContext and return Instrumentation a… Add DataLoaderRegistry to GraphQLContext Jun 20, 2018
@ashu-walmart
Copy link
Contributor Author

@apottere, Is is possible to get this PR reviewed. I have tested it locally in my application and seems to be working as expected.

@ashu-walmart
Copy link
Contributor Author

@apottere @andimarek @bbakerman, Is it possible to get this reviewed and merged? The use of data-loaders in spring boot applications vastly simpler with this fix.

@ryangardner
Copy link
Contributor

ryangardner commented Jul 9, 2018 •
edited
Loading

To make it possible to use the data-loaders along other instrumentation (like the TracingInstrumentation or FieldValidationInstrumentation) should this be changed to create a ChainedInstrumentation if there is both Instrumentation provided and a DataLoaderRegistry?

It looks like right now it will only use the DataLoaderDispatcherInstrumentation or whatever instrumentation is passed in?

@ashu-walmart
Copy link
Contributor Author

ashu-walmart commented Jul 9, 2018 •
edited
Loading

I am not aware of ChainedInstrumentation and can look into that. Another option may be to use an Instrumentation provider interface that can be implemented by the calling application to get greater control over the instrumentation. We can move this logic inside a DefaultInstrumentationProvider implementation

@ashu-walmart ashu-walmart deleted the DataLoader-in-context branch July 16, 2018 16:11
@ashu-walmart ashu-walmart reopened this Jul 16, 2018
@ashu-walmart ashu-walmart force-pushed the DataLoader-in-context branch from bdcbaa6 to 065abc6 Compare July 16, 2018 20:07
@ashu-walmart
Copy link
Contributor Author

@ryangardner Addressed your comment. Please review.

@ashu-walmart
Copy link
Contributor Author

ashu-walmart commented Jul 19, 2018 •
edited
Loading

This fix is actually dependent on graphql-java/graphql-java#1090

@ashu-walmart ashu-walmart force-pushed the DataLoader-in-context branch from 065abc6 to 7e7eb4a Compare July 19, 2018 15:55
oliemansm
oliemansm approved these changes Jul 20, 2018
View reviewed changes
Copy link
Member

@oliemansm oliemansm left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! @ryangardner This solution ok for you as well?

@oliemansm
Copy link
Member

@apottere Does this PR look good to you as well? If so I suggest we merge this one in right away as well and include it in the next release.

@oliemansm
Copy link
Member

oliemansm commented Jul 24, 2018 •
edited
Loading

@ashu-walmart Due to the breaking changes in the 6.0.0 release this branch needed to be reworked to become compatible. I've merged it into your branch and made those changes, can I push it so you can check those changes in this PR as well?

oliemansm added 2 commits July 24, 2018 21:51
@oliemansm
Merge branch 'master' into DataLoader-in-context
db4fcb5 
# Conflicts:
#	src/main/java/graphql/servlet/GraphQLContext.java
#	src/main/java/graphql/servlet/GraphQLServlet.java
#	src/main/java/graphql/servlet/OsgiGraphQLHttpServlet.java
#	src/main/java/graphql/servlet/SimpleGraphQLServlet.java
#	src/test/groovy/graphql/servlet/AbstractGraphQLHttpServletSpec.groovy
@oliemansm
Reworked the DataLoader instrumentation to become compatible with 6.0.0
5304bb6
@ashu-walmart
Copy link
Contributor Author

@oliemansm Yes Please do.

@oliemansm
Copy link
Member

@ashu-walmart Done, please review. I'll merge it afterwards.

@ashu-walmart
Copy link
Contributor Author

@oliemansm, Looks good to me.

@oliemansm oliemansm merged commit c8bb3ac into graphql-java-kickstart:master Jul 25, 2018
@oliemansm
Copy link
Member

@ashu-walmart Thanks, merged it! Will check when we can do a new minor release to push this.

@oliemansm oliemansm mentioned this pull request Oct 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliemansm oliemansm oliemansm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ashu-walmart @ryangardner @oliemansm