piccolo-orm / piccolo_admin

A powerful web admin for your database.
https://piccolo-orm.com/ecosystem/
MIT License
322 stars 39 forks source link

Try `piccolo_admin` with Starlite #211

Closed dantownsend closed 2 years ago

dantownsend commented 2 years ago

It was reported that Piccolo Admin doesn't work with the Starlite framework.

It should be possible to mount Piccolo Admin:

https://starlite-api.github.io/starlite/usage/2-route-handlers/3-asgi-route-handlers/

Tasks required:

sinisaos commented 2 years ago

@dantownsend I already tried that because I wanted to make a Starlite asgi template for Piccolo ORM but I didn't succeed. I hope you know better how it could be done.

dantownsend commented 2 years ago

@sinisaos Ah, cool! Thanks for giving it a go.

sinisaos commented 2 years ago

@dantownsend I tried mounting Piccolo Admin on the Starlite app again, but again no luck. I think it would be useful as Starlite supports Piccolo ORM via a plugin (or we can use controlers and both ways work) and it would be great if Piccolo Admin could be used as an important part of the Piccolo ecosystem. I tried two things with the asgi decorator. First I tried changing the scope path as advised by @peterschutt here.

admin_app = create_admin(tables=[Task])

@asgi(path="/admin/")
async def admin(scope: Scope, receive: Receive, send: Send) -> None:
    scope["path"] = "/"
    await admin_app(scope=scope, receive=receive, send=send)

app = Starlite(route_handlers=[admin])

The admin app appears to be mounted, but all dist files and folders return a 404 and none of the admin api endpoints work.

starlite_admin

After that I tried using the Starlette router to omit the scope changes and it works, but the result is the same.

from starlette.routing import Router, Mount

admin_app = create_admin(tables=[Task])

@asgi(path="/admin/")
async def admin(scope: Scope, receive: Receive, send: Send) -> None:
    router = Router(
        [
            Mount(
                path="/admin/",
                app=admin_app,
            ),
        ]
    )
    await router(scope=scope, receive=receive, send=send)

app = Starlite(route_handlers=[admin, home])

I also tried using 2 routers but the result is always the same. The admin app appears to be mounted, but nothing from admin works. I don't know if the problem is on our side because admin uses FastAPI (or Starlite different router architecture), but that doesn't make sense because both Piccolo Admin or FastAPI app are asgi apps and should work. I appreciate any help.

peterschutt commented 2 years ago

First I tried changing the scope path as advised by @peterschutt here.

In that case OP was trying to mount another Starlite application instance that didn't have routes registered under the url prefix that the app was being mounted under.

Not sure what the issue is here but I'll try to have a look at this one today.

sinisaos commented 2 years ago

@peterschutt Thank you very much for your reply. If you find time, it would be great if you try to integrate Piccolo Admin and Starlite. For now, Piccolo Admin is working with Starlette, FastAPI, BlackSheep and Xpresso, but all these frameworks have the ability to mount another asgi app on the application instance, which is not the case in Starlite. The only way seems to be the one I tried, but unfortunately it doesn't work as we would like.

dantownsend commented 2 years ago

@sinisaos Do you have a demo Starlite project on GitHub? If so I'll download it and have a go.

peterschutt commented 2 years ago

@dantownsend this is what I've been experimenting with: https://github.com/peterschutt/starlite-hello-world/tree/piccolo-admin

There's some instruction in app/main.py to get it running.

Running the starlite app, it will serve the root page, but all the static links (and probably api links) that come from the static apps and private/public apps that are mounted inside the admin app would need to have the /admin prefix to get routed back through the @asgi() decorated handler via starlite routing again. If we get it to this point, we'll need to add an /admin/{path:path} route to field the child routes as well.

image

E.g., in that example the js/... routes don't exist as far as starlite's router is concerned.

I was going to try to experiment with passing root_path to the fastapi app constructor via AdminRouter but hit that issue with the editable installs.

sinisaos commented 2 years ago

@dantownsend No. I just changed the Starlette template to work with Starlite. You can download the zip file and pip install Starlite (forgot to add in requirements.txt). Use it like any Piccolo asgi template (running migrations etc.).

tasks.zip

dantownsend commented 2 years ago

I've looked into it - comparing how Starlite works vs Starlette.

I mounted Piccolo Admin in each, and looked at the ASGI scope.

The root_path and path values are opposite.

Here's Starlite:

starlite_scope = {
    'type': 'http',
    'asgi': {
        'version': '3.0',
        'spec_version': '2.3'
    },
    'http_version': '1.1',
    'server': ('127.0.0.1', 8000),
    'client': ('127.0.0.1', 65236),
    'scheme': 'http',
    'method': 'GET',
    'root_path': '',      # NOTE
    'path': '/admin/',   # NOTE
    'raw_path': b'/admin/',
    'query_string': b'',
    ...
}

Here's Starlette:

starlette_scope = {
    'type': 'http',
    'asgi': {
        'version': '3.0',
        'spec_version': '2.3'
    },
    'http_version': '1.1',
    'server': ('127.0.0.1', 8000),
    'client': ('127.0.0.1', 64073),
    'scheme': 'http',
    'method': 'GET',
    'root_path': '/admin',     # NOTE
    'path': '/',                         # NOTE
    'raw_path': b'/admin/',
    'query_string': b'',
    ...
}

I used a setup similar to @sinisaos for the testing:

admin_app = create_admin(tables=[Task])

@asgi(path="/admin/")
async def admin(scope: Scope, receive: Receive, send: Send) -> None:
    await admin_app(scope=scope, receive=receive, send=send)

I don't think Starlite is forwarding all sub paths on /admin/. For example, /admin/api isn't getting sent to Piccolo Admin. I tried doing things like @asgi(path="/admin/*"), but no luck.

The best docs on how mounting works with ASGI is BlackSheep:

https://www.neoteroi.dev/blacksheep/mounting/

sinisaos commented 2 years ago

Somehow I managed to integrate Starlite and Piccolo Admin.

https://user-images.githubusercontent.com/30960668/190585807-73195d6c-0ae1-4049-96f1-f6a194a66481.mp4

First I used the Starlette Mount

admin_app = create_admin(tables=[Task])

@asgi(path="/admin")
async def admin(scope: Scope, receive: Receive, send: Send) -> None:
    router = Router(
        [
            Mount(
                path="/admin",
                app=admin_app,
            ),
        ]
    )
    await router(scope=scope, receive=receive, send=send)

After that I changed this lines of code in Starlite asgi.py https://github.com/starlite-api/starlite/blob/a3056724a7c4ef1d83dcdf60fd4498d34c5f62f8/starlite/asgi.py#L168-L169 to this

if path != "/":
    path = f"/{scope['path'].split('/')[1]}"

to strip scope path from e.g /admin/api/user to only /admin. Of course there is a problem because that line of code strip other paths that are not the root path / e.g tasks/1 does not show only 1 record but all (at the end of video).

It just a start and it's not great but that shows that Piccolo Admin can be mount to Starlite.

dantownsend commented 2 years ago

@sinisaos Hmm, interesting.

sinisaos commented 2 years ago

It's not an elegant solution, but maybe as @peterschutt a Starlite maintainer knows better how to fix the problem. Basically, in Starlite we should detect when an admin application is mounted and strip only that scope path, leaving other paths (e.g. tasks/1) untouched. Or something else, feel free to suggest what else could be done. Thanks in advance.

peterschutt commented 2 years ago

Great stuff @sinisaos - I'll have a closer look when I get a chance and work out a plan of attack. I think the key thing that the starlette router is doing in your mvce is that it manages stripping the mount prefix from scope["path"] and setting it to scope["root_path"] before forwarding on to the underlying app. I believe the necessity to strip the remaining path after the route prefix is due to the app being mounted on @asgi("/admin") which doesn't match any sub-routes of that path. So would also prob need a mount at @asgi("/admin/{path:path}") - but that's obviously not what we'd want to force users to do.

I think we can prob support a mount() method on Starlite, and before throwing a 404 if the path drops through our routing, check if the prefix matches a mounted path, and then pipe the request through the mounted app. However, take this all with a grain of salt until I get a chance to play around with some implementations, devil is always in the detail:)

sinisaos commented 2 years ago

@peterschutt Thanks. It would be great if you try to find a better solution (the important thing is that now we have at least some solution that shows that Piccolo Admin can be mounted on the Starlite app). mount() would be the best solution if it suits the Starlite router implementation and the community. BlackSheep mount() implementation would be the best example (here and here and here is Piccolo admin usage example). If we end up with a good solution, we could make a Starlite asgi starter template using the crud controller (same as for Starlette, BlackSheep, etc.) and I think that would also benefit Starlite (which already supports Piccolo ORM through a plugin) because Piccolo Admin is an important part of Piccolo ecosystem.

sinisaos commented 2 years ago

@peterschutt I have some update on how we can use Piccolo Admin mount a little better if mount() is not an option or it's difficult to implement, without too much intervention in the Starlite code. Route handlers have a name argument where we can pass e.g. Piccolo Admin (or any other name you think is fine). We can then use that name so Starlite can recognize it as a handler for Piccolo Admin without affecting any other routes (every other route with or without path or query params work as expected). To do this we only need to add these few lines of code, which I don't think will affect coverage even if you don't have test for that. Changes to asgi.py

def _parse_scope_to_route(
        self, scope: "Scope"
    ) -> Tuple[Dict[str, "ASGIApp"], bool]:
        """Given a scope object, retrieve the _asgi_handlers and _is_asgi
        values from correct trie node."""

        path = cast("str", scope["path"]).strip()
        # used only for mounted Piccolo Admin app
        try:
            piccolo_admin_path = self.app._route_handler_index["Piccolo Admin"]["path"] # pylint: disable=W0212
            if piccolo_admin_path in path:
                path = piccolo_admin_path
        except KeyError:
            pass
        # the rest of the code is unchanged
        if path != "/" and path.endswith("/"):
            path = path.rstrip("/")
        if path in self.app.plain_routes:
            current_node: RouteMapNode = self.app.route_map[path]
            path_params: List[str] = []
        else:
            current_node, path_params = self._traverse_route_map(
                path=path, scope=scope
            )
         ...

Usage with changes in app.py

# mount Piccolo Admin app
@asgi(path="/admin", name="Piccolo Admin")
async def admin(scope: Scope, receive: Receive, send: Send) -> None:
    router = Router(
        [
            Mount(
                path="/admin",
                app=create_admin(tables=[Task]),
            ),
        ]
    )
    await router(scope=scope, receive=receive, send=send)
sinisaos commented 2 years ago

@peterschutt Wildcard URL matching did the trick to mount Piccolo Admin in the Starlite app without modifying the Starlite source code. Mystery solved.

from starlette.routing import Mount, Router

# mount Piccolo Admin app
@asgi(path="/{admin:path}") # <- Wildcard URL matching
async def admin(scope: Scope, receive: Receive, send: Send) -> None:
    router = Router(
        [
            Mount(
                path="/admin",
                app=create_admin(tables=[Task]),
            ),
        ]
    )
    await router(scope=scope, receive=receive, send=send)

@dantownsend In the next few days I will do a PR to Piccolo ORM to add the Starlite asgi template.

peterschutt commented 2 years ago

Great stuff, @sinisaos.

I still think Starlite should have a better way to mount an asgi app such as create_admin without all the extra stuff you've had to do here such as using starlette's Mount and Router. I just haven't found the time to dedicate to it yet.

sinisaos commented 2 years ago

@peterschutt Thanks for your effort and help. I will create a Starlite asgi template in case someone wants to use Starlite with the full Piccolo ecosystem (ORM + admin) now. If you come up with a better way to mount admin, we can always make changes.

sinisaos commented 2 years ago

@peterschutt Here is the Starlite asgi starter template. Thanks again for your help. If you come up with a better mounting solution, feel free to make PR to change it. @dantownsend You can close this issue as it is done.

peterschutt commented 2 years ago

Thanks for helping to grow the Starlite community @sinisaos!

sinisaos commented 2 years ago

@dantownsend You can close this because it's done.

Goldziher commented 2 years ago

Just an update - starlite v1.35.0 had proper support for mounts now. The @asgi decorator receives an optional kwarg is_mount that enables this.

dantownsend commented 2 years ago

@Goldziher Great, thanks for the update.

We'll update Piccolo's Starlite template to use this new feature in a future version.

sinisaos commented 2 years ago

@dantownsend I tried to use this new feature but failed to mount admin. I tried

@asgi("/admin", is_mount=True)
async def admin(scope: "Scope", receive: "Receive", send: "Send") -> None:
    await create_admin(tables=APP_CONFIG.table_classes)(scope, receive, send)

and

admin = asgi(path="/admin", is_mount=True)(
    create_admin(tables=APP_CONFIG.table_classes)
)

but it didn't work. I'm probably doing something wrong, so it would be great if someone from the Starlite community could show how to use this new feature. There is also a missing cryptography dependency when installing Starlite.

dantownsend commented 2 years ago

@sinisaos Thanks for trying it out.

peterschutt commented 2 years ago

@sinisaos you wouldn't happen to have a repro handy would you?

sinisaos commented 2 years ago

@peterschutt I'm sorry but I don't understand. Do you think I have a github repo with the code? I have not. I just tried to change the admin mount function from the Starlite template (generated from piccolo asgi new) with the new code from this example and this, but it doesn't work. If you know how to make it work that would be great.

peterschutt commented 2 years ago

@peterschutt I'm sorry but I don't understand. Do you think I have a github repo with the code? I have not. I just tried to change the admin mount function from the Starlite template (generated from piccolo asgi new) with the new code from this example and this, but it doesn't work. If you know how to make it work that would be great.

The starlite template you linked was exactly what I'm looking for, cheers.

dantownsend commented 2 years ago

The problem might be incompatible Starlette versions. Piccolo Admin has FastAPI as a dependency, which only works with older versions of Starlette.

peterschutt commented 2 years ago

Thanks @dantownsend - I've opened a tracking issue on our side, we'll get to the bottom of it (hopefully!) :+1:

provinzkraut commented 2 years ago

@sinisaos

There is also a missing cryptography dependency when installing Starlite.

That was a bug that has since been fixed. Updating to the newest version of starlite (1.35.1) should fix it!

sinisaos commented 2 years ago

There is also a missing cryptography dependency when installing Starlite.

That was a bug that has since been fixed. Updating to the newest version of starlite (1.35.1) should fix it!

@provinzkraut I can confirm that now the installation is good and all dependencies are included. Thanks.

Piccolo Admin app mounting still doesn't work in this new way. The old way works as expected

provinzkraut commented 2 years ago

@sinisaos Thanks for the feedback! Regarding the other issue, as @peterschutt mentioned, we're working on it! You can subscribe to this issue to receive updates: starlite-api/starlite#716

peterschutt commented 2 years ago

The problem might be incompatible Starlette versions. Piccolo Admin has FastAPI as a dependency, which only works with older versions of Starlette.

I think you are correct. I see that the application generated with piccolo asgi new pins starlite to v1.23. If I change that restriction to >=1.35 (first release that includes the feature), then piccolo-admin gets resolved down to v0.10.9, which is the version before this app migrated to fastapi from starlette.

@sinisaos can you confirm how you set up the environment to test this please?

sinisaos commented 2 years ago

@peterschutt Dependencies are not a problem. All major dependencies are on the latest version.

fastapi==0.85.1
pydantic==1.10.2
piccolo==0.95.0
piccolo-admin==0.35.0
piccolo-api==0.48.1
starlette==0.21.0
starlite==1.35.1
peterschutt commented 2 years ago

with that exact requirements.txt I get:

ERROR: Cannot install -r requirements.txt (line 1) and starlette==0.21.0 because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested starlette==0.21.0
    fastapi 0.85.1 depends on starlette==0.20.4

To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip attempt to solve the dependency conflict
sinisaos commented 2 years ago

@peterschutt Thanks again. I tried again with fresh virtualenv and you are right. Starlite 1.35.1 depends on Starlette 0.21.0, and FastAPI 0.85.1 depends on Starlette 0.20.4. If I understand correctly, we will not be able to change the Starlite asgi template until FastAPI switches to the latest Starlette version. Then we will know if it all works together. @dantownsend For now there is no need to change anything because the old way works and later we see if that works.

peterschutt commented 2 years ago

No probs - and as you say, at least there is already the working solution that you put together 👍

dantownsend commented 2 years ago

Yeah, each FastAPI release version pins a single version of Starlette, which means it doesn't play nicely with other libraries which depend on Starlette.

https://github.com/tiangolo/fastapi/blob/f67b19f0f73ebdca01775b8c7e531e51b9cecfae/pyproject.toml#L41

It's quite annoying.

peterschutt commented 2 years ago

Oh right, I just assumed it must've been something to do with the requests/httpx business recently. TIL!

sinisaos commented 2 years ago

@peterschutt FastAPI is now using Starlette version 0.21.0, but the new way to mount an asgi app in Starlite==1.39.0 still doesn't work. I apologize in advance if I don't know how to use that new feature. I tried this to way but neither way works properly (doesn't match any Piccolo Admin sub-routes).

@asgi("/admin/", is_mount=True)
async def admin(scope: "Scope", receive: "Receive", send: "Send") -> None:
    await create_admin(tables=APP_CONFIG.table_classes)(scope, receive, send)

and

admin = asgi(path="/admin/", is_mount=True)(
    create_admin(tables=APP_CONFIG.table_classes)
)

Old way with Starlette router works. Thanks in advance.

peterschutt commented 2 years ago

OK, thanks for the update, have reopened our tracking issue. https://github.com/starlite-api/starlite/issues/716

sinisaos commented 2 years ago

@peterschutt Thanks for your effort. If it's a big problem, maybe you shouldn't even bother troubleshooting because the Starlette router method works with the latest version of Starlite as well.

Goldziher commented 1 year ago

im working on this now. @dantownsend - can you reopen this issue here?

Goldziher commented 1 year ago

@sinisaos @dantownsend - PR created in Starlite (https://github.com/starlite-api/starlite/pull/925) which resolves the path resolution problem. I would emphasize that the issue though is the use of Starlette routing - which is very particular about the presence or lack of a trailing slash. If you create an admin endpoint using Starlite, you wouldnt encounter these problems.

Anyhow, I encountered another issue though, which I'd like your assistance investigating. I added a test app in Starlite (test_apps/piccolo_admin_app) which follows the examples above. It serves the admin correctly, but when I input the user credentials it raises an internal error (i migrated the DB and created a user as per the docs).

It gives a very long stack trace in the shell, mostly inside fastAPI, which seems to be used here. Please check it out and let me know whats what - if the fix we have in place works for you, I will merge and release it. Otherwise let me know what you find and i'll keep digging. I don't think though there are any issues on our end.

sinisaos commented 1 year ago

@Goldziher This is great and with your new changes everything works for me at least. Thanks.

Your test project is not working because you are using Sqlite which does not support Piccolo migrations (SQLite has very limited ALTER TABLE support). You need to make changes in piccolo_conf.py so that you manually add tables and create a user and then everything will work as it should. It should be something like this

# test_apps/piccolo_admin_app/piccolo_conf.py
import asyncio
from piccolo.apps.user.tables import BaseUser
from piccolo.conf.apps import AppRegistry
from piccolo.engine import SQLiteEngine
from piccolo_api.session_auth.tables import SessionsBase

from home.tables import Task

DB = SQLiteEngine(path="test.sqlite")

async def main():
    await BaseUser.create_table(if_not_exists=True)
    await SessionsBase.create_table(if_not_exists=True)
    await Task.create_table(if_not_exists=True)

    user = BaseUser(
        username="piccolo",
        password="piccolo123",
        email="admin@test.com",
        admin=True,
        active=True,
        superuser=True,
    )
    await user.save()

if __name__ == "__main__":
    asyncio.run(main())

After this changes run python piccolo_conf.py to add tables, then run python main.py and log in with username: piccolo and password:piccolo123.

Another way is to change open_database_connection_pool() in main.py like this

# main.py
from piccolo.apps.user.tables import BaseUser
from piccolo_api.session_auth.tables import SessionsBase

async def open_database_connection_pool():
    engine = engine_finder()
    await engine.start_connection_pool()
    await BaseUser.create_table(if_not_exists=True)
    await SessionsBase.create_table(if_not_exists=True)
    await Task.create_table(if_not_exists=True)

    if not await BaseUser.exists().where(BaseUser.email == "admin@test.com"):
        user = BaseUser(
            username="piccolo",
            password="piccolo123",
            email="admin@test.com",
            admin=True,
            active=True,
            superuser=True,
        )
        await user.save()

# test_apps/piccolo_admin_app/piccolo_conf.py
from piccolo.engine import SQLiteEngine

DB = SQLiteEngine(path="test.sqlite")

and then run the script with python main.py and and log in with username: piccolo and password:piccolo123.

I also try your changes in a fresh project generated from piccolo asgi new with Postgres and everything works.

  1. Generate a project withpiccolo asgi new.
  2. Install dependencies with pip install -r requirements.txt (also install msgspec and multidict).
  3. After that replace Starlite==1.23.0 with new Starlite from your PR in site-packages.
  4. For the test project I used Postgres (register db user and db name in piccolo_conf.py).
  5. After that, start the migrations and create user and now you can log in to Piccolo Admin at the address http://localhost:8000/admin/ and everything should be fine (at least for me works).

@dantownsend Can you confirm that everything is working for you with these new Starlite changes?