Browse Source

feat: Add warning when route conflict detected in api routes.

pull/14318/head
YeLinAung 8 months ago
parent
commit
300d5ff04a
  1. 48
      fastapi/routing.py
  2. 312
      tests/test_route_conflicts.py

48
fastapi/routing.py

@ -676,6 +676,52 @@ class APIRoute(routing.Route):
return match, child_scope
def _detect_route_conflicts(
new_route: "APIRoute",
existing_routes: Sequence[BaseRoute],
) -> List[str]:
"""
Detect and warn about route conflicts.
Checks if route patterns could shadow each other. Only warns when HTTP methods overlap.
"""
import warnings
conflicts = []
if not isinstance(new_route, APIRoute):
return conflicts
for existing in existing_routes:
if not isinstance(existing, APIRoute):
continue
if not (new_route.methods & existing.methods):
continue
if new_route.path == existing.path:
continue
methods_str = ", ".join(sorted(new_route.methods))
if new_route.path_regex.match(existing.path):
conflicts.append(
f"Route {new_route.path} ({methods_str}) "
f"may shadow existing route {existing.path}"
)
elif existing.path_regex.match(new_route.path):
conflicts.append(
f"Route {new_route.path} ({methods_str}) "
f"may be shadowed by existing route {existing.path}"
)
if conflicts:
for conflict in conflicts:
warnings.warn(conflict, UserWarning, stacklevel=4)
return conflicts
class APIRouter(routing.Router):
"""
`APIRouter` class, used to group *path operations*, for example to structure
@ -1042,6 +1088,8 @@ class APIRouter(routing.Router):
openapi_extra=openapi_extra,
generate_unique_id_function=current_generate_unique_id,
)
# Detect and warn about potential route conflicts
_detect_route_conflicts(route, self.routes)
self.routes.append(route)
def api_route(

312
tests/test_route_conflicts.py

@ -0,0 +1,312 @@
import warnings
from fastapi import FastAPI
from fastapi.routing import APIRouter
from fastapi.testclient import TestClient
def test_route_conflict_warning_dynamic_before_static():
"""Dynamic route registered before static route should warn."""
app = FastAPI()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@app.get("/items/{item_id}")
def get_item(item_id: str):
return {"item_id": item_id}
@app.get("/items/sync") # This will be shadowed!
def sync_items():
return {"action": "sync"}
# Should have warning
assert len(w) == 1
assert "shadow" in str(w[0].message).lower()
assert "/items/sync" in str(w[0].message)
assert "/items/{item_id}" in str(w[0].message)
def test_route_conflict_warning_static_before_dynamic():
"""Static route before dynamic should work but warn about potential conflict."""
app = FastAPI()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@app.get("/items/sync")
def sync_items():
return {"action": "sync"}
@app.get("/items/{item_id}")
def get_item(item_id: str):
return {"item_id": item_id}
# Should warn about potential conflict
assert len(w) == 1
assert "shadow" in str(w[0].message).lower()
def test_no_conflict_different_methods():
"""Different HTTP methods should not conflict."""
app = FastAPI()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@app.get("/items/{item_id}")
def get_item(item_id: str):
return {"item_id": item_id}
@app.post("/items/sync")
def sync_items():
return {"action": "sync"}
# Should NOT warn (different methods)
assert len(w) == 0
def test_no_conflict_different_paths():
"""Completely different paths should not conflict."""
app = FastAPI()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@app.get("/items/{item_id}")
def get_item(item_id: str):
return {"item_id": item_id}
@app.get("/products/sync")
def sync_products():
return {"action": "sync"}
# Should NOT warn (different paths)
assert len(w) == 0
def test_actual_routing_behavior_static_before_dynamic():
"""Verify that static-before-dynamic actually works correctly."""
app = FastAPI()
with warnings.catch_warnings():
warnings.simplefilter("ignore")
@app.get("/items/sync")
def sync_items():
return {"action": "sync"}
@app.get("/items/{item_id}")
def get_item(item_id: str):
return {"item_id": item_id}
client = TestClient(app)
# Static route should match
response = client.get("/items/sync")
assert response.json() == {"action": "sync"}
# Dynamic route should match others
response = client.get("/items/123")
assert response.json() == {"item_id": "123"}
def test_actual_routing_behavior_dynamic_before_static():
"""Verify that dynamic-before-static causes shadowing."""
app = FastAPI()
with warnings.catch_warnings():
warnings.simplefilter("ignore")
@app.get("/items/{item_id}")
def get_item(item_id: str):
return {"item_id": item_id}
@app.get("/items/sync")
def sync_items():
return {"action": "sync"}
client = TestClient(app)
# Dynamic route will match everything (including "sync")
response = client.get("/items/sync")
assert response.json() == {"item_id": "sync"}
# Dynamic route should still match others
response = client.get("/items/123")
assert response.json() == {"item_id": "123"}
def test_router_conflict_detection():
"""Test conflict detection works with APIRouter."""
router = APIRouter()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@router.get("/users/{user_id}")
def get_user(user_id: str):
return {"user_id": user_id}
@router.get("/users/me")
def get_current_user():
return {"user": "current"}
# Should warn
assert len(w) == 1
assert "shadow" in str(w[0].message).lower()
def test_multiple_conflicts():
"""Test detection of multiple route conflicts."""
app = FastAPI()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@app.get("/items/{item_id}")
def get_item(item_id: str):
return {"item_id": item_id}
@app.get("/items/sync")
def sync_items():
return {"action": "sync"}
@app.get("/items/export")
def export_items():
return {"action": "export"}
# Should have 2 warnings (sync and export both shadowed)
assert len(w) == 2
def test_nested_path_conflict():
"""Test conflict detection with nested paths."""
app = FastAPI()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@app.get("/users/{user_id}/posts/{post_id}")
def get_user_post(user_id: str, post_id: str):
return {"user_id": user_id, "post_id": post_id}
@app.get("/users/me/posts/{post_id}")
def get_my_post(post_id: str):
return {"user": "me", "post_id": post_id}
# Should warn
assert len(w) == 1
def test_no_duplicate_warnings_same_route():
"""Adding the same static route twice should not cause conflict warnings."""
app = FastAPI()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@app.get("/items/sync")
def sync_items_v1():
return {"version": 1}
# FastAPI will overwrite the route, but should not warn about conflict with itself
# (The path regex won't match its own literal path if they're identical)
# Should have 0 warnings
assert len(w) == 0
def test_conflict_with_router_prefix():
"""Test conflict detection when router has a prefix."""
app = FastAPI()
router = APIRouter(prefix="/api/v1")
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@router.get("/items/{item_id}")
def get_item(item_id: str):
return {"item_id": item_id}
@router.get("/items/special")
def get_special_item():
return {"special": True}
# Should warn during router setup
assert len(w) == 1
# Including router in app may generate another warning, suppress it
with warnings.catch_warnings():
warnings.simplefilter("ignore")
app.include_router(router)
client = TestClient(app)
# Verify the routes work correctly with prefix
response = client.get("/api/v1/items/123")
assert response.status_code == 200
def test_post_vs_get_no_conflict():
"""POST and GET to same path patterns should not conflict."""
app = FastAPI()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@app.get("/items/{item_id}")
def get_item(item_id: str):
return {"item_id": item_id}
@app.post("/items/{item_id}")
def update_item(item_id: str):
return {"updated": item_id}
@app.get("/items/sync")
def sync_items():
return {"action": "sync"}
# Should only warn for GET /items/sync vs GET /items/{item_id}
# Should NOT warn about POST /items/{item_id}
assert len(w) == 1
assert "GET" in str(w[0].message) or "methods" in str(w[0].message).lower()
def test_complex_multi_param_paths():
"""Test conflict detection with multiple path parameters."""
app = FastAPI()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@app.get("/api/{version}/items/{item_id}")
def get_item_versioned(version: str, item_id: str):
return {"version": version, "item_id": item_id}
@app.get("/api/v1/items/special")
def get_special_item():
return {"special": True}
# Should warn - /api/{version}/items/{item_id} can match /api/v1/items/special
assert len(w) == 1
assert "/api/v1/items/special" in str(w[0].message)
assert "/api/{version}/items/{item_id}" in str(w[0].message)
def test_no_conflict_different_param_depth():
"""Test no conflict when paths have different depth."""
app = FastAPI()
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
@app.get("/items/{item_id}")
def get_item(item_id: str):
return {"item_id": item_id}
@app.get("/categories/special")
def get_special_category():
return {"category": "special"}
# Should NOT warn - completely different paths
assert len(w) == 0
Loading…
Cancel
Save