fpstore API #203
fpstore API #203
Conversation
WalkthroughA new Starlette web application is introduced with three endpoints for health checks, fingerprint retrieval, and fingerprint search. Supporting test fixtures and test cases are added for these endpoints. Error handling is centralized via new exception handlers and updated import paths. Minor refactoring aligns imports to a shared utility module. Changes
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
acoustid/future/fpstore/app.py(1 hunks)tests/fpstore/conftest.py(1 hunks)tests/fpstore/test_utils.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/fpstore/test_utils.py (1)
tests/fpstore/conftest.py (1)
client(14-15)
tests/fpstore/conftest.py (1)
acoustid/future/fpstore/app.py (1)
create_app(16-22)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
acoustid/future/fpstore/app.py (2)
7-8: Health endpoint looks good.Simple and effective health check implementation. Returns exactly what monitoring systems expect.
16-22: App factory function is well structured.Clean separation with a factory function makes testing easier and follows good practices for application initialization.
tests/fpstore/conftest.py (1)
8-15: Fixtures are properly implemented.Standard pytest fixture pattern with correct dependency injection. The app and client fixtures provide clean test isolation and follow testing best practices.
acoustid/future/fpstore/app.py
Outdated
| def handle_get_fingerprint(request: Request) -> Response: | ||
| fingerprint_id = request.path_params["id"] | ||
| return Response(content=fingerprint_id, media_type="text/plain") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and consider security implications.
The fingerprint endpoint directly returns the path parameter without any validation or sanitization. This could lead to issues if the ID contains unexpected characters or if there are length constraints.
Consider adding:
- Input validation (format, length, allowed characters)
- Proper error handling for invalid IDs
- Sanitization before returning the response
def handle_get_fingerprint(request: Request) -> Response:
fingerprint_id = request.path_params["id"]
+ # Add validation here
+ if not fingerprint_id or len(fingerprint_id) > 255:
+ return Response(content="Invalid fingerprint ID", status_code=400)
return Response(content=fingerprint_id, media_type="text/plain")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def handle_get_fingerprint(request: Request) -> Response: | |
| fingerprint_id = request.path_params["id"] | |
| return Response(content=fingerprint_id, media_type="text/plain") | |
| def handle_get_fingerprint(request: Request) -> Response: | |
| fingerprint_id = request.path_params["id"] | |
| # Add validation here | |
| if not fingerprint_id or len(fingerprint_id) > 255: | |
| return Response(content="Invalid fingerprint ID", status_code=400) | |
| return Response(content=fingerprint_id, media_type="text/plain") |
🤖 Prompt for AI Agents
In acoustid/future/fpstore/app.py around lines 11 to 13, the function
handle_get_fingerprint returns the path parameter directly without validation or
sanitization. To fix this, add input validation to check the fingerprint_id
format, length, and allowed characters; implement error handling to return an
appropriate response if the ID is invalid; and sanitize the fingerprint_id
before including it in the response to prevent security issues.
| def test_health(client: TestClient) -> None: | ||
| response = client.get("/health") | ||
| assert response.status_code == 200 | ||
| assert response.text == "OK" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage for the fingerprint endpoint.
The health endpoint test is solid, but you're missing tests for the main functionality. The fingerprint endpoint should be tested with various scenarios.
Add tests for the fingerprint endpoint:
def test_get_fingerprint(client: TestClient) -> None:
response = client.get("/v2/fingerprint/test123")
assert response.status_code == 200
assert response.text == "test123"
def test_get_fingerprint_with_special_chars(client: TestClient) -> None:
fingerprint_id = "abc-123_xyz"
response = client.get(f"/v2/fingerprint/{fingerprint_id}")
assert response.status_code == 200
assert response.text == fingerprint_id🤖 Prompt for AI Agents
In tests/fpstore/test_utils.py around lines 4 to 7, the current test only covers
the /health endpoint. To improve test coverage, add new test functions for the
/v2/fingerprint endpoint that verify correct responses for typical fingerprint
IDs and those containing special characters. Implement tests that send GET
requests to /v2/fingerprint/{id} with sample IDs and assert the status code is
200 and the response text matches the requested fingerprint ID.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
acoustid/future/fpstore/app.py (1)
17-21: Remove unnecessary async keyword.The function doesn't need to be async since it only awaits
request.body()which could be handled synchronously for this simple case, or you could keep it async for consistency with Starlette patterns.The past review comment about path parameter validation doesn't apply here since this implementation uses JSON body parsing with msgspec, which handles validation automatically.
-async def handle_get_fingerprint(request: Request) -> Response: - raw_body = await request.body() +def handle_get_fingerprint(request: Request) -> Response: + raw_body = request.body() body = msgspec.json.decode(raw_body, type=GetFingerprintRequest) fingerprint_id = body.fingerprint_id return Response(content=str(fingerprint_id), media_type="text/plain")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
acoustid/future/api/app.py(1 hunks)acoustid/future/api/utils.py(0 hunks)acoustid/future/fpstore/app.py(1 hunks)tests/fpstore/test_get_fingerprint.py(1 hunks)
💤 Files with no reviewable changes (1)
- acoustid/future/api/utils.py
✅ Files skipped from review due to trivial changes (1)
- acoustid/future/api/app.py
🧰 Additional context used
🪛 GitHub Actions: Build and test
acoustid/future/fpstore/app.py
[error] 1-1: Imports are incorrectly sorted and/or formatted.
🔇 Additional comments (6)
tests/fpstore/test_get_fingerprint.py (3)
2-8: Test looks solid for the happy path.Good coverage of the successful case with proper assertions on both status code and response content.
11-17: Good validation testing.This properly tests the missing required field scenario. The specific error message assertion ensures the API provides clear feedback for validation failures.
20-26: Solid error handling test.Using
datainstead ofjsonparameter is the right way to send malformed JSON. The test ensures proper error messages for parsing failures.acoustid/future/fpstore/app.py (3)
9-10: Health endpoint is straightforward and correct.Simple health check implementation that serves its purpose well.
13-14: Clean request structure definition.Using msgspec.Struct provides automatic validation and type safety for the API endpoint.
24-31: App setup looks good.Routes are properly defined with correct HTTP methods, and using shared exception handlers ensures consistent error responses across the API.
| import msgspec | ||
| from starlette.applications import Starlette | ||
| from starlette.requests import Request | ||
| from starlette.responses import Response | ||
| from starlette.routing import Route | ||
| from ..apiutils import default_exception_handlers |
There was a problem hiding this comment.
Fix import sorting to resolve pipeline failure.
The imports need to be reordered to satisfy the linting rules.
import msgspec
+
from starlette.applications import Starlette
from starlette.requests import Request
from starlette.responses import Response
from starlette.routing import Route
+
from ..apiutils import default_exception_handlersCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Build and test
[error] 1-1: Imports are incorrectly sorted and/or formatted.
🤖 Prompt for AI Agents
In acoustid/future/fpstore/app.py at lines 1 to 6, the import statements are not
sorted according to the linting rules, causing pipeline failure. Reorder the
imports alphabetically and group them properly, typically standard library
imports first, then third-party imports, and finally local imports, to satisfy
the linting requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
acoustid/future/apiutils.py (2)
5-6: Consider if these aliases are necessary.The aliases might be overkill if they're only used in the handlers dictionary below. Direct imports in the handlers dict would be cleaner unless these aliases serve a broader purpose across the codebase.
20-21: Eliminate code duplication between error handlers.Both
on_validation_errorandon_decode_errorare identical except for their names. This violates DRY principles and creates unnecessary maintenance overhead.Refactor to a single generic handler:
-def on_validation_error(_: Request, exc: Exception) -> Response: - return MsgspecResponse(status_code=400, content=ErrorResponse(error=str(exc))) - - -def on_decode_error(_: Request, exc: Exception) -> Response: - return MsgspecResponse(status_code=400, content=ErrorResponse(error=str(exc))) +def on_client_error(_: Request, exc: Exception) -> Response: + return MsgspecResponse(status_code=400, content=ErrorResponse(error=str(exc)))Then update the handlers dictionary:
default_exception_handlers = { - msgspec.ValidationError: on_validation_error, - msgspec.DecodeError: on_decode_error, + msgspec.ValidationError: on_client_error, + msgspec.DecodeError: on_client_error, }Also applies to: 24-26
tests/fpstore/test_search.py (1)
1-7: Basic test works but could be more robust.The test correctly verifies the endpoint responds, but it's testing what appears to be a stub implementation. The hardcoded
fingerprint_id: 123and expected"OK"response suggest this endpoint isn't fully implemented yet.Consider expanding this test once the real implementation is in place:
def test_search_success(client): response = client.post( "/v2/fingerprint/_search", json={"fingerprint_id": 123}, ) assert response.status_code == 200 - assert response.text == "OK" + # TODO: Replace with actual expected response format + data = response.json() + assert "results" in data + +def test_search_invalid_payload(client): + response = client.post( + "/v2/fingerprint/_search", + json={"invalid_field": "value"}, + ) + assert response.status_code == 400 + +def test_search_missing_fingerprint_id(client): + response = client.post( + "/v2/fingerprint/_search", + json={}, + ) + assert response.status_code == 400
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
acoustid/future/api/app.py(1 hunks)acoustid/future/api/handlers/monitoring.py(1 hunks)acoustid/future/api/handlers/submit.py(1 hunks)acoustid/future/apiutils.py(2 hunks)acoustid/future/fpstore/app.py(1 hunks)tests/fpstore/test_get_fingerprint.py(1 hunks)tests/fpstore/test_search.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- acoustid/future/api/handlers/submit.py
- acoustid/future/api/handlers/monitoring.py
🚧 Files skipped from review as they are similar to previous changes (3)
- acoustid/future/api/app.py
- tests/fpstore/test_get_fingerprint.py
- acoustid/future/fpstore/app.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
acoustid/future/apiutils.py (1)
32-35: Good approach to centralized exception handling.The handlers dictionary provides a clean way to register exception handlers with Starlette. This makes the error handling configuration explicit and easy to extend.
1 participant
No description provided.