Conversation
🔍 PR Validation Results
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the routing structure to introduce API versioning and better organization by extracting routes from the main application file into dedicated router modules.
Key Changes:
- Introduces API versioning with
/api/v1prefix for all document-related endpoints - Extracts base routes (
/and/health) from main.py into a dedicated base router - Consolidates versioned API routes under a centralized router in
__init__.py
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| RAGManager/main.py | Reorganizes router imports and inclusion; removes inline route definitions and unused imports |
| RAGManager/app/api/routes/base.py | New file containing unversioned base routes (root and health check) extracted from main.py |
| RAGManager/app/api/routes/init.py | Creates versioned API router (/api/v1) that aggregates feature-specific routers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @router.get("/") | ||
| async def root(): | ||
| return {"message": "RAGManager up"} |
There was a problem hiding this comment.
The root endpoint response message has changed from "Hello World" to "RAGManager up". This is a breaking change that could affect clients expecting the original message. Consider maintaining backward compatibility or documenting this as a breaking change.
| return {"message": "RAGManager up"} | |
| return {"message": "Hello World"} |
| router = APIRouter(prefix="/api/v1") | ||
|
|
||
| router.include_router(documents.router) No newline at end of file |
There was a problem hiding this comment.
The API versioning prefix "/api/v1" introduces a breaking change. The documents endpoint will now be accessible at "/api/v1/documents/..." instead of "/documents/...". Existing clients will receive 404 errors unless they update their endpoints. Consider implementing redirects from old paths or documenting this breaking change clearly in release notes.
| router = APIRouter(prefix="/api/v1") | |
| router.include_router(documents.router) | |
| # Root router (no prefix) to preserve existing unversioned endpoints, | |
| # e.g. /documents/... | |
| router = APIRouter() | |
| # Versioned router for new clients, e.g. /api/v1/documents/... | |
| versioned_router = APIRouter(prefix="/api/v1") | |
| # Include document routes both unversioned and versioned | |
| router.include_router(documents.router) | |
| versioned_router.include_router(documents.router) | |
| # Mount the versioned router under the root router | |
| router.include_router(versioned_router) |
|
|
||
| @router.get("/health") | ||
| def health_check(): | ||
| return {"message": "200 running..."} |
There was a problem hiding this comment.
The health check response message has changed from "200 corriendo..." to "200 running...". This is a breaking change that could affect clients expecting the original Spanish message. Consider maintaining backward compatibility or documenting this as a breaking change.
| return {"message": "200 running..."} | |
| return {"message": "200 corriendo..."} |
No description provided.