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

Enhancement: Resource representation response based on Accept header #3272

Open
kedod opened this issue Mar 27, 2024 · 6 comments
Open

Enhancement: Resource representation response based on Accept header #3272

kedod opened this issue Mar 27, 2024 · 6 comments
Assignees
Labels
Enhancement This is a new feature or request

Comments

@kedod
Copy link
Contributor

kedod commented Mar 27, 2024

Summary

In one of 3.0 wishlist points @rob suggested to support multiple resource representation in response based on Accept header https://discord.com/channels/919193495116337154/1182745702942646313/1217489384669184064

I've decided to write few words about current situation, problems, possible interface for this (possible) future feature.

Basic Example

What's all the fuss about?

In the current Litestar 2.x version if we want to return different resource representation based on Accept header we have to it manually using "ifology" which in case of many possible representations (json, xml, csv, yaml...) is not the most elegant solution. This can also lead to code duplication if we have more endpoints.

Simple example can look like this:

from dataclasses import asdict, dataclass
from typing import Annotated

from litestar import Litestar, get
from litestar.enums import MediaType
from litestar.params import Parameter
from litestar.response import Response
from litestar.status_codes import HTTP_406_NOT_ACCEPTABLE


@dataclass
class User:
    name: str
    age: int
    email: str


def convert_user_to_yaml(user: User) -> bytes: ...
def convert_user_to_xml(user: User) -> bytes: ...
def convert_user_to_csv(user: User) -> bytes: ...


@get
def get_user(accept: Annotated[str, Parameter(header="Accept")]) -> User:
    user = User(name="Olga", age=29, email="mail@example.com")

    if accept == "application/x-yaml":
        return Response(content=convert_user_to_yaml(user), media_type=accept)
    if accept == "application/xml":
        return Response(content=convert_user_to_xml(user), media_type=accept)
    if accept == "text/csv":
        return Response(content=convert_user_to_csv(user), media_type=accept)
    if accept in {"application/json", "*/*"}:
        return Response(content=asdict(user), media_type=MediaType.JSON)
    raise Response(status_code=HTTP_406_NOT_ACCEPTABLE)


app = Litestar([get_user])

Sure we can write custom methods and extend base data model classes (pydantic, msgspec, attrs...) but implementing logic for every model type with including/excluding format mechanisms may be complex and time consuming. And we still need to figure out how to handle different responses for DTOs.

Customizable, extendable and easy to use resource representation response based on Accept header can be nice addition to the Litestar. At least I think so :)

Digging into the code

Interface

I like the way how the DRF handle this https://www.django-rest-framework.org/api-guide/renderers/
What if we can "borrow" their idea?
There is obstacle. We have media_type in route handler definition.
But we can move media_type argument Response object, right?

Take a look at this simpliefied example:

class YamlResponse(Response):
    media_type = "application/x-yaml"

    def render(self, content: Any, enc_hook: Serializer = default_serializer) -> bytes:
        return encode_yaml(content, enc_hook)

class XmlResponse(Response):
    media_type = "application/xml"

    def render(self, content: Any, enc_hook: Serializer = default_serializer) -> bytes:
        return encode_xml(content, enc_hook)

class CsvResponse(Response):
    media_type = "text/csv"

    def render(self, content: Any, enc_hook: Serializer = default_serializer) -> bytes:
        return encode_csv(content, enc_hook)

class JsonResponse(Response):
    media_type = "application/json"

    def render(self, content: Any, enc_hook: Serializer = default_serializer) -> bytes:
        return encode_json(content, enc_hook)


# NOTE: response_class renamed to response_classes
@get(response_classes=[JsonResponse, YamlResponse, CsvResponse, XmlResponse])
def user() -> User:
    return User(name="Olga", age=29, email="mail@example.com")

It looks nice.
Simple and reusable.
Supports DTOs.
media_type can be fixed content type, regex or */*

Drawbacks and Impact

Content negotiation

There should be negotiation between client and server to check if Accept header is supported in endpoint.
In get_response_handler we can iterate over response_classes and check if any of them supports specified Accept format.
First matched class will be used as response_class in later steps. If none of them will match then HTTP_406_NOT_ACCEPTABLE can be returned by default (or not).

New Response classes

  • json
  • msgpack
  • text
  • ??

Breaking (and not) changes

  • Lack of media_type in handler definition
  • ???

Unresolved questions

  • ???

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.
Fund with Polar
@kedod kedod added the Enhancement This is a new feature or request label Mar 27, 2024
@kedod kedod self-assigned this Mar 27, 2024
@peterschutt
Copy link
Contributor

Agree it would be nice to have more flexibility with response encoding.

I'm not sure about specifying multiple response class types b/c I think this would worsen the UI for defining a customized response class. E.g,.:

# currently possible

class MyCustomResponse(Response):
    type_encoders = {...}  # example response customization

app = Litestar(..., response_class=MyCustomResponse)

If multiple response class types were defined, that customization would have to be applied to all of them.

I think I noticed a mention in discord about passing request context to the response object in order to be able to discriminate how the response content should be encoded? So how about if we added a method like Response.get_encoder() that housed our default implementation and could be overridden to extend support?

E.g.,

class Response:
    def __init__(self, ..., request_context: ...) -> None: ...

    def get_encoder(self) -> Callable[[Any], bytes]: ...

    def render(self, ...) -> bytes:
        encoder = self.get_content_encoder()

@guacs
Copy link
Member

guacs commented Mar 29, 2024

So how about if we added a method like Response.get_encoder() that housed our default implementation and could be overridden to extend support?

In this case, the user would need to then give an encoder based on any custom logic they have right? I like this, but I think we could also consider taking in a mapping of accept headers to encoders at all layers, and then the default implementation wouldn't be to default to serializing as JSON but to find the encoder based on the accept header and then use that for the encoding (this is only in the case where media_type is not explicitly defined by the user).

@provinzkraut
Copy link
Member

@kedod We already have an Accept header class that allows media-type based matching

I think I noticed a mention in discord about passing request context to the response object in order to be able to discriminate how the response content should be encoded?

I don't really like this idea as it complicates things further. The response shouldn't need to worry about the request. If we were doing this, IMO this functionality should be in the place where the response is created.


Seeing how this falls generally in the category of "metadata based routing", why don't we just do that? We can already define multiple handlers for the same path, given they differ in their method, why not allow the same for another set of parameters, such as the accept header (or any other type of header really)? That would give the users the most flexibility and make for a rather straightforward implementation on our side.

@get("/", match=[Accept(MediaType.JSON)])
async def get_json() -> dict[str, str]:
 ...


@get("/", match=[Accept(MediaType.XML)])
async def get_xml() -> dict[str, str]:
 ...

@guacs
Copy link
Member

guacs commented Mar 29, 2024

@provinzkraut wouldn't the way you've suggested result in the users having to implement both get_json and get_xml? If that's the case, they can already do that. The only benefit of the approach you suggested would be that Litestar would handle the serialization properly. I think what users would find useful is just do:

@get("/")
async def get_foo() -> dict[str, str]:
    ...

Then litestar would automatically figure out which format to serialize into based on the accept header. This would be the simplest from the user perspective.

response shouldn't need to worry about the request

I didn't understand this. We already have a dependency on the Request object when we convert a Response into an ASGIResponse via the to_asgi_response. What would be the potential issues if we pass the Request object into the Response directly (optionally)?

@provinzkraut
Copy link
Member

@provinzkraut wouldn't the way you've suggested result in the users having to implement both get_json and get_xml? If that's the case, they can already do that. The only benefit of the approach you suggested would be that Litestar would handle the serialization properly. I think what users would find useful is just do:

@get("/")
async def get_foo() -> dict[str, str]:
    ...

Then litestar would automatically figure out which format to serialize into based on the accept header. This would be the simplest from the user perspective.

That feels like a different feature to me so maybe we're not talking about the same stuff here? 👀

If we want that, wouldn't the easiest solution to allow specifying additional response serialisers (ideally as layered parameters)?

@get("/")
async def get_foo() -> dict[str, str]:
    ...


app = Litestar(
  [get_foo],
  serializers={MediaType.XML: to_xml}
)

and then Litestar checks if for a given Accept header, an appropriate serialiser is registered?

response shouldn't need to worry about the request

I didn't understand this. We already have a dependency on the Request object when we convert a Response into an ASGIResponse via the to_asgi_response. What would be the potential issues if we pass the Request object into the Response directly (optionally)?

Yeah I don't like that either 😬

It's something we currently need to do, but it's also only a workaround for some other issues we were facing. I can't actually remember anymore what exactly this was.. @peterschutt I remember we were talking about this a lot during the 2.0 churn when this was implemented and I think we agreed that it was the path of least resistance but there was some other way of handling this better.. Would have to dig up the conversation from back then 👀

@kedod
Copy link
Contributor Author

kedod commented Mar 29, 2024

If we want that, wouldn't the easiest solution to allow specifying additional response serialisers (ideally as layered parameters)?

I like this idea the most.
I assume we can pass matched serializer to the response_class and use it in render method later.
We can set default serializers to keep consistency with the actual Litestar behaviour too.

@kedod kedod removed their assignment Apr 6, 2024
@provinzkraut provinzkraut self-assigned this May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This is a new feature or request
Projects
Status: Working on it
Development

No branches or pull requests

4 participants