Repository: vitalik/django-ninja
Pull request #369 (2022-02-20 17:26:45)

Allow path parameters to be specified at router level
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

```python
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:

```shell
# 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.:

```python
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.


Patch:
diff --git a/ninja/main.py b/ninja/main.py
index f1bb74b..fbf540e 100644
--- a/ninja/main.py
+++ b/ninja/main.py
@@ -324,8 +324,7 @@ class NinjaAPI:
         result = get_openapi_urls(self)
 
         for prefix, router in self._routers:
-            for path in router.urls_paths(prefix):
-                result.append(path)
+            result.extend(router.urls_paths(prefix))
 
         result.append(get_root_url(self))
         return result
diff --git a/ninja/router.py b/ninja/router.py
index 79636a7..29c9bce 100644
--- a/ninja/router.py
+++ b/ninja/router.py
@@ -16,7 +16,7 @@ from ninja.constants import NOT_SET
 from ninja.errors import ConfigError
 from ninja.operation import PathView
 from ninja.types import TCallable
-from ninja.utils import normalize_path
+from ninja.utils import normalize_path, replace_path_param_notation
 
 if TYPE_CHECKING:
     from ninja import NinjaAPI  # pragma: no cover
@@ -316,8 +316,9 @@ class Router:
             router.set_api_instance(api, self)
 
     def urls_paths(self, prefix: str) -> Iterator[URLPattern]:
+        prefix = replace_path_param_notation(prefix)
         for path, path_view in self.path_operations.items():
-            path = path.replace("{", "<").replace("}", ">")
+            path = replace_path_param_notation(path)
             route = "/".join([i for i in (prefix, path) if i])
             # to skip lot of checks we simply treat double slash as a mistake:
             route = normalize_path(route)
diff --git a/ninja/utils.py b/ninja/utils.py
index f2d6e23..8375375 100644
--- a/ninja/utils.py
+++ b/ninja/utils.py
@@ -8,6 +8,10 @@ from django.middleware.csrf import CsrfViewMiddleware
 __all__ = ["check_csrf", "is_debug_server", "normalize_path"]
 
 
+def replace_path_param_notation(path: str) -> str:
+    return path.replace("{", "<").replace("}", ">")
+
+
 def normalize_path(path: str) -> str:
     while "//" in path:
         path = path.replace("//", "/")


Test patch:
diff --git a/tests/test_router_path_params.py b/tests/test_router_path_params.py
new file mode 100644
index 0000000..c9fdfde
--- /dev/null
+++ b/tests/test_router_path_params.py
@@ -0,0 +1,83 @@
+import pytest
+
+from ninja import NinjaAPI, Path, Router
+from ninja.testing import TestClient
+
+api = NinjaAPI()
+router_with_path_type = Router()
+router_without_path_type = Router()
+router_with_multiple = Router()
+
+
+@router_with_path_type.get("/metadata")
+def get_item_metadata(request, item_id: int = Path(None)) -> int:
+    return item_id
+
+
+@router_without_path_type.get("/")
+def get_item_metadata_2(request, item_id: str = Path(None)) -> str:
+    return item_id
+
+
+@router_without_path_type.get("/metadata")
+def get_item_metadata_3(request, item_id: str = Path(None)) -> str:
+    return item_id
+
+
+@router_without_path_type.get("/")
+def get_item_metadata_4(request, item_id: str = Path(None)) -> str:
+    return item_id
+
+
+@router_with_multiple.get("/metadata/{kind}")
+def get_item_metadata_5(
+    request, item_id: int = Path(None), name: str = Path(None), kind: str = Path(None)
+) -> str:
+    return f"{item_id} {name} {kind}"
+
+
+api.add_router("/with_type/{int:item_id}", router_with_path_type)
+api.add_router("/without_type/{item_id}", router_without_path_type)
+api.add_router("/with_multiple/{int:item_id}/name/{name}", router_with_multiple)
+
+client = TestClient(api)
+
+
+@pytest.mark.parametrize(
+    "path,expected_status,expected_response",
+    [
+        ("/with_type/1/metadata", 200, 1),
+        ("/without_type/1/metadata", 200, "1"),
+        ("/without_type/abc/metadata", 200, "abc"),
+        ("/with_multiple/99/name/foo/metadata/timestamp", 200, "99 foo timestamp"),
+    ],
+)
+def test_router_with_path_params(path, expected_status, expected_response):
+    response = client.get(path)
+    assert response.status_code == expected_status
+    assert response.json() == expected_response
+
+
+@pytest.mark.parametrize(
+    "path,expected_exception,expect_exception_contains",
+    [
+        ("/with_type/abc/metadata", Exception, "Cannot resolve"),
+        ("/with_type//metadata", Exception, "Cannot resolve"),
+        ("/with_type/null/metadata", Exception, "Cannot resolve"),
+        ("/with_type", Exception, "Cannot resolve"),
+        ("/with_type/", Exception, "Cannot resolve"),
+        ("/with_type//", Exception, "Cannot resolve"),
+        ("/with_type/null", Exception, "Cannot resolve"),
+        ("/with_type/null/", Exception, "Cannot resolve"),
+        ("/without_type", Exception, "Cannot resolve"),
+        ("/without_type/", Exception, "Cannot resolve"),
+        ("/without_type//", Exception, "Cannot resolve"),
+        ("/with_multiple/abc/name/foo/metadata/timestamp", Exception, "Cannot resolve"),
+        ("/with_multiple/99", Exception, "Cannot resolve"),
+    ],
+)
+def test_router_with_path_params_nomatch(
+    path, expected_exception, expect_exception_contains
+):
+    with pytest.raises(expected_exception, match=expect_exception_contains):
+        client.get(path)
diff --git a/tests/test_utils.py b/tests/test_utils.py
new file mode 100644
index 0000000..830ca1d
--- /dev/null
+++ b/tests/test_utils.py
@@ -0,0 +1,18 @@
+import pytest
+
+from ninja.utils import replace_path_param_notation
+
+
+@pytest.mark.parametrize(
+    "input,expected_output",
+    [
+        ("abc/{def}", "abc/<def>"),
+        ("abc/<def>", "abc/<def>"),
+        ("abc", "abc"),
+        ("<abc>", "<abc>"),
+        ("{abc}", "<abc>"),
+        ("{abc}/{def}", "<abc>/<def>"),
+    ],
+)
+def test_replace_path_param_notation(input, expected_output):
+    assert replace_path_param_notation(input) == expected_output