From 90d69979dcbe2a105d69517842640008d3d9f616 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 7 Feb 2026 21:56:38 +0000 Subject: [PATCH] Fix S3 upload error: Pass file_id as keyword argument in Celery tasks Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- app/tasks/embed_metadata_into_pdf.py | 2 +- app/tasks/send_to_all.py | 2 +- tests/test_upload_tasks.py | 68 +++++++++++++++++++++++++++- 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/app/tasks/embed_metadata_into_pdf.py b/app/tasks/embed_metadata_into_pdf.py index a33ae8c1..bd49b8d4 100644 --- a/app/tasks/embed_metadata_into_pdf.py +++ b/app/tasks/embed_metadata_into_pdf.py @@ -157,7 +157,7 @@ def embed_metadata_into_pdf(self, local_file_path: str, extracted_text: str, met # Trigger the next step: final storage. logger.info(f"[{task_id}] Queueing final storage task") log_task_progress(task_id, "embed_metadata_into_pdf", "success", "Metadata embedded, queuing finalization", file_id=file_id) - finalize_document_storage.delay(original_file, final_file_path, metadata, file_id) + finalize_document_storage.delay(original_file, final_file_path, metadata, file_id=file_id) # After triggering final storage, delete the original file if it is in workdir/tmp. workdir_tmp = os.path.join(settings.workdir, TMP_SUBDIR) diff --git a/app/tasks/send_to_all.py b/app/tasks/send_to_all.py index 33a9774f..e037e078 100644 --- a/app/tasks/send_to_all.py +++ b/app/tasks/send_to_all.py @@ -228,7 +228,7 @@ def send_to_all_destinations(self, file_path: str, use_validator=True, file_id: logger.info(f"[{task_id}] Queueing {file_path} for {service_name} upload") log_task_progress(task_id, f"queue_{service_name}", "in_progress", f"Queueing upload to {service_name}", file_id=file_id) try: - task = service["upload_func"].delay(file_path, file_id) + task = service["upload_func"].delay(file_path, file_id=file_id) results[f"{service_name}_task_id"] = task.id queued_count += 1 log_task_progress(task_id, f"queue_{service_name}", "success", f"Queued for {service_name}", file_id=file_id) diff --git a/tests/test_upload_tasks.py b/tests/test_upload_tasks.py index 89419ee2..db621933 100644 --- a/tests/test_upload_tasks.py +++ b/tests/test_upload_tasks.py @@ -421,7 +421,7 @@ def test_all_upload_tasks_have_consistent_signature(sample_text_file): """Test that all upload tasks accept file_id as a keyword parameter. This test verifies that all upload tasks can be called with the same signature - as used in send_to_all.py: task.delay(file_path, file_id) + as used in send_to_all.py: task.delay(file_path, file_id=file_id) Note: We use task.run to inspect the actual function signature because Celery tasks wrap the original function, and .run provides access to @@ -451,3 +451,69 @@ def test_all_upload_tasks_have_consistent_signature(sample_text_file): # file_id should have a default value (None) assert sig.parameters["file_id"].default is None, f"{task.name} file_id should default to None" + + +@pytest.mark.unit +def test_send_to_all_calls_upload_tasks_with_keyword_argument(): + """Test that send_to_all_destinations calls upload tasks with file_id as keyword argument. + + Regression test for issue: upload_to_s3() takes 1 positional argument but 2 were given. + This ensures that file_id is always passed as a keyword argument, not positional. + """ + from app.tasks.send_to_all import send_to_all_destinations + + test_file = "/tmp/test_file.pdf" + + # Create the test file + with open(test_file, "w") as f: + f.write("test content") + + try: + # Mock all the upload functions and settings + with patch("app.tasks.send_to_all.upload_to_s3") as mock_s3, \ + patch("app.tasks.send_to_all.settings") as mock_settings, \ + patch("app.tasks.send_to_all.log_task_progress"), \ + patch("app.tasks.send_to_all.SessionLocal"), \ + patch("app.tasks.send_to_all.get_configured_services_from_validator") as mock_validator: + + # Configure validator to return S3 as configured + mock_validator.return_value = {"s3": True} + + # Configure settings to enable only S3 + mock_settings.s3_bucket_name = "test-bucket" + mock_settings.aws_access_key_id = "test-key" + mock_settings.aws_secret_access_key = "test-secret" + mock_settings.dropbox_app_key = None + mock_settings.nextcloud_upload_url = None + mock_settings.paperless_ngx_api_token = None + mock_settings.google_drive_credentials_json = None + mock_settings.webdav_url = None + mock_settings.ftp_host = None + mock_settings.sftp_host = None + mock_settings.email_host = None + mock_settings.onedrive_client_id = None + mock_settings.workdir = "/tmp" + + # Mock the delay method to track how it's called + mock_s3_task = Mock() + mock_s3_task.id = "test-task-id" + mock_s3.delay.return_value = mock_s3_task + + # Call send_to_all_destinations with file_id + result = send_to_all_destinations.apply(args=[test_file], kwargs={"file_id": 123}).get() + + # Verify that upload_to_s3.delay was called with file_id as keyword argument + mock_s3.delay.assert_called_once() + call_args, call_kwargs = mock_s3.delay.call_args + + # The call should be: delay(file_path, file_id=file_id) + # So we expect 1 positional arg (file_path) and file_id in kwargs + assert len(call_args) == 1, "Should have exactly 1 positional argument (file_path)" + assert call_args[0] == test_file, "First positional arg should be file_path" + assert "file_id" in call_kwargs, "file_id should be passed as keyword argument" + assert call_kwargs["file_id"] == 123, "file_id value should be correct" + + finally: + # Clean up test file + if os.path.exists(test_file): + os.remove(test_file)