-
-
Notifications
You must be signed in to change notification settings - Fork 946
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
Endpoint class decorators #734
Comments
Looks like what you're seeing is caused by this block in if inspect.isfunction(endpoint) or inspect.ismethod(endpoint):
# Endpoint is function or method. Treat it as `func(request) -> response`.
self.app = request_response(endpoint)
if methods is None:
methods = ["GET"]
else:
# Endpoint is a class. Treat it as ASGI.
self.app = endpoint Basically, you seem to expect that your class decorator will get wrapped with So it seems class decorators are currently unsupported. |
Thanks @ianhoffman that's entirely correct. One way we could resolve this would be if we had something more explicit in the routing, rather that "guessing" if the callable is a request/response callable or an ASGI app. Eg. |
@tomchristie That seems reasonable. Are there worries about backward compatibility? IMO it's strange that routes can even accept an ASGI app as an argument. Doesn't an app have many routes, which each map to a given function (or view, in Django parlance)? It's cool that ASGI supports nested ASGI apps — I guess the idea is to simplify composing middleware? I'm just curious if passing an app to a route is even something which Starlette needs to support. ASGI apps also seem like a lower-level interface which the average developer using Starlette shouldn't need to care about. I may be rambling here, though. If you do want to adopt |
@ianhoffman An ASGI app doesn't have to support routing in itself — e.g. AFAIU the purpose of the ASGI part of I think removing the guess from @azmeuk ^ Just tried it out myself, it won't work: Code (click to expand)import functools
import inspect
class decorator:
def __init__(self, func):
functools.update_wrapper(self, func)
self.func = func
def __call__(self, *args, **kwargs):
return self.func(*args, **kwargs)
@decorator
def add(x, y):
return x + y
inspect.isfunction(add) # False |
@florimondmanca this is a bigger change, but couldn't Obviously that might break a lot of existing contracts, so I get it if it's impractical. But it does seem like a cleaner API. Any reasons |
@ianhoffman Now that I think of it, my usual workaround for endpoint decorators in Starlette is just to switch to So, in your example: @foobar
class MyEndpoint(HTTPEndpoint):
async def get(self, request):
return JSONResponse({"response": "my_endpoint"}) This is a constraint I had to impose to the API of from asgi_caches.decorators import cached
from starlette.endpoints import HTTPEndpoint
@cached(cache)
class UserDetail(HTTPEndpoint):
async def get(self, request):
... |
Gotcha — using a CBV instead of a functional view does seem like a reasonable fix. That's probably good enough given this isn't likely to be a common issue. |
Right. And actually thinking about it, we don’t need to be doing that. The request/response function to Route needs to accept a request and return a response. I’d assumed we can do that on CBV’s because eg init just returns the class instance. (So eg in Django there’s a My solution had been “okay, let’s have Route accept either a request/response function, or an ASGI app. Then CBVs can be implemented as mini ASGI apps) Happily, in our case I think instantiating the CBV can be the request/response function, because “response” here just means, an ASGI callable. So... if the CBV init method accepts a request, and if call implements the ASGI interface and handles dispatch, then the classes are request/response callables. We can then drop the “Route can also be an ASGI app” case, because we don’t really need it. Mount is appropriate for pointing at apps, Route is appropriate for pointing as req/response callables. |
One question, then — will Starlette still support decorating endpoints at the ASGI level? The fact that It's a crucial fact, but of course it doesn't mean it's the only way to do things. So, lemme try to see if this would work… Previously, an ASGI-level CBV decorator could basically be a proxy to an ASGI middleware, so… def decorate(endpoint: ASGIApp) -> ASGIApp:
return SomeASGIMiddleware(endpoint) From @tomchristie's description, I understand the proposed change would be to make CBVs look something like this… class HTTPEndpoint:
def __init__(self, request: Request):
self.request = request
def __await__(self) -> Iterator[ASGIApp]:
handler = getattr(self, self.request.method)
return handler(self.request).__await__()
# Example user code
async def get(self, request) -> Response:
return PlainTextResponse("...") This way But when previously an ASGIView = Callable[[...], Awaitable[ASGIApp]] Decorators would end up looking like: def decorate(view: ASGIView) -> ASGIView:
async def wrapper(*args: Any, **kwargs: Any) -> ASGIApp:
app = await view(*args, **kwargs) # e.g. a Starlette 'Response' instance...
return SomeASGIMiddleware(app)
return wrapper It's a bit more involved, but the good thing is that this would work with both class- and function-based views (because they'd now have the same signature by design). (I don't think this would work out of the box with any view interface though. Eg. FastAPI's view interface doesn't return an ASGI app instance, so ASGI decorators in the form of the above won't work in that case. But that's a different problem — FastAPI doesn't provide a way to decorate views at the ASGI level today anyway.) So, uh, to answer my own question…
Yes (but differently). |
Actually it’s more complicated then that, since the request/response function needs to be awaitable. I think we can still do this, but needs a bit more investigation. Failing that we could always do something like Django’s as_view |
I believe the solution would be to implement |
Ah yeah absolutely. Sorry, I’d see the issue, and had already been meaning to respond, didn’t see your comment getting there in the meantime! |
Hi, same problem using dependency injection: from starlette import applications
from starlette import responses
from starlette import routing
class Ping:
def __init__(self, db):
self._db = db
async def __call__(self, request):
...
return responses.PlainTextResponse("fails")
db = ...
myview = Ping(db)
app = applications.Starlette(routes=[routing.Route("/", myview, methods=["GET"])]) Raises:
The expected behavior is that the method is called like a function. The workaround using HTTPEndpoint complicates the implementation without gains and makes even more complex to build handler factories. |
Just to check, currently the only way to implement endpoint decorators is by rewriting them as a CBV? On that point, is there recommended best practice around using a function endpoint or a class endpoint? Surprised there isn't an opinion in the docs. Currently, it seems like for simple endpoints the FBV is equivalent but requires less code so would be best practice? But then would be strange to have a few endpoints as CBVs just to implement a decorator and the rest as FBVs? Just thinking out loud around this topic. For the record I'm trying to implement user scopes, with certain endpoints requiring certain scopes to be found under |
Is this still wanted? 🤔 If no one replies to this, I'll close it as won't do in a month. 🙏 |
Back in 2019 when my company considered to move to starlette, this was a blocker issue. I think they can leave without this feature as they moved to other frameworks. |
It's still relevant to me, I used a nasty workaround in several projects. |
I don't think the route class needs a new argument. The decorator itself has to manage request. If we add new "app=" argument then it will intersect with Mount type. |
@Kludex @florimondmanca @tomchristie Hi, I went through this thread of discussion. and the points highlighted by @florimondmanca and @tomchristie . If this issue needs work. I would be happy to contribute. |
I think this will always be a limitation. 🤔 Any reasonable solution that is not a breaking change here? |
I need to implement a decorator for an endpoint, and for some technical reasons, I would like to implement it with a class. I also would like my decorator to not need parenthesis when called. (i.e. prefer
@my_decorator
over@my_decorator()
).Here is a simple implementation:
But accessing to
/my_endpoint
produces this traceback:I am not really sure what is going wrong here. I also did not find documentation about endpoint decorators with starlette.
Do I make mistakes when defining my decorator or is it something not supported by starlette?
PS: I used starlette 0.13 and python 3.8
Important
The text was updated successfully, but these errors were encountered: