Clamp recent usage limit requests #652
Conversation
WalkthroughAdds two public constants, introduces a limit-normalization utility, and integrates it across the usage controller and service to validate, clamp, and early-return on invalid limits. API parameters now constrain the limit via FastAPI Query. Tests are updated to cover edge cases, clamping, and zero/negative limit behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as UsageController (FastAPI)
participant U as normalize_recent_usage_limit
participant S as UsageTrackingService
participant R as Repository
C->>API: GET /usage/recent?limit=L
note right of API: FastAPI Query enforces 0 ≤ L ≤ MAX
API->>U: normalize(L)
U-->>API: normalized_limit
alt normalized_limit == 0
API-->>C: []
note over API,C: Early return for non-positive/invalid limit
else
API->>S: get_recent_usage(normalized_limit)
S->>U: normalize(limit)
U-->>S: normalized_limit
alt normalized_limit == 0
S-->>API: []
API-->>C: []
else
S->>R: get_all()
alt no data
S-->>API: []
API-->>C: []
else
S-->>API: recent[:normalized_limit]
API-->>C: result
end
end
end
note over API,S: Logs when clamping occurs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (2)
src/core/app/controllers/usage_controller.py (2)
67-93: Consider removing duplicate normalization logic.Both the controller and service perform identical normalization: converting to int, calling
normalize_recent_usage_limit, checking for zero, logging clamping, and returning early. This violates the DRY principle.Since the service already handles normalization defensively, the controller could simply pass through the limit and rely on the service layer. Alternatively, if validation is desired at the controller layer, the service could skip normalization.
Option 1: Remove normalization from controller, rely on service:
async def get_recent_usage( self, session_id: str | None = None, limit: int = 100 ) -> list[UsageData]: """Get recent usage data. Args: session_id: Optional session ID filter limit: Maximum number of records to return Returns: List of usage data entities """ if not self.usage_service: return [] - try: - requested_limit = int(limit) - except (TypeError, ValueError): - requested_limit = 0 - - normalized_limit = normalize_recent_usage_limit(requested_limit) - - if normalized_limit == 0: - if logger.isEnabledFor(logging.DEBUG): - logger.debug( - "Recent usage requested with limit=%s; returning empty result", limit - ) - return [] - - if normalized_limit < requested_limit: - if logger.isEnabledFor(logging.INFO): - logger.info( - "Recent usage limit clamped from %s to %s (max=%s)", - limit, - normalized_limit, - MAX_RECENT_USAGE_RECORDS, - ) - result = await self.usage_service.get_recent_usage( - session_id=session_id, limit=normalized_limit + session_id=session_id, limit=limit ) return result # type: ignore[no-any-return]Option 2: Create a shared validation helper that both layers call without duplicating the full logic.
139-164: Router endpoint normalization is partially unreachable.The FastAPI
Queryvalidation at lines 120-125 ensureslimitis in the range[0, MAX_RECENT_USAGE_RECORDS]. This means:
- Negative values are rejected by FastAPI (HTTP 422)
- Values exceeding
MAX_RECENT_USAGE_RECORDSare rejected by FastAPI (HTTP 422)As a result, the normalization logic that handles these cases (lines 141-142 catching negative, lines 153-160 logging clamping) will never execute for those edge cases. The only reachable case is when
limit=0, which is allowed byge=0.This isn't harmful but creates dead code. Consider either:
- Removing the normalization logic here and relying solely on FastAPI validation + service-layer normalization
- Removing the
geandleconstraints fromQueryif you prefer to handle validation in application logic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/constants.py(1 hunks)src/core/app/controllers/usage_controller.py(4 hunks)src/core/common/usage_limits.py(1 hunks)src/core/services/usage_tracking_service.py(2 hunks)tests/unit/core/app/controllers/test_usage_controller_comprehensive.py(3 hunks)tests/unit/core/services/test_usage_tracking_service_comprehensive.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting
Files:
src/constants.pytests/unit/core/app/controllers/test_usage_controller_comprehensive.pytests/unit/core/services/test_usage_tracking_service_comprehensive.pysrc/core/services/usage_tracking_service.pysrc/core/app/controllers/usage_controller.pysrc/core/common/usage_limits.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions
Files:
src/constants.pysrc/core/services/usage_tracking_service.pysrc/core/app/controllers/usage_controller.pysrc/core/common/usage_limits.py
🧬 Code graph analysis (4)
tests/unit/core/app/controllers/test_usage_controller_comprehensive.py (3)
tests/unit/core/services/test_usage_tracking_service_comprehensive.py (2)
test_get_recent_usage_zero_limit(351-359)test_get_recent_usage_negative_limit(362-370)src/core/app/controllers/usage_controller.py (3)
UsageController(23-93)get_recent_usage(52-93)get_recent_usage(118-165)src/core/services/usage_tracking_service.py (1)
get_recent_usage(334-379)
tests/unit/core/services/test_usage_tracking_service_comprehensive.py (2)
tests/unit/core/app/controllers/test_usage_controller_comprehensive.py (3)
test_get_recent_usage_zero_limit(236-243)test_get_recent_usage_negative_limit(246-254)test_get_recent_usage_negative_limit(390-397)src/core/services/usage_tracking_service.py (2)
UsageTrackingService(48-379)get_recent_usage(334-379)
src/core/services/usage_tracking_service.py (2)
src/core/common/usage_limits.py (1)
normalize_recent_usage_limit(10-30)src/core/repositories/in_memory_usage_repository.py (1)
get_by_session_id(63-66)
src/core/app/controllers/usage_controller.py (2)
src/core/common/usage_limits.py (1)
normalize_recent_usage_limit(10-30)src/core/services/usage_tracking_service.py (1)
get_recent_usage(334-379)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10)
🔇 Additional comments (12)
tests/unit/core/app/controllers/test_usage_controller_comprehensive.py (3)
11-11: LGTM! Constant import aligns with new normalization design.Using
MAX_RECENT_USAGE_RECORDSfrom the centralized constants module ensures consistency across tests and implementation.
232-233: LGTM! Using constant instead of hardcoded value.Replacing the hardcoded
10000withMAX_RECENT_USAGE_RECORDScorrectly validates that large limits are clamped to the project-wide maximum.
242-243: LGTM! Test correctly validates early-return behavior.The assertions confirm that zero limits result in an empty list without invoking the service, which is the intended optimization to avoid unnecessary repository calls.
src/constants.py (1)
1-7: LGTM! Well-defined project-wide constants.The constants are properly documented with clear docstrings. The type hints and naming conventions follow Python best practices.
MAX_RECENT_USAGE_RECORDSprovides a single source of truth for the clamping behavior across the codebase.src/core/common/usage_limits.py (1)
10-30: LGTM! Robust normalization utility with proper defensive checks.The function correctly handles untrusted input by:
- Safely converting to int with fallback to 0 on type errors
- Rejecting non-positive values to enable early-exit optimizations
- Clamping to the project-wide maximum
The implementation is clear, well-documented, and follows the principle of defense-in-depth.
tests/unit/core/services/test_usage_tracking_service_comprehensive.py (3)
12-12: LGTM! Import supports clamping test validation.Importing the constant enables tests to verify that large limits are correctly clamped to the project-wide maximum.
350-370: LGTM! Comprehensive edge-case coverage for invalid limits.These tests verify that:
- Zero limits return empty results immediately
- Negative limits are treated as zero to prevent unintended large responses
- The repository is not called in either case, avoiding wasteful operations
385-409: LGTM! Validates limit clamping with realistic data volume.The test generates a dataset exceeding the maximum and confirms that the service correctly limits the result to
MAX_RECENT_USAGE_RECORDS. This protects against resource exhaustion from excessive requests.src/core/services/usage_tracking_service.py (2)
33-34: LGTM! Imports support limit normalization.The imports provide the necessary constant and utility function for validating and clamping usage limits received from untrusted sources.
346-379: LGTM! Robust limit normalization with appropriate logging.The implementation:
- Safely converts the limit to int with fallback
- Normalizes using the centralized utility
- Returns empty results early for zero limits (optimization)
- Logs when clamping occurs (observability)
- Applies the normalized limit to the final slice
This defense-in-depth approach protects against malicious or malformed requests while providing useful operational insights.
src/core/app/controllers/usage_controller.py (2)
15-16: LGTM! Imports enable limit validation at the controller layer.The imports support the normalization and clamping behavior introduced by this PR.
120-125: LGTM! FastAPI validation provides first line of defense.The
Queryconstraints (ge=0,le=MAX_RECENT_USAGE_RECORDS) enforce valid ranges at the API layer, rejecting malformed requests before they reach application logic. This is an appropriate use of FastAPI's built-in validation.
| @pytest.mark.asyncio | ||
| async def test_get_recent_usage_negative_limit( | ||
| self, controller: UsageController, mock_usage_service: IUsageTrackingService | ||
| ) -> None: | ||
| """Test get_recent_usage with negative limit.""" | ||
|
|
||
| result = await controller.get_recent_usage(limit=-5) | ||
|
|
||
| assert result == [] | ||
| mock_usage_service.get_recent_usage.assert_not_called() |
There was a problem hiding this comment.
Duplicate test function name.
The test function test_get_recent_usage_negative_limit is defined twice (lines 245-254 and lines 389-397). Python will only keep the second definition, causing the first test to be silently ignored. This defeats the purpose of having comprehensive test coverage.
Rename one of these test functions to avoid the conflict. For example:
@pytest.mark.asyncio
-async def test_get_recent_usage_negative_limit(
+async def test_get_recent_usage_negative_limit_returns_empty(
self, controller: UsageController, mock_usage_service: IUsageTrackingService
) -> None:
- """Test get_recent_usage with negative limit."""
+ """Test get_recent_usage with negative limit returns empty result."""
result = await controller.get_recent_usage(limit=-5)
assert result == []
mock_usage_service.get_recent_usage.assert_not_called()🤖 Prompt for AI Agents
In tests/unit/core/app/controllers/test_usage_controller_comprehensive.py around
lines 245-254, the test function name test_get_recent_usage_negative_limit is
duplicated later; rename this occurrence to a unique name (e.g.,
test_get_recent_usage_with_negative_limit_returns_empty) and update any
references or fixtures if needed so both tests coexist without name collision,
then run the test suite to verify both execute.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68ec26d70c8083338f26e9e0caaa409e
Summary by CodeRabbit
New Features
Tests