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

Enhancement: fully support builder pattern for apps #3257

Closed
bunny-therapist opened this issue Mar 26, 2024 · 8 comments
Closed

Enhancement: fully support builder pattern for apps #3257

bunny-therapist opened this issue Mar 26, 2024 · 8 comments
Labels
Enhancement This is a new feature or request

Comments

@bunny-therapist
Copy link

bunny-therapist commented Mar 26, 2024

Summary

When you create an app (instantiate litestar.Litestar) you can pass in everything - route handlers, middleware, response headers, etc to the __init__ - and you get a fully functional app. Some of the properties of the app can be modified after instantiation, e.g.,

  • litestar.Litestar.register is a method that exists solely for adding new route handlers.
  • litestar.Litestar.response_headers is just a public-attribute list and can thus be modified (not sure if this is intentional, the typing makes things a bit awkward), and the modifications affect the app.
  • litestar.Litestar.cache_control is - I think - similar to the response headers.

However, modifying litestar.Litestar.middleware does not affect the routes of the app (since they have already been "resolved"), so even though this is a public-attribute list, modifying it does not work as one might expect.

The existence of litestar.Litestar.register seems to imply the intention of supporting a builder pattern, where the app can be constructed one piece at a time, adding route handlers, middleware, etc. However, given that (at least) middleware does not affect already resolved routes, the builder pattern is not really supported. Even if you add more middleware to the app instance, they will not take effect.

I do not know what the best interface would be, but I would like litestar to support modification of all attributes of the app post-initialization so that apps can be built piece by piece.

See discussion at https://github.com/orgs/litestar-org/discussions/3244

Basic Example

import litestar
from some_module import MyMiddleware

@litestar.get("/data")
async def get_data() -> dict:
    return {"data": "1234"}

@litestar.get("/hello")
async def get_hello_world() -> str:
    return "hello world"

app = litestar.Litestar(route_handlers=[get_data])
app.middleware.append(MyMiddleware)  # or something like this
app.register(get_hello_world)

# Resulting app uses MyMiddleware for both endpoints

Drawbacks and Impact

The suggested changes should not affect any backwards compatibility or have any significant drawbacks, unless someone, somewhere is, e.g., creating app with route handlers, then adding middleware, then registering a new endpoint, and expecting only the new endpoint to use the middleware - and this behavior changes.
If the feature is implemented as suggested in the discussion - with a litestar.Litestar.recreate method, then there should be no concerns at all, since it is just adding a new method and everything else works the same.

Unresolved questions

What is the intended way users would use the builder pattern?

Are attributes like litestar.Litestar.response_headers public and mutable intentionally, or just a consequence of inheritance? At a minimum, the behavior of modifying apps post-initialization must be properly documented.


Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Fund with Polar
@bunny-therapist bunny-therapist added the Enhancement This is a new feature or request label Mar 26, 2024
@provinzkraut
Copy link
Member

provinzkraut commented Mar 29, 2024

What problem would this solve that an InitPlugin does not?

At the moment I'm not convinced that this is a good feature to add. We are actively trying to get away from the mutable app instances, and making this work while taking care of all the edge cases is going to be quite a significant amount of work which I think will cause more trouble down the road than it's worth. I also think it's important that we keep things as simple as possible and don't offer too many ways to do similar things. Right now, we already have a pattern in place that allows to fully customise application creation and configuration, and that is plugins.

@bunny-therapist If you have a real world example of where plugins cannot solve the issue in a decent way, I think we should first try to see how we can make this solvable with plugins.

@litestar-org/maintainers

@bunny-therapist
Copy link
Author

Can an InitPlugin add middleware that affects all the route handlers of the app, including the ones that were passed as route_handlers to litestar.Litestar.__init__?

@provinzkraut
Copy link
Member

Yes. InitPlugins are able to fully customise everything that is passed to the app; The effect of modifying the AppConfig is always the same as if the altered configuration value was passed to the Litestar instance directly. They are pretty much the first thing that's being called, before any other initialisation happens.

@bunny-therapist
Copy link
Author

Thanks. I will try that on Tuesday and then get back to you.

If the intention is for litestar.Litestar instances to be constructed once and then not modified, then I suggest that at a minimum the register method is removed, since it implies that apps can be constructed piece by piece (even described in docs) I would also consider making some public attributes of litestar.Litestar like route_handlers and response_headers etc private/immutable. Right now, apps have a method for registering new route handlers and several mutable public attributes and it all hints to a pattern that you say is to be discouraged. It all becomes a bunch of "gotchas". I thought I could change things and build apps dynamically, but when I tried it some things worked and other things didn't. I asked about this and that is what lead to this ticket.

@provinzkraut
Copy link
Member

Fully agree with you there, and in 3.0 things are probably actually going to be a lot more immutable :)

@bunny-therapist
Copy link
Author

Ok, an InitPlugin worked like a charm. With the caveat that modifying response_headers is kind of horrible because it is typed with a union (and I don't think the conversion function is part of the public API?) of Sequence and Mapping.
Thanks!

@provinzkraut
Copy link
Member

@bunny-therapist I'm going to close this one then, feel free to open up a new one for the issues you're having with the response_headers.

@provinzkraut provinzkraut closed this as not planned Won't fix, can't repro, duplicate, stale Apr 2, 2024
@JacobCoffee
Copy link
Member

@bunny-therapist Do you have this public by chance? :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This is a new feature or request
Projects
None yet
Development

No branches or pull requests

3 participants