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

Allow path parameters to be specified at router level #369

Merged
merged 1 commit into from
Feb 21, 2022
Merged

Allow path parameters to be specified at router level #369

merged 1 commit into from
Feb 21, 2022

Conversation

kaschnit
Copy link
Contributor

@kaschnit kaschnit commented Feb 20, 2022

This PR proposes and implements the following.

Current behavior

Adding path params when including the router (e.g. app.add_router("/item/{item_id}", router) does not work as expected. The test client and the actual application routing logic cannot find the endpoint.

New (proposed) behavior

Using app.add_router("/item/{item_id}", router) will allow the user to treat item_id as a path parameter if they so choose.

This change should not break any existing behavior.

Example

With FastAPI, you can do something like this

api = FastAPI()
router = Router(prefix="item/{item_id}/")

@router.get("/metadata")
def get_item_metadata(item_id: str):
  return item_id

api.include_router(router)

Then you could do the following and expect output:

# command
curl localhost:8000/item/my_item/metadata

# output
my_item

This change will allow a similar pattern in django-ninja path parameters when including the router in a parent router, e.g.:

app = Ninja()
router = Router()

@router.get("/metadata")
def get_item_metadata(request, item_id: str = Path(None)):
  return item_id

app.add_router("/item/{item_id}/", router)

This PR only allows this to work when using the Path() object to indicate that it's a path parameter. The change to make it auto detect these is less trivial because the router's "prefix" is not actually a property of the Router, but a property of its parent router, so reading the prefix path is difficult when resolving path parameters.

@vitalik vitalik merged commit 77d0f6f into vitalik:master Feb 21, 2022
@vitalik
Copy link
Owner

vitalik commented Feb 21, 2022

thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants