Enhancement: simplify type of response_headers in AppConfig

Summary

I recently implemented a InitPluginProtocolthat, among other things, adds a bunch of response headers to the app. I ran into the problem that the type of AppConfig.response_headers is ResponseHeaders, which is a union of Sequence[ResponseHeader] and Mapping[str, str]: https://github.com/litestar-org/litestar/blob/main/litestar/types/composite_types.py#L53

Because of this, there was no straightforward way to add response headers in a type correct way. I ended up doing this:

response_header_list: list[litestar.datastructures.ResponseHeader]
if hasattr(app_config.response_headers, "items"):
    # Mapping[str, str]
    response_header_list = [
        litestar.datastructures.ResponseHeader(name=name, value=value)
        for name, value in app_config.response_headers.items()
    ]
else:
    # Sequence[ResponseHeader]
    response_header_list = list(app_config.response_headers)
response_header_list.extend(_headers.COMMON_RESPONSE_HEADERS)
app_config.response_headers = response_header_list

If AppConfig.response_headers was not a union, or even just list[ResponseHeader), this would have been as simple as using list.extend. I know that, ultimately, the response headers are type narrowed into tuple[ResponseHeader]. I just wish that, e.g., it would have been either type narrowed to a list first in the AppConfig, or that the app init was less generous with the allowed input formats. AppConfig.response_headers being a union of two immutable types with different signature makes modification unnecessarily complex.

Basic Example

For example, if AppConfig.response_headers was list[ResponseHeader], I could just do this (of course, this example is an artificial toy example and there is no great reason for this plugin to exist as-is):

from litestar.plugins import InitPluginProtocol
from litestar.datastructures import ResponseHeader
from litestar.config.app import AppConfig

class AddSomeResponseHeadersPlugin(InitPluginProtocol):
    def __init__(self, response_headers: list[ResponseHeader) -> None:
        self._headers = response_headers

    def on_app_init(self, app_config: AppConfig) -> AppConfig:
        app_config.response_headers.extend(self._headers)   # typing ok!
        return app_config

Drawbacks and Impact

There are ways to implement this which break backwards-compatibility, and there are ways that don't. For example, if AppConfig.response_headers was changed to list[ResponseHeader] and the conversion to this type happened inside Litestar.__init__, before InitPluginProtocol.on_app_init was called with the AppConfig, thus preserving the arguments to Litestar.__init__, then all compatibility would be preserved, since list[ResponseHeader] is in fact of type ResponseHeaders (just much more narrow).

Unresolved questions

No response

[!NOTE]
While we are open for sponsoring on GitHub Sponsors and OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

If you would like to see an issue prioritized, make a pledge towards it!

We receive the pledge once the issue is completed & verified

This, along with engagement in the community, helps us know which features are a priority to our users.

You are correct. As part of the 1.x to 2.x transition we removed a lot of the app parameters that allowed a union of types for this very reason.

There is also not supposed to be any abstract collection types on the AppConfig object, they should all be concrete types like list, dict etc, and as you can see where we instantiate the config object, we cast the abstract collection types to concrete ones (notwithstanding this issue): https://github.com/litestar-org/litestar/blob/3ea7c1523de3ffb27f2dbcc609a741413f008127/litestar/app.py#L324-L374

Also, response_cookies appears to have the same issue.

We should add a unit test that traverses the annotations on AppConfig to check for abstract collections and unions to ensure that we don't make any more of these errors in the future.

Also, the type narrowing util should also be annotated to return a concrete type, not an abstract type, e.g.,:

def narrow_response_headers(headers: ResponseHeaders | None) -> Sequence[ResponseHeader] | None:
    """Given :class:`.types.ResponseHeaders` as a :class:`typing.Mapping`, create a list of
    :class:`.datastructures.response_header.ResponseHeader` from it, otherwise return ``headers`` unchanged
    """

should really be:

def narrow_response_headers(headers: ResponseHeaders | None) -> tuple[ResponseHeader, ...] | None:
    """Given :class:`.types.ResponseHeaders` as a :class:`typing.Mapping`, create a list of
    :class:`.datastructures.response_header.ResponseHeader` from it, otherwise return ``headers`` unchanged
    """

if AppConfig.response_headers was changed to list[ResponseHeader] and the conversion to this type happened inside Litestar.init, before InitPluginProtocol.on_app_init was called with the AppConfig, thus preserving the arguments to Litestar.init, then all compatibility would be preserved

I don't think this is true, given that it is currently allowed to be a mapping it is feasible that there are downstream users checking for a sequence and converting that to a mapping before making modifications. If we remove the mapping type, that would break that pattern.

I think the best we can do for now is to change the typing on the config object to list | dict, perform the relevant casts on app config creation, and add tests to ensure we don't create any more of these issues.

Then, we can properly correct this on the v3 branch by changing the type on the app config object to list[ResponseHeader].

if AppConfig.response_headers was changed to list[ResponseHeader] and the conversion to this type happened inside Litestar.init, before InitPluginProtocol.on_app_init was called with the AppConfig, thus preserving the arguments to Litestar.init, then all compatibility would be preserved

I don't think this is true, given that it is currently allowed to be a mapping it is feasible that there are downstream users checking for a sequence and converting that to a mapping before making modifications. If we remove the mapping type, that would break that pattern.

I think the best we can do for now is to change the typing on the config object to list | dict, perform the relevant casts on app config creation, and add tests to ensure we don't create any more of these issues.

Then, we can properly correct this on the v3 branch by changing the type on the app config object to list[ResponseHeader].

You are right. Good catch.

litestar-org / litestar