:art::building_construction: Review and rewrite request, response and message classes and respective subclasses from pigeon.http

[lstuma]

Proposal @raspitim

markdown at https://pastebin.com/Yfr5yKRJ

[lstuma]

from outside they should behave exactly the same as before @raspitim

only significant change from outside is that render has been moved into builtin string method. this allows to using objects that aren't actually specified as long as they use the str method. second significant change is that the data, files and get (files and get missing in diagram) params will now be accessed through lowercase attributes.

biggest question currently is whether HTTPHeaders._headers should be of type list or dict.

but rewriting like this allows for easier handling of complex headers that may need surgical precision when editing (e.g. cookies)

[lstuma]

optionally HTTPHeaders._headers could also be a custom utils.commons.ParameterDict:

class CustomParameterDict(ParameterDict): def getitem(self, key): return super().getitem(key.lower())

[raspitim]

Why don't we move protocol, path and method to HTTPMessage and only modify the methods of HTTPRequest and HTTPResponse?

[raspitim]

And also, what about the HTTPRequests credentials attribute?

[raspitim]

optionally HTTPHeaders._headers could also be a custom utils.commons.ParameterDict:

class CustomParameterDict(ParameterDict): def getitem(self, key): return super().getitem(key.lower())

I think this option would be most coherent with our use of data structures in similar situations (like dynamic params)

[lstuma]

Why don't we move protocol, path and method to HTTPMessage and only modify the methods of HTTPRequest and HTTPResponse?

HTTPResponses don't have path or method. They do use a certain protocol of course though. EDIT: Oh now I see it - I f*cked up the class diagram

And also, what about the HTTPRequests credentials attribute?

That one I forgot as well

The ParameterDict class used for HTTPHeaders._headers would be:

class LowerParameterDict:
    def getitem(self, key):
        return super().getitem(key.replace('_', '-').lower())

(This would allow us to access headers with dashes like Content-Type as well.)

markdown at https://pastebin.com/tqNA86SS

EDIT: ClassDiagram shown above is totally off

[lstuma]

Thinking about this - cookies could also be added to HTTPMessage as a LowerParameterDict or ParameterDict, we should decide on either though and not use both.

HTTPMessage.cookies: LowerParameterDict | ParameterDict

[lstuma]

Just realised that GitHub Issues support mermaid ClassDiagrams - don't know where they weren't supported - I think release notes..

classDiagram
direction TD

class HTTPHeader {
    + name: str
    - _directives: list[str, ...]

    + __init__(self, name, directives) -> HTTPHeader
    + @property value(self) -> str
    + __str__(self) -> str
}

class HTTPHeaders {
    - _headers: LowerParameterDict

    + __init__(self, headers: dict | list[HTTPHeader, ...]) -> HTTPHeaders
    + get(self, key) -> str
    + set(self, key, value) -> None
    + __getitem__(self, key) -> str
    + __setitem__(self, key, value) -> None
    + __str__(self) -> str
}

class HTTPData:::abstract {
    + __str__(self) -> str
}

class HTTPMessage:::abstract {
    + headers: HTTPHeaders
    + data: HTTPData
    + cookies: ?ParameterDict

    + __str__(self) -> str
    + __init__(self) -> HTTPMessage
}

class HTTPRequest {
    + protocol: str
    + method: str
    + path: str
    + get: ParameterDict
    + files: ParameterDict
    + credentials: Credentials

    + __str__(self) -> str
    + __init__(self) -> HTTPRequest
}

class HTTPResponse {
    + __str__(self) -> str
    + __init__(self) -> HTTPResponse
}

HTTPHeader o-- HTTPHeaders
HTTPHeaders o-- HTTPMessage
HTTPData o-- HTTPMessage
HTTPMessage <|-- HTTPResponse
HTTPMessage <|-- HTTPRequest

[lstuma]

@raspitim

The question whether to add cookies highlights a rather general question of how strictly we should tailor the classes towards the standard middleware. Currently HTTPResponse and HTTPRequest objects have a lot of attributes set externally and not in __init__ which I would say is not really clean code. (see e.g. this stack overflow post)

I think tailoring them more toward our middleware is not even a problem, as long as we do not actually add attributes that are specific to our middleware, but can work with user-written middleware as well.

So I would say adding HTTPRequest.credentials and HTTPMessage.cookies is a necessity for code readability and usage as it allows for both easier processing and better usage.

We'll need to have a look at what to do with middlewaretags another time , though they are as well - very unspecific, they still seem to incorporate middleware deeply inside elements that are not necessarily bound to the middleware.

[lstuma]

I think we could also add shortcuts for certain headers such as the Content-Type header for example.:

class HTTPMessage:
    @property
    def content_type(self):
        return self.headers.content_type

though as I am writing this I do realise the small gap between implementing it and just going with message.headers.content_type. maybe we should not use any shortcuts like these internally but allow developers to use them if they want.

[lstuma]

let's have HTTPHeaders inherit from LowerParameterDict - just need to manage setting headers like content-type etc.

[raspitim]

I'm not sure if the shortcuts are necessary but I'm in with adding more attributes directly to the __init__ method of HTTPResponse and HTTPRequest

[lstuma]

got headers working through aggregation for now - this will suffice for now as it implements all external features very nicely.

this allows accessing e.g. the content-type header over headers.content_type

similar to ParameterDict, HTTPHeaders[key] is unsafe and can return error and HTTPHeaders.key returns value or None

[lstuma]

implemented HTTPMessage as shown in diagram apart form cookies (will be added on feature branch in future)

[lstuma]

done in 5bb792e27c4953e76ab203a7ad14c3038cdf8d9d

[lstuma]

implemented: EDIT: yes I know implementation doesn't match 100% what was discussed. feel free to reopen if you wanna propose further changes.

classDiagram
direction TD

class HTTPHeader {
    + name: str
    - _value: Any

    + __init__(self, name, directives) -> HTTPHeader
    + @property value(self) -> str
    + @value.setter value(self, value) -> None
    + __str__(self) -> str
}

class HTTPHeaders {
    - _headers: LowerParameterDict

    + __init__(self, headers: dict[str, str]=None) -> HTTPHeaders

    + __getattr__(self, key) -> str
    + __setattr__(self, key, value) -> str
    + __getitem__(self, key) -> str
    + __setitem__(self, key, value) -> None
    + items(self)
    + keys(self)
    + contains(self, key)
    + __str__(self) -> str
}

class HTTPData:::abstract {
    + __str__(self) -> str
}

class HTTPMessage:::abstract {
    + headers: HTTPHeaders
    + data: HTTPData
    + protocol: str

    + __init__(self, headers: dict[str, str], data: str, protocol: str, content_type=None) -> HTTPMessage
    + @property is_error(self) -> bool
    + set_headers(self, headers:dict) -> None
    + __str__(self) -> str
    + __bytes__(self, encoding='utf-8') -> bytes
}

class HTTPRequest {
    + method: str
    + path: str
    + get: ParameterDict
    + files: ParameterDict
    + auth: Credentials

    + __init__(self, method: str, path: str, headers: dict = None, get: dict = None, data=None, files=None, protocol: str='1.1', content_type=None) -> HTTPRequest
    + @property is_error(self) -> bool
}

class HTTPResponse {
    + status: int

    + __str__(self) -> str
    + __init__(self, headers: dict = None, data: str = None, status: int = 200, cookies=None, protocol: str = '1.1', content_type=None) -> HTTPResponse
    + @property is_error(self) -> bool
    + __bytes__(self, encoding='utf-8') -> bytes
}

HTTPHeader o-- HTTPHeaders
HTTPHeaders o-- HTTPMessage
HTTPData o-- HTTPMessage
HTTPMessage <|-- HTTPResponse
HTTPMessage <|-- HTTPRequest