File size limit - #76
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds centralized document upload validation to enforce a maximum file size and stricter MIME-type checks, while refactoring upload endpoints/services to use the new validation helper.
Changes:
- Extend
validate_document_fileto validate file size + MIME type and save the upload to a temp file. - Update job/culture upload routes and shared document service to use the new
(is_valid, tmp_path, size, type)return signature. - Add DOCX detection fallback when MIME is detected as
application/zip.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
backend/src/api/utils.py |
Adds size validation, centralizes allowed MIME types, enhances type detection, and makes validation return temp-path + metadata. |
backend/src/api/routes/resume_routes.py |
Switches resume upload route to new validate_document_file return signature. |
backend/src/api/routes/job_routes.py |
Switches job and culture document upload routes to use temp-path from validate_document_file (removing duplicate temp save). |
backend/src/services/document_processing/document_service.py |
Switches shared temp-file validation helper to use validate_document_file for temp saving. |
Comments suppressed due to low confidence (2)
backend/src/api/routes/resume_routes.py:38
validate_document_filenow raisesHTTPExceptionon invalid input and currently always returnsTrueforis_valid, so theif not is_valid:branch is effectively unreachable. Consider removing this check (or refactoringvalidate_document_fileto return only the temp path/metadata) to keep the endpoint logic consistent with the new validation contract.
is_valid, tmp_file_path, _, _= await validate_document_file(file)
if not is_valid:
raise HTTPException(
status_code=400,
detail="Invalid file type. Accepted formats: PDF, DOC, DOCX"
)
backend/src/api/routes/resume_routes.py:43
validate_document_filesaves the upload by reading the stream to EOF. Immediately after, this endpoint doesfile_bytes = await file.read(), which will typically return empty bytes (resulting in uploading an empty blob). Either read fromtmp_file_path(and cleanup the temp file in afinally), or rewind the upload stream (await file.seek(0)) before reading it again.
is_valid, tmp_file_path, _, _= await validate_document_file(file)
if not is_valid:
raise HTTPException(
status_code=400,
detail="Invalid file type. Accepted formats: PDF, DOC, DOCX"
)
try:
user_id = current_user["_id"]
file_bytes = await file.read()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Orchestrates the full document ingestion pipeline: validates the uploaded file, | ||
| saves it to a temporary location, and converts it to PDF if necessary. |
There was a problem hiding this comment.
The validate_document_file docstring says it "converts it to PDF if necessary", but the function only validates and saves the file. Please update the docstring to match the actual behavior to avoid misleading future changes/users.
| Orchestrates the full document ingestion pipeline: validates the uploaded file, | |
| saves it to a temporary location, and converts it to PDF if necessary. | |
| Validates the uploaded file, saves it to a temporary location, | |
| and returns the validation result and file metadata. |
|
|
||
| Returns: | ||
| Tuple of (is_valid, file_extension) | ||
| Tuple of (is_valid, file_size_bytes) |
There was a problem hiding this comment.
validate_file_size is annotated to return int, but the docstring says it returns a tuple "(is_valid, file_size_bytes)". Please correct the Returns section so it matches the actual return type/value.
| Tuple of (is_valid, file_size_bytes) | |
| The file size in bytes. |
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Error occurred while validating DOCX file {file.filename}: {e}" |
There was a problem hiding this comment.
The HTTP 400 error detail for DOCX validation includes the raw exception ({e}), which may leak internal parsing details to clients. Prefer logging the exception server-side and returning a generic client-facing message (e.g., "Invalid DOCX file") with a stable error code/context.
| raise HTTPException( | |
| status_code=400, | |
| detail=f"Error occurred while validating DOCX file {file.filename}: {e}" | |
| logger.warning(f"Invalid DOCX file during validation: {file.filename}: {e}") | |
| raise HTTPException( | |
| status_code=400, | |
| detail="Invalid DOCX file." |
|
|
||
| # Save to temporary location | ||
| tmp_file_path = await save_upload_file_tmp(file) | ||
| is_valid, tmp_file_path, _, _ = await validate_document_file(file) |
There was a problem hiding this comment.
validate_document_file now raises on invalid input, so the is_valid return value is unused here. Consider unpacking to _ (or updating validate_document_file to not return a boolean) to reduce confusion and avoid unused variables.
| is_valid, tmp_file_path, _, _ = await validate_document_file(file) | |
| _, tmp_file_path, _, _ = await validate_document_file(file) |
| file_size = await validate_file_size(file) | ||
| file_type = await validate_file_type(file) | ||
|
|
||
| temp_file_path = await save_upload_file_tmp(file) | ||
|
|
||
| return True, temp_file_path, file_size, file_type |
There was a problem hiding this comment.
validate_document_file reads and saves the upload to a temp file via save_upload_file_tmp(file), which consumes the UploadFile stream. Callers that subsequently do await file.read() will get empty bytes unless the stream is rewound. Either rewind the upload (await file.seek(0)) after saving, or change this helper to avoid consuming the stream / return the bytes, so downstream endpoints can still read the file content reliably.
| if detected_mime == 'application/zip': | ||
| try: | ||
| Document(file.file) | ||
| await file.seek(0) | ||
| detected_mime = 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' |
There was a problem hiding this comment.
validate_file_type calls python-docx's Document(...) directly inside an async request handler. This is synchronous and can block the event loop for large/complex files. Consider running DOCX parsing in a threadpool (or using a lightweight ZIP/content-type check) so uploads don't stall other requests.
| status_code=400, | ||
| detail=f"Invalid file type. Accepted formats: PDF, DOC, DOCX" | ||
| ) | ||
| is_valid, tmp_file_path, _, _ = await validate_document_file(file) |
There was a problem hiding this comment.
The is_valid value returned by validate_document_file is assigned but never used. Since the helper now raises HTTPException on invalid input, consider unpacking to _ (or removing the boolean from the helper's return type) to avoid unused variables and make the control flow clearer.
| is_valid, tmp_file_path, _, _ = await validate_document_file(file) | |
| _, tmp_file_path, _, _ = await validate_document_file(file) |
| is_valid, _ = await validate_document_file(file) | ||
| if not is_valid: | ||
| raise HTTPException(status_code=400, detail="Invalid file type. Accepted formats: PDF, DOC, DOCX") | ||
| is_valid, tmp_file_path, _, _ = await validate_document_file(file) |
There was a problem hiding this comment.
The is_valid value returned by validate_document_file is assigned but never used. Since validate_document_file raises on invalid input, consider unpacking to _ (or removing the boolean from the helper return) to avoid unused variables.
| is_valid, tmp_file_path, _, _ = await validate_document_file(file) | |
| _, tmp_file_path, _, _ = await validate_document_file(file) |
No description provided.