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 support for conditional requests with ETag and Last-Modified headers #79

Closed
JWCook opened this issue Aug 14, 2021 · 3 comments · Fixed by #188
Closed

Add support for conditional requests with ETag and Last-Modified headers #79

JWCook opened this issue Aug 14, 2021 · 3 comments · Fixed by #188
Labels
enhancement New feature or request
Milestone

Comments

@JWCook
Copy link
Member

JWCook commented Aug 14, 2021

Quick summary of how this would work:

  • If a cached response contains an ETag header, add an If-None-Match header to the new request
  • If a cached response contains a Last-Modified header, add an If-Modified-Since header to the new request
  • If a 304 Not Modified response is received, return the cached response and update expiration and headers (if changed)
  • If a 200 response is received, update the cache and return the new response
@JWCook JWCook added the enhancement New feature or request label Aug 14, 2021
@JWCook JWCook changed the title Add support for ETag and Last-Modified headers Add support for conditional requests with ETag and Last-Modified headers Aug 28, 2021
@JWCook JWCook added this to the v0.6 milestone Aug 28, 2021
@JWCook JWCook modified the milestones: v0.6, v0.7 Feb 12, 2022
@JWCook JWCook removed this from the v0.7 milestone May 21, 2022
@JWCook JWCook added this to the v0.9 milestone Jul 24, 2023
@JWCook JWCook modified the milestones: v0.9, v0.10 Sep 19, 2023
@netomi
Copy link
Contributor

netomi commented Oct 1, 2023

I have a workaround implemented which can be found here:

https://github.com/netomi/otterdog/blob/main/otterdog/providers/github/rest/requester.py#L182

I am willing to work on a patch to have this integrated in this library by default.

Something that I noticed while working on the workaround: when using session.disabled() to temporarily disable the cache, this is only valid for getting requests from the cache, the cache itself is still updated with responses received from the remote server, this should be fixed imho, as it is misleading, will create a separate ticket for it.

@JWCook
Copy link
Member Author

JWCook commented Oct 2, 2023

I am willing to work on a patch to have this integrated in this library by default.

That would be great! Thank you.

when using session.disabled()... the cache itself is still updated with responses

That's not the intended behavior, and this is supposed to be handled here. Thanks for the heads up.

@netomi
Copy link
Contributor

netomi commented Oct 2, 2023

Created #182 to illustrate the problem. The problem is that there are 2 ways to determine whether the cache is used. Via the cache actions and the is_cacheable method which leads to very wrong behavior when the cache is disabled which is hidden from the user. I just noticed some problems as strangely some cached responses got removed and I wanted to understand why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants