feat: add duplicate route detection in APIIngress #46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Caution Review failedThe pull request is closed. WalkthroughA new add_api_route method was added to APIIngress to centralize route registration: it normalizes path patterns (parameter placeholders), uppercases/validates methods, detects duplicate path+method conflicts against app.routes and raises RuntimeError on duplicates; register_route now delegates to it. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller / register_route
participant Ingress as APIIngress.add_api_route
participant App as FastAPI app
participant Routes as app.routes (APIRoute-like)
Caller->>Ingress: add_api_route(path, endpoint, methods?, **kwargs)
Ingress->>Ingress: normalize path (replace params with `{}`)
Ingress->>Ingress: normalize & validate methods (uppercase, check case)
Ingress->>Routes: iterate existing routes
alt normalized path matches existing route
Routes-->>Ingress: existing route (with methods)
Ingress->>Ingress: compute method intersection
alt intersection non-empty
Ingress-->>Caller: raise RuntimeError("Duplicate API route")
else
Ingress->>App: app.add_api_route(path, endpoint, methods=methods, **kwargs)
App-->>Ingress: route registered
Ingress-->>Caller: return
end
else no matching path
Ingress->>App: app.add_api_route(path, endpoint, methods=methods, **kwargs)
App-->>Ingress: route registered
Ingress-->>Caller: return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Owner
Author
|
@coderabbitai generate test |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1491 🏁 Script executed: Length of output: 6740 @touale, I'll generate comprehensive tests for the duplicate route detection feature. Here's a complete test file: import pytest
from unittest.mock import Mock, patch
from fastapi.routing import APIRoute
from framex.driver.ingress import APIIngress
class TestAPIIngressDuplicateRouteDetection:
"""Test suite for duplicate route detection in APIIngress.add_api_route()"""
@pytest.fixture
def ingress(self):
"""Create an APIIngress instance for testing"""
with patch("framex.driver.ingress.app") as mock_app:
mock_app.routes = []
mock_app.state = Mock()
ingress = APIIngress.__new__(APIIngress)
return ingress
@pytest.fixture
def mock_app(self):
"""Mock FastAPI app with routes"""
with patch("framex.driver.ingress.app") as mock_app:
mock_app.routes = []
mock_app.add_api_route = Mock()
yield mock_app
def test_add_first_route_success(self, ingress, mock_app):
"""Test adding the first route succeeds"""
endpoint = Mock()
ingress.add_api_route(
path="/users",
endpoint=endpoint,
methods=["GET"]
)
mock_app.add_api_route.assert_called_once()
args, kwargs = mock_app.add_api_route.call_args
assert args[0] == "/users"
assert args[1] == endpoint
assert kwargs["methods"] == ["GET"]
def test_duplicate_route_same_path_same_method_raises_error(self, ingress, mock_app):
"""Test that adding duplicate route with same path and method raises RuntimeError"""
existing_route = Mock(spec=APIRoute)
existing_route.path = "/users/{id}"
existing_route.methods = {"GET"}
mock_app.routes = [existing_route]
endpoint = Mock()
with pytest.raises(RuntimeError, match=r"Duplicate API route: \['GET'\] /users/\{\}"):
ingress.add_api_route(
path="/users/{id}",
endpoint=endpoint,
methods=["GET"]
)
def test_duplicate_route_different_param_names_raises_error(self, ingress, mock_app):
"""Test that routes with different parameter names but same pattern are detected as duplicates"""
existing_route = Mock(spec=APIRoute)
existing_route.path = "/users/{id}"
existing_route.methods = {"GET"}
mock_app.routes = [existing_route]
endpoint = Mock()
# Try to add route with different parameter name but same pattern
with pytest.raises(RuntimeError, match=r"Duplicate API route: \['GET'\] /users/\{\}"):
ingress.add_api_route(
path="/users/{user_id}",
endpoint=endpoint,
methods=["GET"]
)
def test_same_path_different_methods_allowed(self, ingress, mock_app):
"""Test that same path with different methods is allowed"""
existing_route = Mock(spec=APIRoute)
existing_route.path = "/users/{id}"
existing_route.methods = {"GET"}
mock_app.routes = [existing_route]
endpoint = Mock()
# Should succeed - different method
ingress.add_api_route(
path="/users/{id}",
endpoint=endpoint,
methods=["POST"]
)
mock_app.add_api_route.assert_called_once()
def test_overlapping_methods_raises_error(self, ingress, mock_app):
"""Test that routes with overlapping methods raise error"""
existing_route = Mock(spec=APIRoute)
existing_route.path = "/users/{id}"
existing_route.methods = {"GET", "POST"}
mock_app.routes = [existing_route]
endpoint = Mock()
# Try to add route that overlaps with one method
with pytest.raises(RuntimeError, match=r"Duplicate API route:"):
ingress.add_api_route(
path="/users/{id}",
endpoint=endpoint,
methods=["POST", "PUT"] # POST overlaps
)
def test_multiple_parameters_normalized_correctly(self, ingress, mock_app):
"""Test that paths with multiple parameters are normalized correctly"""
existing_route = Mock(spec=APIRoute)
existing_route.path = "/users/{user_id}/posts/{post_id}"
existing_route.methods = {"GET"}
mock_app.routes = [existing_route]
endpoint = Mock()
# Try to add route with different parameter names
with pytest.raises(RuntimeError, match=r"Duplicate API route: \['GET'\] /users/\{\}/posts/\{\}"):
ingress.add_api_route(
path="/users/{id}/posts/{pid}",
endpoint=endpoint,
methods=["GET"]
)
def test_different_paths_no_conflict(self, ingress, mock_app):
"""Test that different paths don't conflict"""
existing_route = Mock(spec=APIRoute)
existing_route.path = "/users/{id}"
existing_route.methods = {"GET"}
mock_app.routes = [existing_route]
endpoint = Mock()
# Should succeed - different path
ingress.add_api_route(
path="/posts/{id}",
endpoint=endpoint,
methods=["GET"]
)
mock_app.add_api_route.assert_called_once()
def test_case_insensitive_method_matching(self, ingress, mock_app):
"""Test that HTTP methods are matched case-insensitively"""
existing_route = Mock(spec=APIRoute)
existing_route.path = "/users"
existing_route.methods = {"GET"}
mock_app.routes = [existing_route]
endpoint = Mock()
# Try to add route with lowercase method
with pytest.raises(RuntimeError, match=r"Duplicate API route: \['GET'\] /users"):
ingress.add_api_route(
path="/users",
endpoint=endpoint,
methods=["get"] # lowercase
)
def test_multiple_methods_sorted_in_error_message(self, ingress, mock_app):
"""Test that error message contains sorted methods"""
existing_route = Mock(spec=APIRoute)
existing_route.path = "/users"
existing_route.methods = {"POST", "PUT"}
mock_app.routes = [existing_route]
endpoint = Mock()
# Try to add duplicate with multiple methods
with pytest.raises(RuntimeError, match=r"Duplicate API route: \['POST', 'PUT'\] /users"):
ingress.add_api_route(
path="/users",
endpoint=endpoint,
methods=["PUT", "POST"] # Unsorted input
)
def test_none_methods_parameter(self, ingress, mock_app):
"""Test handling of None methods parameter"""
endpoint = Mock()
# Should succeed with empty method set
ingress.add_api_route(
path="/users",
endpoint=endpoint,
methods=None
)
mock_app.add_api_route.assert_called_once()
args, kwargs = mock_app.add_api_route.call_args
assert kwargs["methods"] == []
def test_non_api_route_ignored(self, ingress, mock_app):
"""Test that non-APIRoute routes are ignored in duplicate detection"""
from starlette.routing import Route
non_api_route = Mock(spec=Route)
non_api_route.path = "/users/{id}"
mock_app.routes = [non_api_route]
endpoint = Mock()
# Should succeed - non-APIRoute is ignored
ingress.add_api_route(
path="/users/{id}",
endpoint=endpoint,
methods=["GET"]
)
mock_app.add_api_route.assert_called_once()
def test_kwargs_passed_through(self, ingress, mock_app):
"""Test that additional kwargs are passed through to app.add_api_route"""
endpoint = Mock()
ingress.add_api_route(
path="/users",
endpoint=endpoint,
methods=["GET"],
tags=["users"],
dependencies=[],
response_class=Mock()
)
args, kwargs = mock_app.add_api_route.call_args
assert "tags" in kwargs
assert "dependencies" in kwargs
assert "response_class" in kwargs
def test_complex_path_with_multiple_segments(self, ingress, mock_app):
"""Test complex paths with multiple segments and parameters"""
existing_route = Mock(spec=APIRoute)
existing_route.path = "/api/v1/organizations/{org_id}/teams/{team_id}/members/{member_id}"
existing_route.methods = {"DELETE"}
mock_app.routes = [existing_route]
endpoint = Mock()
expected_pattern = r"Duplicate API route: \['DELETE'\] /api/v1/organizations/\{\}/teams/\{\}/members/\{\}"
with pytest.raises(RuntimeError, match=expected_pattern):
ingress.add_api_route(
path="/api/v1/organizations/{organization_id}/teams/{t_id}/members/{m_id}",
endpoint=endpoint,
methods=["DELETE"]
)This test file covers:
To run these tests: pytest tests/driver/test_ingress.py -vThe tests use mocking to isolate the |
Owner
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/framex/driver/ingress.py:
- Around line 159-168: The duplicate-detection misses routes when methods is
None/empty because method_set becomes empty; update add_api_route to handle that
edge case by defaulting method_set to {"GET"} when methods is None or an empty
iterable (i.e., replace method_set: set[str] = {m.upper() for m in (methods or
[])} with logic that yields {"GET"} if the resulting set is empty), then keep
the existing APIRoute/path comparison and intersection check so duplicate routes
without explicit methods are detected; alternatively, if you prefer validation,
raise a clear error in add_api_route when methods is None/empty (mirror
register_route behavior) — pick one consistent approach and apply it to the
duplicate check and any error message text.
🧹 Nitpick comments (2)
src/framex/driver/ingress.py (2)
82-85: Consider consolidating duplicate detection logic.This exact-path check is now partially redundant with the normalized path + method checking in
add_api_route(lines 162-168). The two checks have different behaviors:
- This check: exact path match → logs warning and returns
False- New check: normalized path match with method overlap → raises
RuntimeErrorConsider either removing this check entirely (relying on the more comprehensive check in
add_api_route) or documenting why both are needed.🔎 Option to remove redundant check
- try: - routes: list[str] = [route.path for route in app.routes if isinstance(route, Route | APIRoute)] - if path in routes: - logger.warning(f"API({path}) with tags {tags} is already registered, skipping duplicate registration.") - return False - if (not path) or (not methods): + try: + if (not path) or (not methods):
168-168: Improve error message formatting.The error message uses
sorted(method_set)which returns a list. Consider formatting it more clearly for better readability.🔎 Improved error message formatting
- raise RuntimeError(f"Duplicate API route: {sorted(method_set)} {norm_path}") + raise RuntimeError(f"Duplicate API route detected: {', '.join(sorted(method_set))} {norm_path}")Additionally, the static analysis tool suggests avoiding long messages outside exception classes (TRY003). While this is a style preference, you could consider defining a custom exception class if this becomes more complex.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/framex/driver/ingress.py
🧰 Additional context used
🪛 Ruff (0.14.10)
src/framex/driver/ingress.py
168-168: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
src/framex/driver/ingress.py (3)
1-1: LGTM!The
remodule import is necessary for path normalization in the duplicate detection logic.
127-134: LGTM!Delegating to
self.add_api_routeproperly centralizes route registration and enables the duplicate detection logic.
170-170: Route registration looks correct.Converting
method_setback to a list is appropriate for FastAPI'sadd_api_route. However, this depends on properly handling the empty methods case flagged in the previous comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/driver/test_ingress.py (2)
30-32: Consider refactoring to avoid bypassing__init__.Using
__new__to bypass__init__tightly couples the tests to implementation details and makes them brittle. Ifadd_api_routeever needs to access instance attributes, these tests will break.Consider alternatives:
- Provide a factory method or test constructor that accepts minimal dependencies
- Use dependency injection to pass the app as a parameter
- Refactor
add_api_routeto be a standalone function if it doesn't need instance stateAlternative fixture approach
@pytest.fixture def ingress(mock_app): # Create a proper instance with minimal dependencies # or refactor APIIngress to accept app as a parameter with patch("framex.driver.ingress.app", mock_app): instance = APIIngress.__new__(APIIngress) # Explicitly set any required attributes if needed return instanceNote: This still uses
__new__but makes the dependency on mock_app explicit. A better long-term solution would be to refactor the class to support testing without__new__.
76-80: Consider clarifying the test name.The test name
test_case_insensitive_methodscould be more descriptive. It's specifically testing that duplicate detection is case-insensitive (i.e., "get" matches "GET"). A name liketest_duplicate_detection_is_case_insensitiveortest_lowercase_method_matches_uppercase_routewould be clearer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/driver/test_ingress.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/driver/test_ingress.py (1)
src/framex/driver/ingress.py (2)
APIIngress(31-170)add_api_route(151-170)
🔇 Additional comments (4)
tests/driver/test_ingress.py (4)
1-16: LGTM!The imports are appropriate for the testing needs, and the
make_routehelper provides a clean way to create test doubles for APIRoute objects.
38-73: LGTM!These tests provide good coverage:
- Basic route addition is validated
- Path normalization with different parameter names is properly tested via parametrization
- Non-overlapping methods are correctly allowed
- Overlapping methods correctly raise RuntimeError
The parametrized test on lines 46-58 is particularly valuable for verifying path normalization behavior.
100-111: LGTM!The test correctly verifies that additional kwargs are passed through to the underlying
app.add_api_routecall, ensuring the wrapper doesn't lose configuration options.
4-4: No issues found. FastAPI 0.115.13 exists and theAPIRouteimport fromfastapi.routingis compatible with this version. The code is correct.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.