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

Fix interface compatibility by inheriting from Response #254

Merged

Conversation

alessio-locatelli
Copy link
Collaborator

@alessio-locatelli alessio-locatelli commented Sep 27, 2024

Closes #251

What

Now CachedResponse inherits from Response.

Breaking changes

No, but there might be some edge cases that I could overlook.

Small notes

  • After the change, some unit tests are redundant, such as accessing the links attribute. In the old code, we had to redefine all the attributes and properties, but in the new code, we rely on the existing class.
  • CachedStreamReader was a bit tricky. This is what you may want to review more carefully.
  • To solve "pickling" errors I had to exclude a few private attributes (e.g. _loop), but for real use cases we never need to restore them.
  • About try/except in aiohttp_client_cache/backends/base.py:158 please read https://github.com/alessio-locatelli/aiohttp-client-cache/pull/1#discussion_r1778760304

@alessio-locatelli alessio-locatelli changed the title Fix interface compatibility Fix interface compatibility inheriting from Response Sep 27, 2024
@alessio-locatelli alessio-locatelli changed the title Fix interface compatibility inheriting from Response Fix interface compatibility by inheriting from Response Sep 27, 2024
skip-checks:true
@JWCook
Copy link
Member

JWCook commented Sep 30, 2024

Thank you for taking the time to work on this.

This will take me a little more time to review and test to see if it introduces breaking changes not covered by tests. I have questions about a couple of these changes, and I'm not necessarily disagreeing, but I'd like to understand the rationale:

  • Why get rid of attrs? A subclass can still be an attrs class, and that library provides a number of nice features that users may or may not be using currently on CachedReponse objects. Removing it will also have some subtly different behaviors when going through (de)serialization. There could be valid reasons to stop using it, though.
  • Why cast new responses to CachedResponse before returning from CachedSession._request()? Currently it returns new responses as the original ClientResponse class.

@alessio-locatelli
Copy link
Collaborator Author

alessio-locatelli commented Sep 30, 2024

Why get rid of attrs? A subclass can still be an attrs class, and that library provides a number of nice features that users may or may not be using currently on CachedReponse objects. Removing it will also have some subtly different behaviors when going through (de)serialization. There could be valid reasons to stop using it, though.

When I read the existing code, I saw use cases for attrs:

  1. not having to define __slots__ manually;
  2. saving some effort on writing __init__ (similar to a "dataclass");
  3. create CachedResponse by feeding another class (Response) without inheritance.

After the refactoring I found no more applicable cases for the attrs.

I've read the attrs documentation, but I've never used it in real life for my personal or work projects. As a result, you may be able to use it more broadly, or find some very specific use cases.

I will be happy to review your commits that add some attrs usage again, or you can clarify the details and I will be happy to try using attrs on my end and ask for a re-review.

@alessio-locatelli
Copy link
Collaborator Author

alessio-locatelli commented Sep 30, 2024

Why cast new responses to CachedResponse before returning from CachedSession._request()? Currently it returns new responses as the original ClientResponse class.

The ClientSession._request() really returns CachedResponse because I pass the response_class=CachedResponse argument to the ClientSession. Now there is no need to convert or create classes on the fly after getting a response.

@JWCook JWCook added this to the v0.12 milestone Oct 1, 2024
@JWCook
Copy link
Member

JWCook commented Oct 1, 2024

I will be happy to review your commits that add some attrs usage again

Alright, let's try it without attrs, and I can add it back later if needed. Thanks again for the contributions!

@JWCook JWCook merged commit f26321e into requests-cache:main Oct 1, 2024
6 checks passed
@alessio-locatelli
Copy link
Collaborator Author

To be clear, what would you like to see as the next step?

  1. Do I have to release a new version now?
  2. Want to release a new version yourself later?
  3. Do not release for now?
  4. Publish a pre-release? This can be a nice option right now.

@JWCook
Copy link
Member

JWCook commented Oct 1, 2024

It looks like there aren't any other issues to address immediately, so I am fine with publishing a new release. I can take care of that after work today. The only other thing I want to check first is updating Sphinx and other doc dependencies, and making sure the docs still render correctly.

@JWCook
Copy link
Member

JWCook commented Oct 2, 2024

I found a possible issue when testing with python 3.13 alpha:

test/integration/base_backend_test.py:113: in get_url
    return await mysession.get(url)
.venv/lib/python3.13/site-packages/forge/_revision.py:322: in inner
    return await callable(*mapped.args, **mapped.kwargs)
aiohttp_client_cache/session.py:119: in _request
    CachedResponse, await super()._request(method, str_or_url, **kwargs)
.venv/lib/python3.13/site-packages/aiohttp/client.py:688: in _request
    resp.close()
.venv/lib/python3.13/site-packages/aiohttp/client_reqrep.py:1081: in close
    self._notify_content()
.venv/lib/python3.13/site-packages/aiohttp/client_reqrep.py:1156: in _notify_content
    content = self.content
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <ClientResponse(http://localhost:8080/get?page=499) [None None]>
None

    @property
    def content(self) -> StreamReader:
>       if self._content is None:
E       AttributeError: 'CachedResponse' object has no attribute '_content'. Did you mean: 'content'?

I couldn't consistently reproduce it, and it only came up when testing concurrent requests. I can't tell exactly why that would happen, but I believe initializing _content in CachedResponse.__init__ fixes it.

@JWCook
Copy link
Member

JWCook commented Oct 2, 2024

These changes have been released in v0.12!

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

Successfully merging this pull request may close these issues.

aiohttp 3.10.6+ compatibility
2 participants