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

Gracefully fail when freshness cannot be calculated #104

Merged
merged 8 commits into from
Nov 9, 2023

Conversation

parkerhancock
Copy link
Collaborator

This is a simple change to remove a Runtime Error when a freshness can't be calculated. If freshness can't be calculated, then the request should be sent, rather than error out.

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Merging #104 (2e38f49) into master (8c31fe9) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #104   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        27           
  Lines         1666      1675    +9     
=========================================
+ Hits          1666      1675    +9     
Files Coverage Δ
hishel/_controller.py 100.00% <100.00%> (ø)
tests/test_controller.py 100.00% <100.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@karpetrosyan karpetrosyan added the enhancement New feature or request label Nov 9, 2023
@karpetrosyan
Copy link
Owner

karpetrosyan commented Nov 9, 2023

Thank you very much; that makes sense.

So RFC 9111 says:

Caches are encouraged to consider responses that have invalid freshness information (e.g., a max-age directive with non-integer content) to be stale

That means we should re-validate the response instead of raising a RuntimeError.

@karpetrosyan
Copy link
Owner

Can you please add a test and changelog for this PR?

Here is how to test it:

def test_freshness_lifetime_invalid_information():
    controller = Controller()
    response = Response(
        status=200,
    )
    original_request = Request("GET", "https://example.com")
    request = Request("GET", "https://example.com")
    conditional_request = controller.construct_response_from_cache(
        request=request, response=response, original_request=original_request
    )
    assert isinstance(conditional_request, Request)

hishel/_controller.py Outdated Show resolved Hide resolved
@parkerhancock
Copy link
Collaborator Author

Requested changes made! I also confirmed that the new test added actually tests for this change (I wrote the test, reverted the change to _controller.py, confirmed failing, restored change to _controller.py, test now passes).

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
tests/test_controller.py Show resolved Hide resolved
@karpetrosyan
Copy link
Owner

Great job! Thank you @parkerhancock

@karpetrosyan karpetrosyan merged commit aa22fb1 into karpetrosyan:master Nov 9, 2023
7 checks passed
@karpetrosyan karpetrosyan mentioned this pull request Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request RFC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants