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

Document how to create ASGI middlewares #1656

Merged
merged 34 commits into from
Jun 30, 2022
Merged

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented May 31, 2022

This PR is incomplete. A text with some explanations on how to do this is missing.

I'm not sure if I need to provide an explanation or just link to those articles... Think about it.

image

@Kludex Kludex added the documentation Project documentation label May 31, 2022
@Kludex Kludex added this to the Version 0.21.0 milestone May 31, 2022
@florimondmanca
Copy link
Member

florimondmanca commented May 31, 2022

When it comes to writing middleware, I think one thing that those two articles (appreciate linking one of mine btw! 😊) don't mention or dive into is the pattern of wrapping receive/send to perform custom behavior on the request/response. E.g. this is how CORSMiddleware is implemented: wraps send() to add some response headers.

Another thing is a insisting on not storing per-request state on middleware -- middleware classes should be "stateless". Otherwise that state would conflict during concurrent requests. One pattern that solves this is a "Responder" helper ASGI app (not the only pattern -- could also do with a bunch of closures in __call__). This is how GZipMiddleware works: wraps send() in a GZipResponder that buffers the initial http.response.start until it knows if it can send gzip, as well as store some other gzip info. That's a pattern I've also used for msgpack-asgi (which combines that with extensive receive/send wrapping).

So, in terms of structure, we could do:

## Pure ASGI middleware

<!-- Introduction that mentions the benefits of working at the ASGI level?
Eg. ensuring compatibility with any framework... -->

### Guiding principles

<!-- Maybe some general ideas when writing ASGI middleware? -->
- Accept a parent `app` as first argument
- Implement `__call__(scope, receive, send)` that should call into parent `app` to run the underlying app or middleware.
- Handle all three `scope` types: `lifespan`, `http`, `websocket`
- Middleware classes should be stateless -- see [Per-request state](#per-request-state) if you do need to store per-request state.
- You may use Starlette datastructures (`Headers`, `URL`, etc) to ease dealing with raw ASGI components.

### Wrapping the request

<!-- Explains modifying the scope (for request headers, URLs, etc)
or wrapping receive() (for request body) -->

### Wrapping the response

<!-- Explains wrapping send() (response headers or body) -->

### Per-request state

<!-- Explains the Responder pattern -->

### Storing context in `scope`

<!-- Something about storing stuff in `scope` for access in endpoints?
See AuthMiddleware, SessionMiddleware... -->

### Examples

<!-- Perhaps a list of 'best practice' implementations?
Or a full complete example that does all the above? -->

@Kludex
Copy link
Member Author

Kludex commented Jun 3, 2022

I still need to polish it, but reviews are already welcome.

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great :) I love that we'll have a guide on writing pure ASGI middleware, as this style enables more creative / detailed functionality as well as interoperability across ASGI frameworks.

One general thought -- how does this look in the docs? It seems to get pretty long. For good reason, since ASGI middleware is a fairly rich topic in terms of techniques to peek into ASGI data or wrap behavior... But since this topic is by definition not Starlette-specific (apart from the section mentioning reusing Starlette components), it does feel a bit odd having such a large section in a page that otherwise focuses on Starlette-specific middleware matters.

Not sure how we'd resolve this, though...

docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
Co-authored-by: Florimond Manca <florimond.manca@protonmail.com>
Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! Thanks for doing this. 🤩

Just a couple of small suggestions.

docs/middleware.md Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Show resolved Hide resolved
docs/middleware.md Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
Copy link
Member Author

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR.

  • I've removed the "Examples" section, but I'd like your opinion about this. Should I link code sources around that implement different styles of middlewares? Or should I create examples based on the scope types?
  • I still didn't finish the "Storing context in scope" section. I've created an issue on asgiref to have clarifications: Specifications about state storage in the scope django/asgiref#332
  • I've applied all the comments.

Another round of reviews is welcome. 🙏

@Kludex Kludex marked this pull request as ready for review June 8, 2022 05:41
@Kludex Kludex requested a review from a team June 12, 2022 07:16
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this documentation is getting narrowed down! Nice work

docs/middleware.md Outdated Show resolved Hide resolved
Kludex and others added 2 commits June 28, 2022 07:33
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
Copy link
Member Author

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to include an example on how to manipulate the request body on a middleware.

docs/middleware.md Outdated Show resolved Hide resolved
Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is great! 🚀

I just have a couple of minor comments.

docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com>
docs/middleware.md Outdated Show resolved Hide resolved
Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com>
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
Copy link
Member Author

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done my last self review.

I think documentation is not appreciated as it should, so thanks for all the comments and rounds of reviews. 🎉

I'd like to add some points for some decisions or things that I didn't apply that I think we can improve in the future:

  • There are no examples of wrapping the receive method (some example modifying the request body would be cool).
  • The "Per-request state" section is too complex. I think we could add an example of "what not to do" and then following with the "right way", as mentioned by @euri10.
  • I think not adding further patterns (e.g. generator way) on the "Per-request section" was the best approach. I think that pattern is neat, yep, but it's also hard to understand, and not everybody has the same knowledge, so we need to try to give the information in a way that everybody can understand by only reading this documentation (or reference external content that makes reading this possible). That said, I do think that it's worth adding a note to showcase some advanced patterns.

In any case, once again, thanks everybody. 🙏

@Kludex Kludex merged commit 5201065 into master Jun 30, 2022
@Kludex Kludex deleted the document-asgi-middleware branch June 30, 2022 18:38
@tiangolo
Copy link
Member

Thanks for all the work and patience with this @Kludex ! Great job! 😎🚀

And awesome to see all the additional feedback from everybody 😍🍰

@adriangb
Copy link
Member

This is awesome @Kludex ! Thank you everyone for your work on this. Agreed about the not including the generator stuff, too complicated for intro docs 😅

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

Successfully merging this pull request may close these issues.

Improve Documentation on Writing Custom ASGI Middleware and BaseHTTPMiddleware
6 participants