Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

SO-4992: ETag and Cache-Control header support #1270

Merged
merged 18 commits into from
Apr 2, 2024

Conversation

cmark
Copy link
Member

@cmark cmark commented Mar 27, 2024

This PR adds support for ETag and Cache-Control headers so that HTTP clients, proxies, and intermediaries can cache the responses earlier than caching only in an HTTP browser.

Still somewhat blocked by spring-projects/spring-security#12865. We need to figure out a way of preventing Spring from adding the default cache-control headers when we set our own.

Resolved the above in the meantime by explicitly setting the headers in the response before Spring can write its defaults incorrectly.

Background: https://snowowl.atlassian.net/browse/SO-4992

@cmark cmark added the feature label Mar 27, 2024
@cmark cmark requested review from apeteri and nagyo March 27, 2024 13:27
@cmark cmark self-assigned this Mar 27, 2024
cmark added 2 commits March 27, 2024 17:08
...when we build the response upon receiving a resolved/rejected
promise. This prevents header duplication and hopefully overrides
anything that Spring incorrectly adds by default.
Add it to AllSnomedApiTests suite.
@cmark cmark marked this pull request as ready for review March 28, 2024 08:32
Copy link
Member

@apeteri apeteri left a comment

Choose a reason for hiding this comment

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

👍

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Attention: Patch coverage is 81.33333% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 48.21%. Comparing base (d976d85) to head (d7515ae).

Files Patch % Lines
...ernational/snowowl/core/rate/ApiConfiguration.java 33.33% 8 Missing ⚠️
...ore/rest/util/PromiseMethodReturnValueHandler.java 81.25% 2 Missing and 1 partial ⚠️
...ternational/snowowl/core/rate/RateLimitConfig.java 83.33% 2 Missing ⚠️
...ional/commons/exceptions/NotModifiedException.java 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                9.x    #1270      +/-   ##
============================================
+ Coverage     48.19%   48.21%   +0.01%     
- Complexity    13906    13922      +16     
============================================
  Files          1944     1946       +2     
  Lines         95081    95121      +40     
  Branches      10983    10985       +2     
============================================
+ Hits          45822    45859      +37     
+ Misses        46270    46267       -3     
- Partials       2989     2995       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@nagyo nagyo left a comment

Choose a reason for hiding this comment

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

RSLGTM!

public class RateLimitConfig {

@Min(0)
private long overdraft = 0L;
Copy link
Member

Choose a reason for hiding this comment

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

I would love to see a simple one-sentence explanation about the purpose of these values. For future devs I think it would be extremely useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

cmark added 2 commits April 2, 2024 16:13
...which replaces the `api.rate_limit.overdraft` config key

Add some documentation references to ease understanding of how
rate-limiting in Snow Owl works.
Copy link
Member

@apeteri apeteri left a comment

Choose a reason for hiding this comment

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

👍

@nagyo
Copy link
Member

nagyo commented Apr 2, 2024

👌🏻

@cmark cmark merged commit 4633c8c into 9.x Apr 2, 2024
5 checks passed
@cmark cmark deleted the feature/SO-4992-http-cache-control branch April 2, 2024 14:55
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants