Forbid wrong 'expires' values when low-level API is used that silently results in always expired responses

[alessio-locatelli]

This PR closes #286

As I explained on that page:

1. convert_to_utc_naive() is not called for low-level API and it is a user responsibility to convert to a naive datetime. On our side we can raise an error.
2. save_response() and from_client_response() allow to save any random object as expires, including classes, functions, etc. and all these bad things work silently.

An alternative solution is moving convert_to_utc_naive() call inside the from_client_response().

---

Notes:

        except (AttributeError, TypeError, ValueError):

1. AttributeError is impossible because self.expires = None is the default class attribute.
2. TypeError is handled now by verifying that we work with a naive datetime.
3. ValueError - I have no idea how you can get it when you compare two datetime objects. Perhaps an old leftover.

Breaking changes:

There are no breaking changes.
Users who used the low-level functions directly (save_response() and from_client_response()) incorrectly by passing a wrong expires value will get an error. This is expected because so far, with the old code, their cache never worked correctly.

---

The commit history is clean and tidy, so do not squash the commits.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.51%. Comparing base (6e549d6) to head (26bf273).
Report is 43 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
+ Coverage   96.41%   96.51%   +0.10%     
==========================================
  Files          10       10              
  Lines        1059     1063       +4     
  Branches      185      186       +1     
==========================================
+ Hits         1021     1026       +5     
+ Misses         29       28       -1     
  Partials        9        9              

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

[alessio-locatelli]

@layday Would you like to take a look on this change?

[JWCook]

Do you have time to publish a patch release for this?

If not, I can do that after work today.

[alessio-locatelli]

Do you have time to publish a patch release for this?

If not, I can do that after work today.

I think there is no rush to release this immediately. Please update the changelog and release a package today/tomorrow if you have time. In the meantime, maybe someone else will provide a feedback based on the "main" branch.

[JWCook]

Sure, sounds good.

[levpachmanov]

Hi @JWCook @alessio-locatelli, can you please release a version with this fix?

[shaked-seal]

@JWCook @alessio-locatelli I tried to use 0.12.4.dev33

And got a failure:

self = CachedResponse(method='GET', reason='OK', status=200, url=URL('https://pypi.org/simple/ddtrace/?q=RNvnAvOpyEVAoNGnVZQU...org/simple/ddtrace/?q=RNvnAvOpyEVAoNGnVZQU'), history=(), last_used=datetime.datetime(2024, 10, 30, 9, 25, 18, 370048))

    @property
    def is_expired(self) -> bool:
        """Determine if this cached response is expired"""
>       return self.expires is not None and utcnow() > self.expires
E       TypeError: can't compare offset-naive and offset-aware datetimes

.venv/lib/python3.12/site-packages/aiohttp_client_cache/response.py:170: TypeError

So I believe the original issue still exists?

[alessio-locatelli]

@JWCook @alessio-locatelli I tried to use 0.12.4.dev33

And got a failure:

self = CachedResponse(method='GET', reason='OK', status=200, url=URL('https://pypi.org/simple/ddtrace/?q=RNvnAvOpyEVAoNGnVZQU...org/simple/ddtrace/?q=RNvnAvOpyEVAoNGnVZQU'), history=(), last_used=datetime.datetime(2024, 10, 30, 9, 25, 18, 370048))

    @property
    def is_expired(self) -> bool:
        """Determine if this cached response is expired"""
>       return self.expires is not None and utcnow() > self.expires
E       TypeError: can't compare offset-naive and offset-aware datetimes

.venv/lib/python3.12/site-packages/aiohttp_client_cache/response.py:170: TypeError

So I believe the original issue still exists?

@shaked-seal If you stored aware datetimes to the cache manually, this can be a leftover from the previous package version. Try to clean the cache.
I have no idea how to reproduce this unless you show a small example of how you use the library.

[shaked-seal]

I was able to reproduce when overriding "expires" to have a time-zone. We removed that. I added a check that when using CachedResponse the expires shouldn't be with timezone.

Thank you for the quick fix!