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

Proposal: Custom Request Classes using type annotations #930

Closed
wants to merge 2 commits into from

Conversation

erewok
Copy link
Contributor

@erewok erewok commented May 4, 2020

This PR is a proof-of-concept exploring support for custom request classes in Starlette (related issues: #715, #388). I wanted to open this PR as a discussion point: there are certainly other ways to implement custom request classes in Starlette.

To use a custom request class under the changes here, you can simply provide a custom request class type annotation for your request handler.

Here's an example:

class CustomRequest(Request):
    @property
    def custom(self):
        return "Something Custom"


app = Starlette()


@app.route("/regular")
async def reqular_req(request):
    return PlainTextResponse(request.__class__.__name__)


@app.route("/custom")
async def custom(request: CustomRequest):
    return PlainTextResponse(f"{request.__class__.__name__}: {request.custom}")

Results can be seen here:

In [1]: import requests                                                                        

In [2]: requests.get("http://localhost:8000/regular").content                                  
Out[2]: b'Request'

In [3]: requests.get("http://localhost:8000/custom").content                                   
Out[3]: b'CustomRequest: Something Custom'

Changes

  • Use request arg type annotation to instantiate custom request classes.
  • Add tests for the above.

Notes

  • The module starlette.middleware.errors has not been changed.
  • No documentation changes have been made here; this PR is for gathering feedback.
  • This code will throw a TypeError if there is a type annotation for the request arg and that annotation is not a subclass of starlette.requests.Request.

Discussion

I am unsure how people will respond to this proposal. I had a couple of other ideas, but this one seemed ergonomic for end users because it's a small code change, and allows adding any custom request class for any particular endpoint where one is desired. I have tried to break down some of the pros and cons below.

Pros

  • Small code change for users: add a type annotation where a custom request class is desired.
  • Allows custom-request classes per handler instead of globally.
  • Makes explicit the request class being used for a particular handler.
  • Does not affect existing users or anyone who doesn't care about having custom request classes (this seems like a requirement for any solution to this problem).

Cons

  • Potentially surprising and unpythonic: runtime behavior changes based on a type annotation (although, to be fair, other libraries are doing things like this now: Pydantic and ecological come to mind).
  • Looks for annotation specifically for the arg named "request", which means this wouldn't work: def a_handler(req: CustomRequest): because arg-name is not "request". (We could alternately look at first arg and pull annotation for it)
  • Doesn't handle the bug where someone types def a_handler(req: List[str]) or something that's not callable; another way to say this is that it takes advantage of the fact that the type annotation is a class definition.
  • Does not globally change requests: some users may wish to have a single place to swap the Request class for all handlers (this can be solved in other ways, I think).

@erewok erewok changed the title Proposal Custom Request Classes using type annotations Proposal: Custom Request Classes using type annotations May 4, 2020
@erewok erewok requested a review from tomchristie May 4, 2020 19:19
@aviramha
Copy link
Contributor

aviramha commented May 4, 2020

I like this approach but I am a FastAPI user, and I wonder how this could be interfaced there. I assume I would want specific mounts to be handled with my custom request, and not sure if typing is a convinent way.

@tomchristie
Copy link
Member

Thanks. I don't think this fits with the general approach in Starlette, and introspecting the annotated types is a bit brittle. (Eg. it can break if using generators.)

It's feasible that we could support customising the request class, but I think it'd make sense to talk over the design first if so.

@erewok
Copy link
Contributor Author

erewok commented May 5, 2020

Sounds good to me. It was easier to show the idea than describe it in this case, and I intended this for discussion, so I don't mind closing.

@erewok erewok closed this May 5, 2020
@tomchristie tomchristie deleted the poc_custom_request_di branch May 5, 2020 13:03
@erewok
Copy link
Contributor Author

erewok commented May 5, 2020

After experimenting with this, I started to think that offering custom request classes for any arbitrary handler is a powerful and uncommon feature that users are likely to take advantage of. Further, I suspect that changing the request object would be the first obvious thing for users to do, especially when they are unfamiliar with the inner workings of Starlette and they want something other than default behavior. In the discussion about modifying the FormParser class, for instance, someone mentions that customizing there feels too low-level, and I tend to sympathize with that.

As the framework is already using dependency injection for the request object, it seems like a natural place for users to specify what to inject.

But I was also thinking about making an argument that the Route or Mount constructors could take to achieve the same thing. That answer would likely be more explicit, less magical, more pythonic than this example and it would be easy to apply globally or for particular routes.

@aviramha
Copy link
Contributor

aviramha commented May 5, 2020

I agree with you @erewok but I think the comment belongs to the issue #715 so it wont be missed out!

@tomchristie
Copy link
Member

As the framework is already using dependency injection for the request object, it seems like a natural place for users to specify what to inject.

We're not using dependency injection for the request instance. I think the only place we're introspecting function signatures is the @requires decorator, and that's something that I'd like to eventually see deprecated in favour of specifying required scopes in the Routes themselves.

@erewok
Copy link
Contributor Author

erewok commented May 5, 2020

I see. If that's the case, then customizing the request object, if it's something that is desirable to do, most likely makes sense in the Routes as well, I think. Alternately, you could also make it so users could specify tooling there instead of swapping the whole request class. For instance, your request-parsing customizability suggestion could go there? It's also true that allowing people to customize the request may offer so much freedom and extensibility that people would use it for things that are better solved elsewhere, such as authentication or data validation and then their implementations could end up too strictly wedded to a particular implementation of the Request class in Starlette.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants