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

Set Cache-Control: no-cache for responses with an ETag #141

Merged
merged 8 commits into from
Nov 14, 2024

Conversation

garyttierney
Copy link
Member

@garyttierney garyttierney commented Nov 12, 2024

Background: right now browsers will cache IIIF resources to disk for the caching period before checking if new content is available. That means we're never sending up an If-None-Match and getting a corresponding 304, so we end up with stale data in the browser. This change makes clients always send an If-None-Match before using the local cache.

Related to #140

Copy link
Member

@donaldgray donaldgray left a comment

Choose a reason for hiding this comment

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

LGTM - brackets on same line is inconsistent with rest of code base (nothing enforcing this).

Worth adding a test, a regression here would cause FE issues?

@garyttierney
Copy link
Member Author

Worth adding a test, a regression here would cause FE issues?

What test suite would that go in (i.e. do we have an integration suite in the .NET code, or do you mean e2e)?

@garyttierney
Copy link
Member Author

LGTM - brackets on same line is inconsistent with rest of code base (nothing enforcing this).

Ah, I ran the auto-formatter but it changed way more, let me re-run it just on those lines.

@garyttierney
Copy link
Member Author

Actually, it was the auto-formatter that put the braces on the same line. Can someone check the correct code style into the shared DotSettings in Rider?
image

@donaldgray
Copy link
Member

What test suite would that go in (i.e. do we have an integration suite in the .NET code, or do you mean e2e)?

Integration tests are in https://github.com/dlcs/iiif-presentation/tree/main/src/IIIFPresentation/API.Tests/Integration - I think a test for this could be added to GET Manifest and/or Collection tests.

Using existing integration test class will bootstrap db (and localstack if required) and configure WebApplicationFactory for making http requests.

@garyttierney garyttierney merged commit 76696e2 into main Nov 14, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache-Control headers causes browser to cache incorrectly after updates
2 participants