dgs-framework
dgs-framework copied to clipboard
Support new @cacheControl directive by Apollo, with Cache-Control header
Fixes Netflix/dgs-framework#929
This was a pain in the back to be honest 😐. We needed the GraphQLContext object in the transport layers' class. This is mainly why the refactorings are so big.
Pull request checklist
- [X] Please read our contributor guide
- [X] Consider creating a discussion on the discussion forum first
- [X] Make sure the PR doesn't introduce backward compatibility issues
- [X] Make sure to have sufficient test cases
Pull Request type
- [ ] Bugfix
- [X] Feature
- [X] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] Other (please describe):
Changes in this PR
Fixes Netflix/dgs-framework#929
This has recently been implemented by federation-jvm (see: apollographql/federation-jvm#133)
A CacheControlInstrumentation
instrumentation class must be injected, which adds the appropriate Cache-Control value into the GraphQLContext
I think the only breaking change is when people mock the execute
method in DgsReactiveQueryExecutor
or DgsQueryExecutor
.
Alternatives considered
N/A, the header value must be set by the framework derived from the internal GraphQLContext
Thanks for the PR @jord1e , I'm rebasing to the latest changes and will review. Need some time thought to go through the changes.
Do we have an ETA on when this PR can be reviewed/merged?
Hi @vilkazz , haven't been able to prioritize this review, since the feature is not used internally. I can't give you an ETA yet but will have it in mind as we plan work on DGS.
@berngp thanks for a quick answer, and sorry for being annoying with such pings!
hi @berngp any update on ETA for this by any chance? we're really keen to start using this!
I vote for it too
That took me a little bit less then 3 hours because I used the wrong media type after the media type change some time ago (... and I merged into master instead of from master the first time 👎, this field is hard).
Ready for review again, all tests still passing
P.s. @berngp when are the hoodies coming 😁
Will also look into updating #923 , but thats a bit lower priority
Weird javadoc error, tests do pass
Hi Team.. we have recently started using DGS for our subgraph.. do we have an ETA for this to be reviewed and merged ?
This has not been prioritized yet since we do not use this internally and the scope of files changed is large. Unfortunately, we don't have an ETA yet. If this changes, we will post an update.
@srinivasankavitha -Thanks for the prompt response. Look forward to this being supported in future.
Hi guys, I recently started to migrate to graphql using DGS and having this feature will be great. I hope it will be merged some time.
@srinivasankavitha is there interest in reviving this PR?
This enhancement is quite important to us and would love for it to be in the framework
Thanks for all the work @jord1e and for all the useful discussions around this feature. Unfortunately, we are not able to prioritize this effort at the moment due to other internal priorities. We will add this to our backlog of future work, and will post an update when we are able to look into this, hopefully in the near future.
This pull request has been marked as stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 7 days
Can we reopen this issue? The need is still present
Hi - we now have spring-graphql integration for the DGS framework. That would be a better project to request this feature since we can integrate that better into the framework.
@srinivasankavitha is this feature is present in the spring-graphql integration for the DGS framework ?
No, I don't believe it is available there either. Feel free to open a discussion thread in the spring-graphql project as well so we can determine the best path forward for this feature. Now that we have the DGS framework integrated with spring-graphql, we should be able to easily pick up any new features available in spring-graphql.