litestar-org / litestar

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

router with path already registered #148

Closed mybigman closed 2 years ago

mybigman commented 2 years ago

Thought I'd have a little play with Starlite as it seems nice.

https://starlite-api.github.io/starlite/usage/1-routers-and-controllers/#registering-routes

# controller
from starlite.controller import Controller
from starlite.handlers import get

class HomeController(Controller):
    path = "/"

    @get()
    async def home(self) -> None:
        return {"message": "hello"}
# route handler
from controllers.home_controller import HomeController
from starlite import Router

base_router = Router(path="/base", route_handlers=[HomeController])
# main
from starlite import Starlite

from routers.base import base_router

app = Starlite(route_handlers=[base_router])

From what I am reading on the docs this should work? However it fails with

  File "/home/champ/projects/v7-starlite/__pypackages__/3.10/lib/starlite/router.py", line 133, in validate_registration_value
    raise ImproperlyConfiguredException(f"Router with path {value.path} has already been registered")
starlite.exceptions.ImproperlyConfiguredException: Router with path /base has already been registered
peterschutt commented 2 years ago

OK, so I roll all those components into a single file as I cannot guess how your project structure is configured, and I don't get the same error.

If you can provide a minimal, reproducible example, I'll have another look.

Here is my test:

from starlite import Router, Starlite
from starlite.controller import Controller
from starlite.handlers import get

class HomeController(Controller):
    path = "/"

    @get()
    async def home(self) -> dict[str, str]:
        return {"message": "hello"}

base_router = Router(path="/base", route_handlers=[HomeController])
app = Starlite(route_handlers=[base_router])
mybigman commented 2 years ago

Using a single file works... but split fails :/

https://github.com/mybigman/test-starlite.git

peterschutt commented 2 years ago

The issue was caused by the import and load of uvicorn inside of main.py. I made a PR into your repo with a bit of a refactor that works.

https://github.com/mybigman/test-starlite/pull/1

@Goldziher I don't think this is a bug on our side, but probably another tick in the box for a cli that bootstraps a standard project template with some instructions in the readme.

mybigman commented 2 years ago

@peterschutt thanks for taking the time too look and PR.

I believe it should work the way it is though?

If I make a change to toml using the start feature and relative imports it works.

[tool.pdm.scripts]
start = "uvicorn app.main:app --host 0.0.0.0 --port 8080 --reload --debug"

Then run pdm run start

Something funny going on there that I am just not seeing.

peterschutt commented 2 years ago

Hey, so this is what is happening here.

Inside main.py when it is first invoked, you tell uvicorn to import app from main.py with the uvicorn.run("main:app", ...). So your command pdm run python app/main.py tells uvicorn to import app from main.py as well. That causes main.py to be loaded a 2nd time and at that point, the router you are passing to the Starlite constructor has already been processed and given an owner, which is why you get the error.

To see for yourself, just put print("whatever") at the top of main.py, and you'll see that when you invoke pdm run python main.py "whatever" is printed twice. Once for your invocation, and once for uvicorn's invocation.

The reason why you don't get the error when running via tool.pdm.scripts is that main.py only has to be invoked once in that case - by uvicorn. In the other case you invoke main.py to invoke uvicorn which then invokes main.py again.

I'll close this now, but if you have any more Qs, feel free to reach out in the discord server. You'll find the invite in README.md.