collection: include signed url of documents #509
Conversation
📝 WalkthroughWalkthroughcollection_info now accepts an Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Route as collection_info (Route)
participant DB as Collections Store
participant Helper as build_document_schemas
participant Storage as Cloud Storage
Client->>Route: GET /collections/{id}?include_docs&include_url&limit
Route->>DB: read(collection, skip=None, limit)
DB-->>Route: documents[]
alt include_url == true AND documents not empty
Route->>Storage: get_cloud_storage(session, project_id)
Storage-->>Route: storage
Route->>Helper: build_document_schemas(documents, storage, include_url=true)
else
Route->>Helper: build_document_schemas(documents, None, include_url=false)
end
Helper-->>Route: documents_with_schemas
Route-->>Client: 200 collection_info response (documents as built)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/docs/collections/info.mdbackend/app/api/routes/collections.pybackend/app/api/routes/documents.py
🧰 Additional context used
📓 Path-based instructions (3)
backend/app/api/docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Store endpoint documentation files in
backend/app/api/docs/<domain>/<action>.md
Files:
backend/app/api/docs/collections/info.md
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
backend/app/api/**/*.py: Define FastAPI REST endpoints inbackend/app/api/organized by domain
Load Swagger endpoint descriptions from external markdown files instead of inline strings usingload_description("domain/action.md")
Files:
backend/app/api/routes/collections.pybackend/app/api/routes/documents.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/api/routes/collections.pybackend/app/api/routes/documents.py
🧬 Code graph analysis (2)
backend/app/api/routes/collections.py (4)
backend/app/core/cloud/storage.py (1)
get_cloud_storage(261-278)backend/app/services/documents/helpers.py (1)
build_document_schemas(130-142)backend/app/crud/document_collection.py (1)
read(25-48)backend/app/models/auth.py (1)
project_(32-36)
backend/app/api/routes/documents.py (1)
backend/app/core/cloud/storage.py (1)
get_cloud_storage(261-278)
⏰ 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: checks (3.12, 6)
🔇 Additional comments (4)
backend/app/api/routes/documents.py (2)
29-36: LGTM!The import reorganization is clean.
get_cloud_storageis correctly imported fromapp.core.cloud, andbuild_document_schemasis added alongsidebuild_document_schemafor consistent document schema construction across endpoints.
263-264: LGTM!Minor formatting improvement with the blank line before the return statement improves readability.
backend/app/api/routes/collections.py (2)
15-15: LGTM!Imports are correctly added for cloud storage integration and document schema building.
Also applies to: 35-35
203-215: LGTM!The implementation correctly:
- Fetches documents with the optional
limitparameter- Initializes storage conditionally only when
include_urlis true and documents exist (avoiding unnecessary cloud storage calls)- Uses
build_document_schemaswith proper keyword arguments for consistent document schema constructionThis aligns well with the pattern in
documents.pyand the helper function signature frombackend/app/services/documents/helpers.py.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/app/api/routes/collections.py (1)
188-197: Type annotation has been corrected.The
limitparameter now correctly usesint | Noneto match its default value ofNone. This addresses the type annotation issue raised in the previous review.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/docs/collections/info.mdbackend/app/api/routes/collections.pybackend/app/tests/api/routes/collections/test_collection_info.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/docs/collections/info.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/api/routes/collections/test_collection_info.pybackend/app/api/routes/collections.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/api/routes/collections/test_collection_info.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
backend/app/api/**/*.py: Define FastAPI REST endpoints inbackend/app/api/organized by domain
Load Swagger endpoint descriptions from external markdown files instead of inline strings usingload_description("domain/action.md")
Files:
backend/app/api/routes/collections.py
🧬 Code graph analysis (2)
backend/app/tests/api/routes/collections/test_collection_info.py (3)
backend/app/tests/conftest.py (3)
client(65-68)db(28-45)user_api_key_header(90-92)backend/app/tests/utils/utils.py (1)
get_project(54-73)backend/app/tests/utils/collection.py (1)
get_collection(25-47)
backend/app/api/routes/collections.py (3)
backend/app/core/cloud/storage.py (1)
get_cloud_storage(261-278)backend/app/services/documents/helpers.py (1)
build_document_schemas(130-142)backend/app/crud/document_collection.py (1)
read(25-48)
🪛 Ruff (0.14.10)
backend/app/tests/api/routes/collections/test_collection_info.py
202-202: Local variable document is assigned to but never used
Remove assignment to unused variable document
(F841)
⏰ 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: checks (3.12, 6)
🔇 Additional comments (1)
backend/app/api/routes/collections.py (1)
208-216: Conditional storage initialization and centralized schema building look good.The logic correctly initializes storage only when
include_urlis true and documents exist, avoiding unnecessary overhead. The centralizedbuild_document_schemashelper properly handles document transformation with optional signed URLs.
backend/app/tests/api/routes/collections/test_collection_info.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/api/routes/collections.py (1)
180-198: Add return type hint to align with coding guidelines.The function signature lacks a return type annotation. As per coding guidelines, "Always add type hints to all function parameters and return values in Python code."
While the
response_modeldecorator documents the API contract, an explicit return type improves type-checking and IDE support.🔎 Proposed fix
def collection_info( session: SessionDep, current_user: AuthContextDep, collection_id: UUID = FastPath(description="Collection to retrieve"), include_docs: bool = Query( True, description="If true, include documents linked to this collection", ), include_url: bool = Query( False, description="Include a signed URL to access the document" ), limit: int | None = Query( None, gt=0, le=500, description="Limit number of documents returned (default: all, max: 500)", ), -): +) -> APIResponse[CollectionWithDocsPublic]:Based on coding guidelines.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/api/routes/collections.pybackend/app/tests/api/routes/collections/test_collection_info.py
🧰 Additional context used
📓 Path-based instructions (3)
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
backend/app/api/**/*.py: Define FastAPI REST endpoints inbackend/app/api/organized by domain
Load Swagger endpoint descriptions from external markdown files instead of inline strings usingload_description("domain/action.md")
Files:
backend/app/api/routes/collections.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/api/routes/collections.pybackend/app/tests/api/routes/collections/test_collection_info.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/api/routes/collections/test_collection_info.py
🧬 Code graph analysis (2)
backend/app/api/routes/collections.py (3)
backend/app/core/cloud/storage.py (1)
get_cloud_storage(261-278)backend/app/services/documents/helpers.py (1)
build_document_schemas(130-142)backend/app/crud/document_collection.py (1)
read(25-48)
backend/app/tests/api/routes/collections/test_collection_info.py (3)
backend/app/tests/conftest.py (3)
client(65-68)db(28-45)user_api_key_header(90-92)backend/app/tests/utils/utils.py (1)
get_project(54-73)backend/app/tests/utils/collection.py (1)
get_collection(25-47)
⏰ 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: checks (3.12, 6)
🔇 Additional comments (2)
backend/app/tests/api/routes/collections/test_collection_info.py (1)
190-226: LGTM! Well-structured test with comprehensive validation.The test correctly validates the new
include_urlfunctionality:
- Proper setup with linked document
- API call with both
include_docsandinclude_urlparameters- Verification that the specific linked document is returned with a valid signed URL
All previous review feedback has been addressed—the return type hint is present and the linked document is properly verified in the response.
backend/app/api/routes/collections.py (1)
206-216: LGTM! Efficient implementation with correct parameter passing.The implementation correctly addresses previous review feedback:
- Explicit keyword arguments (
skip=None, limit=limit) ensure proper pagination behavior- Storage is lazily initialized only when
include_url=Trueand documents exist, avoiding unnecessary overhead- The
build_document_schemascall properly delegates document schema construction with appropriate parameters
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/api/routes/collections.py (1)
188-216: Implementation looks correct; consider adding return type annotation.The implementation properly addresses the missing signed URL functionality:
- Parameters are correctly typed (
int | Noneforlimit)- Documents are fetched with explicit
skip=None, limit=limit(avoiding the previous bug)- Storage is efficiently initialized only when needed (
include_url=Trueand documents exist)build_document_schemasis called with correct keyword argumentsThe logic flow is sound and handles edge cases appropriately.
Optional: Add return type annotation per coding guidelines
According to the coding guidelines, all functions should have return type annotations. While FastAPI's
response_modelin the decorator specifies the response type, adding an explicit annotation improves type checking:def collection_info( session: SessionDep, current_user: AuthContextDep, collection_id: UUID = FastPath(description="Collection to retrieve"), include_docs: bool = Query( True, description="If true, include documents linked to this collection", ), include_url: bool = Query( True, description="Include a signed URL to access the document" ), limit: int | None = Query( None, gt=0, le=500, description="Limit number of documents returned (default: all, max: 500)", ), -): +) -> APIResponse[CollectionWithDocsPublic]:This is a very minor refinement and can be deferred.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/routes/collections.py
🧰 Additional context used
📓 Path-based instructions (2)
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
backend/app/api/**/*.py: Define FastAPI REST endpoints inbackend/app/api/organized by domain
Load Swagger endpoint descriptions from external markdown files instead of inline strings usingload_description("domain/action.md")
Files:
backend/app/api/routes/collections.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/api/routes/collections.py
🧬 Code graph analysis (1)
backend/app/api/routes/collections.py (3)
backend/app/core/cloud/storage.py (1)
get_cloud_storage(261-278)backend/app/services/documents/helpers.py (1)
build_document_schemas(130-142)backend/app/crud/document_collection.py (1)
read(25-48)
⏰ 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: checks (3.12, 6)
Summary
Explain the motivation for making this change. What existing problem does the pull request solve?
The
DocumentPublicresponse model includes asigned_urlfield, but the collection info endpoint had no logic to populate it. Theinclude_urlparameter existed but was never used. In this pull request, Implemented the missing URL generation logic by fetching storage and callingbuild_document_schemaswith theinclude_urlparameter.Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.