litestar-org / litestar

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs
https://litestar.dev/
MIT License
5.52k stars 378 forks source link

Enhancement: SQLAdmin Support #3396

Closed colebaileygit closed 6 months ago

colebaileygit commented 6 months ago

Summary

SQLAdmin has a fork for litestar, but it isn't properly supported and there are no issues or discussions around its limitations. Is there support for making this better integrated in the litestar community?

Basic Example

When using both litestar sqlalchemy plugin and the sqladmin fork https://github.com/cemrehancavdar/sqladmin-litestar, I get errors about pickling a psycopg2 module or AttributeError: Can't pickle local object 'create_engine.<locals>.connect'

This seems to be due to the fact that litestar pickles each router mounted to the app, and the fact that sqladmin relies on passing the sql engine or session maker around which ends up getting pickled. If I want to implement a fix for this I don't know the right place to look. The examples in the fork only show sqlite and I don't see any similar discussions so I suppose this is not fully tested behavior.

Drawbacks and Impact

Unfortunately sqladmin itself does not seem to be open to being framework agnostic, so any fork suffers the problems of forking and future updates.

Unresolved questions

No response


[!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

peterschutt commented 6 months ago

When using both litestar sqlalchemy plugin and the sqladmin

I don't think this is related to the sqlalchemy plugin, I find the error occurs with the basic example from the sqladmin-litestar project repo, which doesn't invoke any plugin.

This seems to be due to the fact that litestar pickles each router mounted to the app

Yeah, its a deepcopy of the router. There is ongoing work that will remove this requirement, but that is the case for now and it shouldn't be an issue anyway.

the fact that sqladmin relies on passing the sql engine or session maker around which ends up getting pickled

Yep, this is the issue, sqlalchemy engines cannot be deepcopied, yet as the handlers that are added to the router in the Admin class are instance methods of the Admin class, and that class has a self reference to the engine, we arrive at this point.

If I want to implement a fix for this I don't know the right place to look.

I think that instead of trying to shoehorn the existing sqladmin approach, it would be better to do this the "litestar" way from the start. That is, instead of wrapping the application object with the Admin object, it should be a plugin that can register dependencies, routers etc on the application.

See: https://github.com/peterschutt/sqladmin-litestar/tree/litestar-refactor And: https://github.com/peterschutt/litestar-sqla-admin-test

It seems to work, but I didn't test it any more than to create a model or two and delete them. There's probably more we'd do to better integrate it with the SQLAlchemyPlugin if we were going to go full throttle with it.

proton32 commented 6 months ago

Thanks for litestar-refactor, great work! But it has issue: async def list conflicts with builtin list function in application.py

peterschutt commented 6 months ago

Thanks for litestar-refactor, great work! But it has issue: async def list conflicts with builtin list function in application.py

You can make a PR over there if you like, or I'll hopefully find time to take a look tomorrow.

I'm planning to experiment with a different approach for this, and instead of forking sqladmin, see if we can get away with building a plugin that depends on it instead. Theory is that we'd still get benefit of upstream maintenance then, but I don't know how feasible it is just yet.

colebaileygit commented 6 months ago

Great work, I finally got back to this and ran into the same issue and also created a small fork for it 😄

peterschutt commented 6 months ago

I've just published https://github.com/peterschutt/sqladmin-litestar-plugin and https://pypi.org/project/sqladmin-litestar-plugin/.

It is not a fork of SQLAdmin, instead it depends on it which means the maintenance burden should be much lower, and we can more easily participate in upstream improvements. It does this by mounting a starlette app that is configured by SQLAdmin on the Litestar application.