Open kedod opened 8 months ago
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()
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).
@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]:
...
@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 wouldn't the way you've suggested result in the users having to implement both
get_json
andget_xml
? If that's the case, they can already do that. The only benefit of the approach you suggested would be thatLitestar
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? :eyes:
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 aResponse
into anASGIResponse
via theto_asgi_response.
What would be the potential issues if we pass theRequest
object into theResponse
directly (optionally)?
Yeah I don't like that either :grimacing:
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 :eyes:
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.
If anyone ever takes this task, please don't forget Accept
does content negotiation, can take priority in parameter q
, and generally a best match should be implemented.
If anyone ever takes this task, please don't forget
Accept
does content negotiation, can take priority in parameterq
, and generally a best match should be implemented.
That part is already implemented :)
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 theresponse_class
and use it inrender
method later. We can set default serializers to keep consistency with the actual Litestar behaviour too.
One thing we shouldn't forget to take into account though is how to integrate with type_encoders
. As a user, I would expect a type encoder to work for all serializers, so for the non-standard one, we'd probably have to do something like calling msgspec.to_builtins
with the type encoders, and passing that result into the serializer. Also, while we're at it, I'm not sure if serializers
is the best name. Having type_encoders
and serializers
feels a bit inconsistent, and internally, we also use different jargon (rendering, seralizing, encoding). Maybe content_encoders
or media_type_encoders
would be better.
@provinzkraut media_type_encoders
sounds perfect for me.
Summary
In one of 3.0 wishlist points
@rob
suggested to support multiple resource representation in response based onAccept
header https://discord.com/channels/919193495116337154/1182745702942646313/1217489384669184064I'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:
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
Accept
header is set to"*/*"
if request does not provide any (https://github.com/litestar-org/litestar/blob/main/litestar/connection/request.py#L120)media_type
is set on route handler level and passed toResponse.render
method ("application/json"
by default)Response.render
matchesmedia_type
patterns and renders properly encoded messageInterface
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 movemedia_type
argumentResponse
object, right?Take a look at this simpliefied example:
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. Inget_response_handler
we can iterate overresponse_classes
and check if any of them supports specifiedAccept
format. First matched class will be used asresponse_class
in later steps. If none of them will match thenHTTP_406_NOT_ACCEPTABLE
can be returned by default (or not).New
Response
classesBreaking (and not) changes
media_type
in handler definitionUnresolved questions