Support new @cacheControl directive by Apollo, with Cache-Control header #968
Conversation
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 Fixes Netflix#929
berngp
force-pushed
the
new-cache-control
branch
from
9e2cde1
to
1b7be36
Compare
|
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 |
# Conflicts: # graphql-dgs-reactive/src/main/java/com/netflix/graphql/dgs/reactive/DgsReactiveQueryExecutor.java # graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/autoconfiguration/DgsWebFluxAutoConfiguration.kt # graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/handlers/DefaultDgsWebfluxHttpHandler.kt # graphql-dgs-spring-webmvc-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webmvc/autoconfigure/DgsWebMvcAutoConfiguration.kt # graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt # graphql-dgs-spring-webmvc/src/test/kotlin/com/netflix/graphql/dgs/mvc/DgsMultipartPostControllerTest.kt # graphql-dgs/src/main/java/com/netflix/graphql/dgs/DgsQueryExecutor.java # graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt
|
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. |
Fixes #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
first
Pull Request type
Changes in this PR
Fixes #929
This has recently been implemented by federation-jvm (see: apollographql/federation-jvm#133)
A
CacheControlInstrumentationinstrumentation class must be injected, which adds the appropriate Cache-Control value into the GraphQLContextI think the only breaking change is when people mock the
executemethod inDgsReactiveQueryExecutororDgsQueryExecutor.Alternatives considered
N/A, the header value must be set by the framework derived from the internal GraphQLContext