From adb0b2329e8cf328e2a569f4ab9b6df6a3afdab9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Feb 2026 22:35:30 +0000 Subject: [PATCH] Address code review feedback - Replace LIKE queries with exact matches in fallback lookups - Add comments clarifying fallback queries should not be needed - Fix duplicate comment in embed_metadata_into_pdf - Add missing file_id parameter to log_task_progress call Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- app/tasks/embed_metadata_into_pdf.py | 9 ++++----- app/tasks/finalize_document_storage.py | 8 +++++--- app/tasks/send_to_all.py | 6 ++++-- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/app/tasks/embed_metadata_into_pdf.py b/app/tasks/embed_metadata_into_pdf.py index 2ae1cacc..9c3e8da4 100644 --- a/app/tasks/embed_metadata_into_pdf.py +++ b/app/tasks/embed_metadata_into_pdf.py @@ -64,23 +64,22 @@ def embed_metadata_into_pdf(self, local_file_path: str, extracted_text: str, met logger.info(f"[{task_id}] Starting metadata embedding for: {local_file_path}") log_task_progress(task_id, "embed_metadata_into_pdf", "in_progress", f"Embedding metadata into {os.path.basename(local_file_path)}", file_id=file_id) - # Get file_id from database if not provided + # Get file_id from database if not provided (fallback only, prefer passing file_id explicitly) if file_id is None: with SessionLocal() as db: file_record = db.query(FileRecord).filter_by(local_filename=local_file_path).first() if file_record: file_id = file_record.id + + # Check for file existence; if not found, try the known shared tmp directory. if not os.path.exists(local_file_path): alt_path = os.path.join(settings.workdir, "tmp", os.path.basename(local_file_path)) if os.path.exists(alt_path): local_file_path = alt_path else: logger.error(f"[{task_id}] Local file {local_file_path} not found, cannot embed metadata.") - log_task_progress(task_id, "embed_metadata_into_pdf", "failure", "File not found") + log_task_progress(task_id, "embed_metadata_into_pdf", "failure", "File not found", file_id=file_id) return {"error": "File not found"} - - - # Check for file existence; if not found, try the known shared tmp directory. # Work on a safe copy in a secure temporary directory original_file = local_file_path diff --git a/app/tasks/finalize_document_storage.py b/app/tasks/finalize_document_storage.py index c3f7b23e..05ad24e1 100644 --- a/app/tasks/finalize_document_storage.py +++ b/app/tasks/finalize_document_storage.py @@ -26,12 +26,14 @@ def finalize_document_storage(self, original_file: str, processed_file: str, met logger.info(f"[{task_id}] Finalizing document storage for {processed_file}") log_task_progress(task_id, "finalize_document_storage", "in_progress", f"Finalizing: {os.path.basename(processed_file)}", file_id=file_id) - # Get file_id from database if not provided + # Get file_id from database if not provided (fallback only, prefer passing file_id explicitly) if file_id is None: with SessionLocal() as db: - # Try to find by the original file path + # Only as a last resort, try to find by exact match on local_filename + # This should not be needed if file_id is passed correctly through the chain + tmp_path = os.path.join(settings.workdir, "tmp", os.path.basename(original_file)) file_record = db.query(FileRecord).filter( - FileRecord.local_filename.like(f"%{os.path.basename(original_file)}%") + FileRecord.local_filename == tmp_path ).first() if file_record: file_id = file_record.id diff --git a/app/tasks/send_to_all.py b/app/tasks/send_to_all.py index 465a7058..1f284c82 100644 --- a/app/tasks/send_to_all.py +++ b/app/tasks/send_to_all.py @@ -128,11 +128,13 @@ def send_to_all_destinations(self, file_path: str, use_validator=True, file_id: logger.info(f"[{task_id}] Sending {file_path} to all configured destinations") log_task_progress(task_id, "send_to_all_destinations", "in_progress", f"Distributing: {os.path.basename(file_path)}", file_id=file_id) - # Get file_id from database if not provided + # Get file_id from database if not provided (fallback only, prefer passing file_id explicitly) if file_id is None: with SessionLocal() as db: + # Only as a last resort, try to find by basename match + # This should not be needed if file_id is passed correctly through the chain file_record = db.query(FileRecord).filter( - FileRecord.local_filename.like(f"%{os.path.basename(file_path)}%") + FileRecord.local_filename == os.path.join(settings.workdir, "tmp", os.path.basename(file_path)) ).first() if file_record: file_id = file_record.id