dgs-framework icon indicating copy to clipboard operation
dgs-framework copied to clipboard

Support new @cacheControl directive by Apollo, with Cache-Control header

Open jord1e opened this issue 2 years ago • 14 comments

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

jord1e avatar Apr 09 '22 15:04 jord1e

Thanks for the PR @jord1e , I'm rebasing to the latest changes and will review. Need some time thought to go through the changes.

berngp avatar Apr 12 '22 00:04 berngp

Do we have an ETA on when this PR can be reviewed/merged?

sijonelis avatar Jun 15 '22 09:06 sijonelis

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 avatar Jun 16 '22 00:06 berngp

@berngp thanks for a quick answer, and sorry for being annoying with such pings!

sijonelis avatar Jun 22 '22 12:06 sijonelis

hi @berngp any update on ETA for this by any chance? we're really keen to start using this!

cmboult avatar Jul 28 '22 16:07 cmboult

I vote for it too

marquies avatar Jul 28 '22 18:07 marquies

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

jord1e avatar Jul 28 '22 22:07 jord1e

Weird javadoc error, tests do pass

jord1e avatar Jul 28 '22 22:07 jord1e

Hi Team.. we have recently started using DGS for our subgraph.. do we have an ETA for this to be reviewed and merged ?

heerak80 avatar Nov 17 '22 18:11 heerak80

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 avatar Nov 17 '22 18:11 srinivasankavitha

@srinivasankavitha -Thanks for the prompt response. Look forward to this being supported in future.

heerak80 avatar Nov 17 '22 21:11 heerak80

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.

SpeedyGonzaless avatar Dec 20 '22 13:12 SpeedyGonzaless

@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

setchy avatar Apr 22 '23 10:04 setchy

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.

srinivasankavitha avatar Apr 24 '23 17:04 srinivasankavitha

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

github-actions[bot] avatar Apr 27 '24 00:04 github-actions[bot]

Can we reopen this issue? The need is still present

cicoub13 avatar May 05 '24 06:05 cicoub13

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 avatar May 06 '24 16:05 srinivasankavitha

@srinivasankavitha is this feature is present in the spring-graphql integration for the DGS framework ?

yamalameyooooo avatar May 13 '24 08:05 yamalameyooooo

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.

srinivasankavitha avatar May 13 '24 17:05 srinivasankavitha