Litestar
Replies: 9 comments 44 replies
-
General caching considerationsNot just for that; I'd say they are mainly useful in conjunction with caching. If starlite were to add support for Etags (which I'm quite interested in - they fit my use case well), It'd be great to adopt it not only as a response header, but also add it to cached responses, and respect the complementary headers like In general, with support for the Right now, setting up a well rounded caching cascade requires quite a lot of tinkering, boilerplate code and configuration in multiple places. For Etags specifically, integration with the response caches is - in my opinion - a central aspect, because that's a major use case for Etags. I guess this should be a separate discussion about a "unified caching API"? Etag APIAs for the # General Etag representation object. Contains the logic to serialise/parse
# to and from headers. Can be used on a handler with the etag keyword
class Etag:
def __init__(self, value: str, weak: bool = True) -> None:
...
def to_header(self) -> str:
"""Serialise an etag into a header-value"""
@classmethod
def from_header(cls, header: str) -> Etag:
"""Parse a header-value into an etag"""
def __eq__(self, other: str | Etag) -> bool:
"""Compare two etags. If necessary, parse `other` into an etag first"""# starlite/handlers/http.py
# Add an etag parameter, which accepts either a string, an Etag instance,
# or a callable that takes a response as its first argument and returns an Etag
# instance.
#
# If a string is passed, it will be set as the `Etag` header as-is.
#
# If an Etag instance is passed, it will be serialised into a header value and
# added as the `Etag` header.
#
# If a callable is passed, it will be called after the response has been constructed
# and the resulting Etag instance will be serialised and added as an `Etag` header
class HTTPRouteHandler(BaseRouteHandler["HTTPRouteHandler"]):
def __init__(
self,
# other parameters omitted for brevity
etag: str | Etag | Callable[[Response], Etag] | None = None,
**kwargs: Any
) -> None:
...Usage could then look something like this: def make_etag(response: Response) -> Etag:
...
@get("/", etag=Etag(value="foo", weak=False))
def resource_with_etag() -> list:
...
@get("/", etag=make_etag)
def resource_with_dynamic_etag() -> list:
...
@get("/", etag='/W"<etag value>"')
def resource_with_string_etag() -> list:
...
|
Beta Was this translation helpful? Give feedback.
-
|
This looks good. Lets bring @seladb into the discussion, as well as @peterschutt, @cofin and @infohash . |
Beta Was this translation helpful? Give feedback.
-
|
This looks good. I'll study more about its implementation. |
Beta Was this translation helpful? Give feedback.
-
|
The implementation itself should be fairly trivial. The only thing I'm unsure about is where to best resolve the callable-style etag. It has to be after the response object is available, for which the earliest point would be in HTTPRouteHandler.to_response, but that somehow feels a bit clunky. Maybe it would be better to actually implement it as an On the other hand, at that point it might actually be the better solution to make users pass it explicit to the handler decorator like so: def after_request_handler(response: Response) -> Response:
# reusing the make_etag function from the previous example, but it yould
# also be returning a string directly
response.headers.update({"Etag": make_etag(response).to_header()})
return response
@get("/", after_request=after_request_handler)
def resource_with_dynamic_etag() -> list:
...Passing in a callable that generates an If it's just a convenience wrapper to create a header field, I'd say it's better to not create any special logic in |
Beta Was this translation helpful? Give feedback.
-
|
First draft of an implementation: # starlite/datastructures/etag.py
import re
from dataclasses import dataclass
from typing import Optional, Union
ETAG_RE = re.compile(r'(/W)?"(.+)"')
@dataclass(eq=False, frozen=True)
class Etag:
value: str
weak: Optional[bool] = True
def to_header_value(self) -> str:
"""Serialise to a string for usage as an `Etag` header value"""
header = f'"{self.value}"'
if self.weak:
header = header + "/W"
return header
@classmethod
def from_header_value(cls, header_value: str) -> "Etag":
"""Parse a header-value into an etag"""
header_value = header_value.strip()
match = ETAG_RE.match(header_value)
if not match:
raise ValueError(f"Invalid etag header: {header_value!r}")
weak, value = match.group(1, 2)
return cls(value=value, weak=weak is not None)
def __eq__(self, other: Union[str, "Etag"]) -> bool:
"""Compare two `Etag`s. If necessary, parse `other` into an `Etag` first"""
if isinstance(other, str):
other = Etag.from_header_value(other)
elif not isinstance(other, Etag):
return NotImplemented
return self.weak == other.weak and self.value == other.value# starlite/handlers/http.py
...
def _etag_after_request_handler(etag, original_handler, response: Response) -> Response:
if callable(etag):
etag = etag(response)
etag_value = etag.to_header_value() if isinstance(etag, Etag) else etag
response.headers.update({"Etag": etag_value})
# i think resolving user-defined handlers last makes the most sense?
return original_handler(response) if original_handler else response
...
class HTTPRouteHandler(BaseRouteHandler["HTTPRouteHandler"]):
@validate_arguments(config={"arbitrary_types_allowed": True})
def __init__(
self,
path: Union[Optional[str], Optional[List[str]]] = None,
*,
after_request: Optional[AfterRequestHookHandler] = None,
after_response: Optional[AfterResponseHookHandler] = None,
background: Optional[Union[BackgroundTask, BackgroundTasks]] = None,
before_request: Optional[BeforeRequestHookHandler] = None,
cache: Union[bool, int] = False,
cache_key_builder: Optional[CacheKeyBuilder] = None,
etag: Optional[Union[str, Etag, Callable[[Response], Etag]]],
dependencies: Optional[Dict[str, Provide]] = None,
exception_handlers: Optional[ExceptionHandlersMap] = None,
guards: Optional[List[Guard]] = None,
http_method: Union[HttpMethod, Method, List[Union[HttpMethod, Method]]],
media_type: Union[MediaType, str] = MediaType.JSON,
middleware: Optional[List[Middleware]] = None,
name: Optional[str] = None,
opt: Optional[Dict[str, Any]] = None,
response_class: Optional[ResponseType] = None,
response_cookies: Optional[ResponseCookies] = None,
response_headers: Optional[ResponseHeadersMap] = None,
status_code: Optional[int] = None,
sync_to_thread: bool = False,
# OpenAPI related attributes
content_encoding: Optional[str] = None,
content_media_type: Optional[str] = None,
deprecated: bool = False,
description: Optional[str] = None,
include_in_schema: bool = True,
operation_id: Optional[str] = None,
raises: Optional[List[Type[HTTPException]]] = None,
response_description: Optional[str] = None,
responses: Optional[Dict[int, ResponseSpec]] = None,
security: Optional[List[SecurityRequirement]] = None,
summary: Optional[str] = None,
tags: Optional[List[str]] = None,
**kwargs: Any,
) -> None:
self.etag = etag
...
def resolve_response_handler(
self,
) -> Callable[[Any], Awaitable[StarletteResponse]]:
"""Resolves the response_handler function for the route handler.
This method is memoized so the computation occurs only once.
Returns:
Async Callable to handle an HTTP Request
"""
if self._resolved_response_handler is Empty:
after_request_handlers: List[AsyncCallable] = [
layer.after_request for layer in self.ownership_layers if layer.after_request # type: ignore[misc]
]
after_request = cast(
"Optional[AfterRequestHookHandler]",
after_request_handlers[-1] if after_request_handlers else None,
)
if self.etag:
after_request = partial(_etag_after_request_handler, etag=self.etag, original_handler=after_request)
media_type = self.media_type.value if isinstance(self.media_type, Enum) else self.media_type
response_class = self.resolve_response_class()
headers = self.resolve_response_headers()
cookies = self.resolve_response_cookies()
if is_class_and_subclass(self.signature.return_annotation, ResponseContainer): # type: ignore[misc]
handler = _create_response_container_handler(
after_request=after_request,
cookies=cookies,
headers=headers,
media_type=media_type,
status_code=self.status_code,
)
elif is_class_and_subclass(self.signature.return_annotation, Response):
handler = _create_response_handler(cookies=cookies, after_request=after_request)
elif is_class_and_subclass(self.signature.return_annotation, StarletteResponse):
handler = _create_starlette_response_handler(cookies=cookies, after_request=after_request)
else:
handler = _create_data_handler(
after_request=after_request,
background=self.background,
cookies=cookies,
headers=headers,
media_type=media_type,
response_class=response_class,
status_code=self.status_code,
)
self._resolved_response_handler = handler
return cast("Callable[[Any], Awaitable[StarletteResponse]]", self._resolved_response_handler) |
Beta Was this translation helpful? Give feedback.
-
|
this looks good. We don't use dataclasses as such, we use pydantic models for dataclasses - because the validation is better and more consistent across python versions. What about layering etags? That is, supporting something like this: from starlite import Controller, ETag, get
class MyController(Controller):
path = "/controller"
etag = Etag(...)
@get("/")
def my_handler() -> dict:
...That is, allows users to define etags on different layers of the application (app level, routers, controllers, and individual functions), and then using this ? You can see how this is handled for the other stuff like |
Beta Was this translation helpful? Give feedback.
-
|
Sure, shouldn't be to complicated to adopt it to a pydantic model. Hmm. I'm not sure about the use case of application and router level etags? Router and controller kinda make sense since they (can) represent individual resources which could share an etag. But I guess there's no reason to restrict the usage either? |
Beta Was this translation helpful? Give feedback.
-
|
It's primarily I'd say for consistency. We will also need to derive openapi headers from this, so that's another use case. Consider for example the |
Beta Was this translation helpful? Give feedback.
-
|
This is something we can probably do for |
Beta Was this translation helpful? Give feedback.
-
|
Actually implementing it in |
Beta Was this translation helpful? Give feedback.
-
|
Regarding the proposal to include |
Beta Was this translation helpful? Give feedback.
-
|
@Goldziher As I said earlier, I think as it currently stands, this is the best approach, so yes, I'd vote for this. |
Beta Was this translation helpful? Give feedback.
-
|
I'm glad I didn't close my existing PR 🙂. I'll make the changes in the header class so it'd be easier to reuse it for ETags |
Beta Was this translation helpful? Give feedback.
-
|
Ok, let's Boogie. @provinzkraut - you wanna add a or as well? |
Beta Was this translation helpful? Give feedback.
-
|
@Goldziher @seladb I guess it probably makes sense if we consolidate our implementation efforts into a single fork/branch? I have the |
Beta Was this translation helpful? Give feedback.
-
|
Sure I'll make the updates in my PR. This PR is still missing tests and documentation so it'll take me a day or two to get it ready |
Beta Was this translation helpful? Give feedback.
-
|
In #571 (comment) you discussed tying |
Beta Was this translation helpful? Give feedback.
-
Completely agree! That's part of what I was trying to address in #573 (comment) 🙃
Yes. Let's do some meta-thinking :P |
Beta Was this translation helpful? Give feedback.
-
|
Cache-Control headers are meant for browsers usually. It's not meant for internal use - but an instruction to the consumer that the response should be cached rather than a new request sent. Server side response caching has some overlap, but it's usually used for different purposes. E.g. you can cache based on specific parameters, or not. So, imo these are different and they should be different. |
Beta Was this translation helpful? Give feedback.
-
That's for To quote MDN
If it's used in a response its semantics are a bit different:
IMHO a client should not need to know where caching is implemented, since it can be done in many different layers, so if a client requests a response with a certain The response cache would be the last caching layer to be hit anyway (are there exceptions?), so it shouldn't mess with anything upstream. Maybe just disable this behaviour by default and make it available via a configuration option so it's up to the users to decide? |
Beta Was this translation helpful? Give feedback.
-
Ok |
Beta Was this translation helpful? Give feedback.
-
|
@provinzkraut I think this could be a nice addition, however as we discussed before we need to think about how to implement this in a way that will be clear for users and easy to config/disable |
Beta Was this translation helpful? Give feedback.
-
|
@provinzkraut @Goldziher the PR is ready for review: #601 |
Beta Was this translation helpful? Give feedback.
-
|
ETag support in headers was added with #661. The more involved features should be addressed in a separate PR. |
Beta Was this translation helpful? Give feedback.
-
Etags are a caching mechanism. You can read more about it in for example here: https://en.wikipedia.org/wiki/HTTP_ETag. They are primarily useful for static pages or for resources that can be hashed. We should explore how to implement and combine integrated etag generation into Starlite - and if it makes sense.
Beta Was this translation helpful? Give feedback.
All reactions