feat: If-Modified-Since for /environment-document#5283
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
github-actions
bot
added
api
Docker builds report
|
|
Uffizzi Preview |
rolodato
force-pushed
the
feat/if-modified-since
branch
from
09cddef to
12af449
Compare
github-actions
bot
added
feature
github-actions
bot
added
feature
matthewelwell
left a comment
matthewelwell
left a comment
There was a problem hiding this comment.
The reason that the tests are failing is because the updated_at value on the environment is updated by a hook on an audit log record getting created (it is not using the usual auto_now logic from django). Since you're updating the name of the environment manually, an audit log record isn't being created (because there's no user associated with the change).
Some options:
- Just manually update the
updated_atvalue on the environment instance - Convert this test into more of an integration test and actually make an update to the environment via the PUT endpoint for example
I'm happy with either option really so I'd recommend just going with option 1 as it's the easiest. As per my comment on the specific lines in the code though, we should add a test to verify that not passing the if-modified-since header still returns the whole document.
| if not self._environment: | ||
| raise AssertionError( | ||
| "Tried to access environment on an authenticated request, but was None" | ||
| ) |
There was a problem hiding this comment.
Maybe this is intentionally verbose, but this can be rewritten as
| if not self._environment: | |
| raise AssertionError( | |
| "Tried to access environment on an authenticated request, but was None" | |
| ) | |
| assert self._environment, ( | |
| "Tried to access environment on an authenticated request, but was None" | |
| ) |
| assert response.status_code == status.HTTP_403_FORBIDDEN | ||
|
|
||
|
|
||
| def test_environment_document_caching( |
There was a problem hiding this comment.
I'd argue that this isn't really 'caching' ?
| # Then - second request should return 304 Not Modified | ||
| assert response2.status_code == status.HTTP_304_NOT_MODIFIED |
There was a problem hiding this comment.
Could we also make a request from a client that doesn't have the If-Modified-Since header to make sure that it still returns the environment document?
github-actions
bot
added
feature
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5283 +/- ##
=======================================
Coverage 97.61% 97.62%
=======================================
Files 1237 1237
Lines 42975 43015 +40
=======================================
+ Hits 41952 41992 +40
Misses 1023 1023 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
feature
rolodato
changed the title
2 participants
Adds support for
If-Modified-SincetoGET /environment-document. This makes it so that if clients already have a recent environment document, the API can respond with an empty 304 instead of serving the whole document.Tests are not yet passing. I understand this is because the environment document cache uses SDK keys and not environment IDs as its cache keys, which means that calling
clear_environment_cachehas no effect if the environment was only ever fetched using server-side SDK keys.