Avoid `can't compare offset-naive and offset-aware datetimes` in is_expired check for CachedResponse.

[shaked-seal]

Currently, the following line: return self.expires is not None and utcnow() > self.expires fails on: can't compare offset-naive and offset-aware datetimes Because self.expires is timezone aware but utcnow() is not.

Because all errors are silenced as part of 0.12.0 release, we always return True, even if there's an exception.

Replacing the timezone in self.expires to None in this check will resolve the error, and the comparison will succeed.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.51%. Comparing base (6e549d6) to head (66ea381). Report is 39 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
+ Coverage   96.41%   96.51%   +0.10%     
==========================================
  Files          10       10              
  Lines        1059     1061       +2     
  Branches      185      185              
==========================================
+ Hits         1021     1024       +3     
+ Misses         29       28       -1     
  Partials        9        9              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[alessio-locatelli]

It is still necessary to check that your PR is correct, but at least I can see that we have a problem with poor test coverage, because arbitrary changes to the code do not fail the tests.

@shaked-seal Can you show a small MRE that leads to the described problem? It would be great if you could cover your change with a test, or at least show me some examples so I can extend the tests myself.

[alessio-locatelli]

@JWCook I have an assumption that people call save_response() manually and that low-level helper function allows to store anything as expires:

    await cache.save_response(response, expires="fake_random_wrong_value")
    assert cached_response and isinstance(cached_response, CachedResponse)
    assert cached_response.expires == "fake_random_wrong_value"

    await cache.save_response(response, expires=datetime.now(UTC))

This means that people can save an aware datetime to the cache.

Another problem is that convert_to_utc_naive() is not called for this case, leading to storing an aware datetime.

I think that we must call convert_to_utc_naive() under the save_response() or at least add some validation to prevent storing wrong values under the expires field. Does this make sense to you?

[shaked-seal]

Hello @alessio-locatelli Added a UT. Verified to fails with the old code and works with the new code. I would appreciate your help with this - this issue blocks us from upgrading to Python 3.12 and with the fix we will be able to do that :) Thank you

[alessio-locatelli]

Hello @alessio-locatelli Added a UT. Verified to fails with the old code and works with the new code. I would appreciate your help with this - this issue blocks us from upgrading to Python 3.12 and with the fix we will be able to do that :) Thank you

Could you please show a small example how you use this library?

Sine you want to pass an aware datetime, here is my working MRE that works on the "main" branch with Python 3.12:

import asyncio
from datetime import UTC, datetime, timedelta
from typing import cast
from aiohttp_client_cache import CachedSession, CachedResponse
from aiohttp_client_cache.backends.sqlite import SQLiteBackend


async def func():
    async with CachedSession(
        cache=SQLiteBackend('demo_cache', expire_after=datetime.now(UTC) + timedelta(days=1))
    ) as session:
        for _ in range(2):
            response = cast(CachedResponse, await session.get('http://httpbin.org/delay/1'))
            print(response.expires)
            assert response.expires.tzinfo is None


asyncio.run(func())

and it works because under the hood we convert "naive/aware datetime" -> "UTC datetime" -> "naive datetime".

[shaked-seal]

[Just saw the other PR. I believe it will resolve the issue as well so will wait for any to be merged and released. Thanks!]

@alessio-locatelli Sorry I may not provided all the information. The problem is that the record is always being deleted from the cache. We enabled debug and found that inside the function is_expired, the condition utcnow() > self.expires always returns a failure, and because it is in the try/except we always return True. Meaning the record is always expired and deleted from the cache. When checking via Debug Console, we found that utcnow() > self.expires returns can't compare offset-naive and offset-aware datetimes.

This fix was the only way I found to fix the error and not delete the record every time. To reproduce I would enable debug on the function, and try to run the condition utcnow() > self.expires, or use the old code and run the test I provided in this PR.

The code you provided will work - there is no exception - however, the result will be that we don't actually use the cache because the record always caught as expired.

Thank you

[JWCook]

Thanks @shaked-seal for reporting the issue and @alessio-locatelli for looking into it! I am going to close this and merge #287, since that will handle a couple more cases (and raise errors in cases we can't handle).