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

Add Vary + s-maxage to CacheControl directive for efficient CDN caching #6047

Merged
merged 11 commits into from
Sep 26, 2024

Conversation

rickardp
Copy link
Contributor

As discussed with @tobias-tengler this is what I would want from the caching.

I will add some comments with questions on how to best solve some problems

@michaelstaib michaelstaib added this to the HC-13.1.0 milestone Apr 12, 2023
@rickardp rickardp marked this pull request as ready for review April 12, 2023 21:12
@rickardp rickardp changed the title Draft idea of how Vary + s-maxage would work Add Vary + s-maxage to CacheControl directive for efficient CDN caching Apr 12, 2023
@michaelstaib michaelstaib modified the milestones: HC-13.1.0, HC-13.0.x Jun 2, 2023
@rickardp rickardp changed the base branch from main-version-13 to main September 8, 2023 11:54
@rickardp
Copy link
Contributor Author

rickardp commented Sep 8, 2023

@tobias-tengler I rebased this PR on main, as I think the old target branch got stale? Have you had the chance to have a look at this PR? Thanks!

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

HotChocolate.AspNetCore.Subscriptions.GraphQLOverWebSocket.WebSocketProtocolTests.Send_Pong_With_Payload [FAIL]

@sonarcloud
Copy link

sonarcloud bot commented Sep 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Patch coverage: 82.07% and project coverage change: +0.32% 🎉

Comparison is base (41c82e1) 79.00% compared to head (0d8499d) 79.33%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6047      +/-   ##
==========================================
+ Coverage   79.00%   79.33%   +0.32%     
==========================================
  Files        2915     2915              
  Lines      140092   140092              
==========================================
+ Hits       110681   111138     +457     
+ Misses      29411    28954     -457     
Flag Coverage Δ
unittests 79.33% <82.07%> (+0.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...olate/Caching/src/Caching/CacheControlAttribute.cs 86.95% <50.00%> (ø)
...ons/CacheControlObjectFieldDescriptorExtensions.cs 71.42% <60.00%> (ø)
...s/CacheControlInterfaceTypeDescriptorExtensions.cs 35.71% <66.66%> (ø)
...ions/CacheControlObjectTypeDescriptorExtensions.cs 35.71% <66.66%> (ø)
...sions/CacheControlUnionTypeDescriptorExtensions.cs 71.42% <66.66%> (ø)
...ng/src/Caching/CacheControlConstraintsOptimizer.cs 80.00% <77.41%> (ø)
...Core/Serialization/DefaultHttpResponseFormatter.cs 73.80% <83.33%> (-20.24%) ⬇️
...c/Caching/CacheControlValidationTypeInterceptor.cs 100.00% <100.00%> (ø)
...rc/HotChocolate/Caching/src/Caching/ErrorHelper.cs 100.00% <100.00%> (ø)
...ching/Properties/CacheControlResources.Designer.cs 75.43% <100.00%> (ø)
... and 3 more

... and 201 files with indirect coverage changes

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

@michaelstaib michaelstaib force-pushed the main branch 2 times, most recently from 8a97a79 to e2d2300 Compare June 17, 2024 17:49
@rickardp
Copy link
Contributor Author

rickardp commented Sep 5, 2024

@tobias-tengler is there still interest in this? I could rebase/merge master but we since implemented this in our application side. Happy to help move this to HC if there is value in it though.

I suppose #7054 needs to be fixed first anyhow. That one is BTW blocking us from upgrading past HC 13.8.

@michaelstaib
Copy link
Member

@rickardp I will check with the team this week what the status is on this ... sorry for the delay here.

@rickardp
Copy link
Contributor Author

rickardp commented Sep 18, 2024

Thank you for fixing #7054! Happy to rebase this if there is interest in it

@michaelstaib
Copy link
Member

Can you rebase it ... V14 is essentially done and we want to take this in for 15 .... so if you rebase it to the current main we can start integrating this. Sorry that this took so long on our side but with shifting priorities sometimes you loose the focus on a PR.

@michaelstaib michaelstaib marked this pull request as draft September 23, 2024 08:30
@michaelstaib
Copy link
Member

I changed this to a draft for now.

@rickardp rickardp force-pushed the cache-policy branch 2 times, most recently from 372958f to a7d8a79 Compare September 24, 2024 20:43
@rickardp
Copy link
Contributor Author

@michaelstaib Updated this PR and added a few more clarifying unit tests

@michaelstaib
Copy link
Member

This looks good ... I will go over your comments ones more and verify things within the code and then its good to go. Sorry again for the long wait and thank you for your contribution.

@michaelstaib michaelstaib self-assigned this Sep 25, 2024
@michaelstaib michaelstaib modified the milestones: HC-14.x.x, HC-15.0.0 Sep 25, 2024
@michaelstaib michaelstaib marked this pull request as ready for review September 25, 2024 21:12
@michaelstaib michaelstaib merged commit 5c183e0 into ChilliCream:main Sep 26, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants