Abstract the public-facing API away from ClientResponse - #13152
Abstract the public-facing API away from ClientResponse#13152Moist-Cat wants to merge 13 commits into
Conversation
It's important to notice that the __init__ method varies between
implementations and there are some leftovers (e.g, the _in_context field).
The base class is basically an interface.
for more information, see https://pre-commit.ci
Merging this PR will not alter performance
Comparing Footnotes
|
The CI complained but unit tests did not fail.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13152 +/- ##
==========================================
- Coverage 98.98% 98.95% -0.04%
==========================================
Files 132 133 +1
Lines 49073 49113 +40
Branches 2553 2552 -1
==========================================
+ Hits 48576 48598 +22
- Misses 373 391 +18
Partials 124 124
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
| import contextlib | ||
| import json | ||
| from http.cookies import SimpleCookie | ||
| from typing import Any, Callable, Optional, Tuple |
There was a problem hiding this comment.
Callable, Optional, and Tuple are all deprecated.
There was a problem hiding this comment.
Optional doesn't seem deprecated. Still, I will use [type] | None to be consistent with the rest of the code.
| self._in_context = False | ||
| self._released: bool = False | ||
| self._resolve_charset: Callable[[Any, bytes], str] = lambda *_: "utf-8" |
There was a problem hiding this comment.
The subclass doesn't call this. These could just be class defaults anyway, so don't need the init to define them.
| self._resolve_charset: Callable[[Any, bytes], str] = lambda *_: "utf-8" | ||
|
|
||
| # ---------------------------------------------------------------- | ||
| # Abstract / overridable protocol methods |
There was a problem hiding this comment.
We probably want to ensure these are defined, right? In which case this should probably be an abstract class and these methods can all be defined as abstract.
There was a problem hiding this comment.
This requires multiple inheritance (HeadersMixin, ABC), but alright.
| def headers(self) -> Any: | ||
| return self._headers | ||
|
|
||
| @property | ||
| def history(self) -> Tuple[Any, ...]: |
There was a problem hiding this comment.
Seem to have lost typing information that we had before?
There was a problem hiding this comment.
Changing it to tuple["BaseResponse", ...] breaks static analysis for functions that expect tuple["ClientResponse", ...]. In particular, ClientResponseError. I would rather be conservative and use Any than to change too many files and risk conflicts/rollbacks.
I ended up updating all references, in any case.
|
|
||
|
|
||
| class ClientResponse(HeadersMixin): | ||
| class ClientResponse(BaseResponse): |
There was a problem hiding this comment.
So this is the HTTP/1 class, right?
I still think, atleast in master/v4, that we probably want to rename this and have the base class be called ClientResponse. That means that middlewares etc. which reference ClientResponse shouldn't need any changes to work in future with the HTTP/2 class.
With these current changes, we'd need to changes lots of references from ClientResponse -> BaseResponse, both in our code and in user's code.
There was a problem hiding this comment.
I still think, atleast in master/v4, that we probably want to rename this and have the base class be called ClientResponse
This causes type errors as well because we are reducing the capabilities of the ClientResponse class. All the methods that use the response as a connector object via start, read, etc, stop working. Though, reviewing the type errors, it seems that most come from the tests. I'm not sure if any users or middlewares use these methods.
That said, I'm okay with switching the child class with the new parent class if you believe it's better for backwards compatibility.
There was a problem hiding this comment.
Yeah, but that's the HTTP/1 specific methods, right? Any code that currently requires them is going to break regardless, as anything that currently receives ClientResponse, will in future need to handle receiving both HTTP1 and HTTP2 classes.
Code which doesn't depend on those methods will need no updates to annotations etc. if the base class reuses the existing ClientResponse name. So, yeah, I think it's a much easier migration for users if the parent is ClientResponse.
There was a problem hiding this comment.
Yeah, but that's the HTTP/1 specific methods, right? Any code that currently requires them is going to break regardless, as anything that currently receives ClientResponse, will in future need to handle receiving both HTTP1 and HTTP2 classes.
Yes, that's true.
Code which doesn't depend on those methods will need no updates to annotations etc. if the base class reuses the existing ClientResponse name. So, yeah, I think it's a much easier migration for users if the parent is ClientResponse.
That makes sense to me.
|
The main problem is that the code does not differentiate when the request object is used as a data structure and when it's used as a connector object that holds the current state of the connection. After finding more examples of this mixed behaviour it appears to me at the present that it would be difficult to implement If you agree, I believe the best course of action would be to try to add |
OK, how about you start preparing these changes as a stacked PR? I'll ask for us to be opted-in to the stacked PRs beta, then you can create it. That way you can show the full implementation, and we can merge the iterative steps as we go. |
Oh, this is the first time I hear about stacked PRs. Yes, that's perfect. I will need some time to study the codebase and design an architecture that can accommodate the three protocols and doesn't break the public API so I will be switching between refactoring, and adapting the |
|
I've just been told they have a lot of requests for the beta already, so not sure if we'll get added or not. Will update you if I hear back. |
|
Whether due to my pestering or not, stacked PRs are now in public preview, so we can use them. So, feel free to create a stacked PR. One thing to include in the full PR would be separate HTTP/2 codspeed benchmarks, so we can compare the performance between the HTTP/1 and HTTP/2 stacks. |
HTTPResponse is the old ClientRequest that mixed presentation with connection handling. In the case of the tests, HTTPResponse was aliased to avoid conflicts.
It's important to notice that the
__init__method varies between implementations and there are some extra fields (e.g, the _in_context field) that are required by some methods used outside the class.The base class is basically an interface that implements the public API. Fields can't be stored in the base class because they may or may not be available during
__init__so I'm thinking of removing the constructor altogether.Related issues
#11743
Related PR
#13039