Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR updates dependencies and Python version in Pipfile, integrates MCP via fastmcp in the app factory, refactors graph routes to delegate to core APIs with standardized error handling, adjusts a schema loader function to accept an optional prefix, and tags the database connect route for MCP tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Env as Environment
participant App as FastAPI App
participant FM as FastMCP
participant MCP as MCP HTTP App (/mcp)
Env->>App: Start process
App->>App: Initialize base FastAPI and include routers
Env-->>App: Read DISABLE_MCP
alt MCP enabled
App->>FM: FastMCP.from_fastapi(app, route_maps=[resources, templates, tools, exclude])
FM-->>App: FastMCP instance
App->>FM: build_http_app()
FM-->>MCP: MCP ASGI app
App->>App: mount("/mcp", MCP)
App->>App: merge MCP routes with main app
else MCP disabled
App->>App: Skip MCP setup (log)
end
App-->>Env: Ready to serve
sequenceDiagram
autonumber
participant Client
participant API as Graph Routes (api/routes/graphs.py)
participant Core as Core Services
Client->>API: GET /graphs
API->>Core: list_databases(user_id, GENERAL_PREFIX?)
Core-->>API: list[str] or error
API-->>Client: 200 list or mapped error
Client->>API: GET /graphs/{id}
API->>Core: get_schema(user_id, id)
Core-->>API: schema or GraphNotFound/InternalError
API-->>Client: 200/404/500
Client->>API: POST /graphs/{id}/query
API->>Core: query_database(user_id, id, chat_data)
Core-->>API: streamed JSON or InvalidArgument
API-->>Client: 200 stream or 400
Client->>API: POST /graphs/{id}/confirm
API->>Core: execute_destructive_operation(...)
Core-->>API: streamed JSON or InvalidArgument
API-->>Client: 200 stream or 400
Client->>API: POST /graphs/{id}/refresh
API->>Core: refresh_database_schema(...)
Core-->>API: ok or InternalError
API-->>Client: 200 or 500
Client->>API: DELETE /graphs/{id}
API->>Core: delete_database(...)
Core-->>API: ok or InvalidArgument/GraphNotFound/InternalError
API-->>Client: 200/400/404/500
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Dependency ReviewThe following issues were found:
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/routes/graphs.py (1)
130-146: Add missing error handling for not-found and internal errorsAlign with get_graph_data and delete_graph for consistent UX.
async def query_graph( request: Request, graph_id: str, chat_data: ChatRequest ): # pylint: disable=too-many-statements @@ try: generator = await query_database(request.state.user_id, graph_id, chat_data) return StreamingResponse(generator, media_type="application/json") except InvalidArgumentError as iae: return JSONResponse(content={"error": str(iae)}, status_code=400) + except GraphNotFoundError as gnfe: + return JSONResponse(content={"error": str(gnfe)}, status_code=404) + except InternalError as ie: + return JSONResponse(content={"error": str(ie)}, status_code=500)api/routes/database.py (1)
28-35: Catch early URL validation errors; consider NDJSON content type
- load_database can raise InvalidArgumentError before returning a generator; return 400 instead of 500.
- The stream is delimiter-separated JSON; using application/x-ndjson can be clearer for clients.
-from api.core.schema_loader import load_database +from api.core.schema_loader import load_database +from api.core.errors import InvalidArgumentError @@ - generator = await load_database(db_request.url, request.state.user_id) - return StreamingResponse(generator, media_type="application/json") + try: + generator = await load_database(db_request.url, request.state.user_id) + except InvalidArgumentError as iae: + return JSONResponse(content={"error": str(iae)}, status_code=400) + # If you keep the custom delimiter, NDJSON is a clearer content type for clients parsing line-by-line. + return StreamingResponse(generator, media_type="application/x-ndjson")Pipfile (1)
29-30: Regenerate Pipfile.lock to update its python_version to 3.12.
🧹 Nitpick comments (6)
api/routes/graphs.py (3)
83-86: Fix Ruff issues: unusedrequestandFile(None)default
- Rename unused
requestto_requestto satisfy ARG001.- Keep
File(None)(FastAPI param marker) but silence Ruff B008 inline.Apply:
-async def load_graph( - request: Request, data: GraphData = None, file: UploadFile = File(None) -): # pylint: disable=unused-argument +async def load_graph( + _request: Request, data: GraphData | None = None, file: UploadFile | None = File(None) # noqa: B008 +): # pylint: disable=unused-argument
101-119: Harden extension detectionHandle case-insensitive extensions and empty filenames.
- filename = file.filename + filename = (file.filename or "").lower()
168-180: Return 404 on missing graph during refreshAdd GraphNotFoundError handling.
try: return await refresh_database_schema(request.state.user_id, graph_id) - except InternalError as ie: + except GraphNotFoundError as gnfe: + return JSONResponse(content={"error": str(gnfe)}, status_code=404) + except InternalError as ie: return JSONResponse(content={"error": str(ie)}, status_code=500)api/app_factory.py (2)
168-176: Harden prod settings via env toggles
- trusted_hosts="*" is permissive. Consider reading from an env var in prod.
- Set SessionMiddleware https_only=True in staging/prod via env.
- app.add_middleware(ProxyHeadersMiddleware, trusted_hosts="*") + trusted = os.getenv("TRUSTED_HOSTS", "*") + app.add_middleware(ProxyHeadersMiddleware, trusted_hosts=trusted) @@ - https_only=False, # True for HTTPS environments (staging/prod), False for HTTP dev + https_only=os.getenv("SESSION_COOKIE_SECURE", "false").lower() in ("1", "true", "yes"),
192-195: Silence Ruff ARG001: unused request paramRename to
_request.- async def handle_oauth_error( - request: Request, exc: Exception - ): # pylint: disable=unused-argument + async def handle_oauth_error( + _request: Request, exc: Exception + ): # pylint: disable=unused-argumentapi/core/schema_loader.py (1)
144-163: Acknowledge intended namespace behavior; optional dedupe and sort
- The mix of stripped user graphs and prefixed general graphs matches
_graph_namelogic and downstream expectations.- For stable ordering and to remove duplicates, consider returning
sorted(set(filtered_graphs)).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Pipfile(1 hunks)api/app_factory.py(6 hunks)api/core/schema_loader.py(2 hunks)api/routes/database.py(1 hunks)api/routes/graphs.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.13.3)
api/routes/graphs.py
84-84: Unused function argument: request
(ARG001)
84-84: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
api/app_factory.py
193-193: Unused function argument: request
(ARG001)
⏰ 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: e2e-tests
🔇 Additional comments (5)
api/routes/graphs.py (2)
37-42: MCP tagging looks correctTagging the list route with "mcp_tool" aligns with the FastMCP route map.
193-202: Delete route handling is complete and consistentCovers 400/404/500; good parity with other endpoints.
api/routes/database.py (1)
24-26: MCP tag added correctlyThis makes the connect route discoverable as a tool.
Pipfile (1)
20-20: fastmcp addition aligns with app integrationVersion pin looks good.
api/app_factory.py (1)
12-14: FastMCP imports and usage look correctGood switch from FastAPI_MCP to FastMCP.
1 participant
Summary by CodeRabbit