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

unsplit the wrapper mixins #1963

Closed
davidism opened this issue Nov 5, 2020 · 3 comments · Fixed by #2003
Closed

unsplit the wrapper mixins #1963

davidism opened this issue Nov 5, 2020 · 3 comments · Fixed by #2003
Assignees
Milestone

Comments

@davidism
Copy link
Member

davidism commented Nov 5, 2020

Request and Response are empty classes which combine a base implementation and some mixins that add different parts of the HTTP spec. None of the mixins change behavior, they only add properties and methods.

There doesn't seem to be any clear reason why the wrappers were designed this way. At first I thought it was to split up code, but then I remembered that I was the one to split up the code, it used to be in one giant wrappers.py file. I also thought it might be to reduce duplication, but only JSONMixin is written to apply to both Request and Response, and that caused its own set of issues. The docs imply that users would want to combine these mixins themselves to create their own subclass, but it's not clear why they would do that instead of subclassing Request/Response.

The classes and docs use a pattern where the mixins are listed after the base implementations in the list of base classes, even though there should be no overlap and the base implemenations provide all the stuff the mixins use. The reversed order can cause issues when combining subclasses that don't use the same order. Ran into this before, but most recently in Flask where it had Response(ResponseBase, JSONMixin) while Werkzeug had TestResponse(JSONMixin, BaseResponse).

Having everything split up but tightly coupled to the base type also causes a lot of issues with static typing, especially when further subclassing. Weird errors pop up saying that a mixin which doesn't define an attribute is the source of a type error with that attribute. There's plenty of ignores scattered around because of this, and it also causes warnings in PyCharm because it doesn't understand where the attributes are coming from.

There's also a lot of inconsistency in the Flask and Werkzeug docs (and I feel like I've seen it in the code as well) with using BaseRequest vs Request. It also makes using them a bit annoying, because you usually want to return a full object, but you have to do instance checks against the base object in case someone subclassed that instead.

The code for the base and mixins should be combined into a single class. The previous classes can be emptied and just raise a deprecation warning in __init__. The __instancecheck__ and __subclasscheck__ special methods can be used to fake those checks against the previous classes and raise a deprecation warning as well.

@davidism davidism added this to the 2.0.0 milestone Nov 5, 2020
@pgjones
Copy link
Member

pgjones commented Nov 5, 2020

I like the sound of this and think if restructuring these classes is considered we should go further. Specifically I've been playing with a branch that notes that requests and responses consist of headers and a body and that only the body part has IO concerns.

I'd like then to move all the body related methods to a body class that a request/response has an instance of. Then I can change better sync or async body versions. This way in Quart or other ASGI frameworks the request's body class can be made async and the response's body class can be inspected to know if it is (i.e. allow either sync or async bodies) whilst using the same header accessing/altering code. Finally for backwards compatibility I'd rewrite all the current body methods to access via the body instance. Hopefully this will be clearer with some example pseudo-code, (or if this sounds good I'll finish off the branch),

class SansIORequest/Response:
    body: BodyProtocol
    headers: Headers
    # All the current header methods e.g.
    @property
    def content_length(self): ...  

# To ensure backwards compatibility including the same name
class Request/Response(SansIORequest/Response):
    # All the current body altering/access methods e.g.
    def get_data(self): 
        return self.body.get_data()

# In an ASGI framework (for a good name), e.g. Quart 
class Request/Response(SansIORequest/Response):
    pass

(I don't like the SansIO name, but I think it makes this comment clearer).

@davidism
Copy link
Member Author

@pgjones I've finished the unsplit work, so I'm closing this issue, if you want you can make a separate issue about extracting sans-io behavior with more detail.

@pgjones
Copy link
Member

pgjones commented Jan 16, 2021

Ta, see #2005

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants