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

Support for conditional requests #188

Merged
merged 11 commits into from
Oct 27, 2023

Conversation

netomi
Copy link
Contributor

@netomi netomi commented Oct 3, 2023

This PR adds support for conditional requests.

I extracted the relevant code in a separate method so that I can check in a test whether it was called with mocking.
However, it would be great if httpbin would support an endpoint to changes its etag over time. Will look into that and provide a patch there so we can use it here as well if accepted.

This fixes #79 .

Code cov needs to be improved though.

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (8ed3214) 97.21% compared to head (69c2b49) 96.32%.
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #188      +/-   ##
==========================================
- Coverage   97.21%   96.32%   -0.90%     
==========================================
  Files          10       10              
  Lines         934      979      +45     
  Branches      171      181      +10     
==========================================
+ Hits          908      943      +35     
- Misses         15       23       +8     
- Partials       11       13       +2     
Files Coverage Δ
aiohttp_client_cache/backends/base.py 96.77% <ø> (ø)
aiohttp_client_cache/cache_control.py 98.43% <100.00%> (+0.16%) ⬆️
aiohttp_client_cache/session.py 84.72% <67.74%> (-13.16%) ⬇️

... and 2 files with indirect coverage changes

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

@netomi netomi marked this pull request as ready for review October 26, 2023 12:49
@JWCook JWCook added this to the v0.10 milestone Oct 27, 2023
@JWCook JWCook added the enhancement New feature or request label Oct 27, 2023
@JWCook JWCook merged commit c3299d4 into requests-cache:main Oct 27, 2023
6 of 8 checks passed
@JWCook
Copy link
Member

JWCook commented Oct 27, 2023

Thank you for putting this together. I appreciate it!

@netomi
Copy link
Contributor Author

netomi commented Oct 27, 2023

I still work on improving code coverage in the test, need a proper endpoint I can use during the tests. httpbin does not support that and is basically a dead project. It probably makes sense to create a fork supporting something like that.

I have a pretty good idea how this could be done, e.g. a /cache/10 endpoint that updates the etag every 10s.

The etag would be generated from the current time in an interval based on the parameter.

There is a function available that can do that: https://gist.github.com/mscheper/9c39091e12f2ca8addb3d5eef7a40480

Then you could write a test that calls /cache/1, waits 2s and make a conditional request with the etag from before. Now the etag has changed and the server should respond with a 200 with the new content.

@JWCook
Copy link
Member

JWCook commented Oct 27, 2023

@netomi I believe I handled this in requests-cache with mock responses instead of real requests + httpbin. I used the requests-mock library there, and aioresponses is a similar one available for aiohttp.

If you want to test with a real endpoint, I think it would be simpler to make a separate test app rather than forking httpbin. Here's an example with Flask that does what you describe:

from datetime import datetime
from hashlib import md5
from flask import Flask, request

app = Flask(__name__)


@app.route("/cache/<int:interval>")
def cache(interval):
    """Cacheable endpoint that uses an ETag that resets every `interval` seconds"""
    now = datetime.now()
    server_etag = md5(str(now.second // interval).encode()).hexdigest()
    request_etag = request.headers.get("If-None-Match")
    if request_etag == server_etag:
        return ("NOT MODIFIED", 304)
    else:
        return ("OK", 200, {"ETag": server_etag})

@netomi
Copy link
Contributor Author

netomi commented Oct 27, 2023

that is really nice will work on a PR

@netomi netomi deleted the feat-conditional-requests branch October 30, 2023 19:28
@JWCook
Copy link
Member

JWCook commented Oct 30, 2023

These changes are now available in v0.10. Thank you again for your help!

Just fyi, at some point in the future I may merge some of this logic into the CacheActions class (similar to the current layout in requests-cache, to separate general HTTP cache logic from the aiohttp-specific parts).

@netomi
Copy link
Contributor Author

netomi commented Oct 30, 2023

great, so I can update my app to use that version and remove my workaround. Putting the logic into the CacheActions class certainly makes sense!

btw. could you also make a release from the 0.10.0 tag? The latest release is still 0.9.

@JWCook
Copy link
Member

JWCook commented Oct 30, 2023

Yes, I'll add that shortly.

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 this pull request may close these issues.

Add support for conditional requests with ETag and Last-Modified headers
2 participants