Address PR comments

Author: saquino0827

backend/src/documents/upload_document.py

@@ -3,37 +3,22 @@
 import uuid
 
 from src import context
-from src.external.aws.s3 import S3
-from src.storage import CloudStorage, CloudStorageException
+from src.storage import CloudStorage
 
-appContext = context.ApplicationContext()
-appContext.register(CloudStorage, S3())
 
+def upload_file_data(file_name, file_content, bucket_name, default_folder) -> str:
+    decoded_file_content = decode_file_content(file_content)
+    secure_filename, document_id = generate_secure_filename(file_name)
+    upload_file_to_cloud(decoded_file_content, secure_filename, file_name, bucket_name, default_folder)
+    return document_id
 
-def upload_file_data(body, bucket_name, default_folder) -> str:
-    filedata = generate_file_data(body)
-    upload_to_s3(filedata, bucket_name, default_folder)
-    return filedata.get("document_id")
-
-
-def generate_file_data(body):
-    if not all(k in body for k in ["file_content", "file_name"]):
-        raise ValueError("Missing required fields")
 
+def decode_file_content(file_content) -> bytes:
     try:
-        decoded_file_data = base64.b64decode(body["file_content"])
+        decoded_file_data = base64.b64decode(file_content)
     except Exception as e:
         raise TypeError("Invalid file content encoding") from e
-
-    original_filename = body["file_name"]
-    secure_filename, document_id = generate_secure_filename(original_filename)
-    file_data = {
-        "secure_filename": secure_filename,
-        "original_filename": original_filename,
-        "decoded_file_data": decoded_file_data,
-        "document_id": document_id,
-    }
-    return file_data
+    return decoded_file_data
 
 
 def generate_secure_filename(original_filename):
@@ -47,16 +32,15 @@ def generate_secure_filename(original_filename):
     return f"{document_id}{ext}", document_id
 
 
-def upload_to_s3(file_data: dict, bucket_name, default_folder):
-    try:
-        s3_key = f"{default_folder}{file_data['secure_filename']}"
-        put_object(
-            bucket_name, s3_key, file_data["decoded_file_data"], {"original_filename": file_data["original_filename"]}
-        )
-    except CloudStorageException as e:
-        raise CloudStorageException() from e
-
-
 @context.inject
-def put_object(bucket_name, key, body, metadata, cloud_storage: CloudStorage = None):
-    return cloud_storage.put_object(bucket_name, key, body, metadata)
+def upload_file_to_cloud(
+    decoded_file_content,
+    secure_file_name,
+    original_file_name,
+    bucket_name,
+    default_folder,
+    cloud_storage: CloudStorage = None,
+):
+    key = f"{default_folder}{secure_file_name}"
+
+    return cloud_storage.put_object(bucket_name, key, decoded_file_content, {"original_filename": original_file_name})

backend/src/external/aws/lambdas/s3_file_upload.py

@@ -1,7 +1,13 @@
 import json
 import os
 
+from src import context
 from src.documents.upload_document import upload_file_data
+from src.external.aws.s3 import S3
+from src.storage import CloudStorage
+
+appContext = context.ApplicationContext()
+appContext.register(CloudStorage, S3())
 
 
 def lambda_handler(event, context):
@@ -16,10 +22,10 @@ def lambda_handler(event, context):
         try:
             bucket_name = os.environ.get("S3_BUCKET_NAME", "ocr-poc-flex")
             default_folder = "input/"
-            document_id = upload_file_data(body, bucket_name, default_folder)
+            document_id = upload_file_data(body["file_name"], body["file_content"], bucket_name, default_folder)
         except Exception as e:
             return {
-                "statusCode": 400,
+                "statusCode": 500,
                 "body": json.dumps({"error": str(e)}),
             }
 

backend/src/external/aws/s3.py

@@ -34,7 +34,7 @@ def put_object(self, bucket_name: str, key: str, body: bytes, metadata: dict[str
                 Metadata=metadata,
             )
         except Exception as e:
-            raise CloudStorageException(f"Failed to upload into s3: {e}") from e
+            raise CloudStorageException(f"Failed to upload into 's3://{bucket_name}/{key}'.") from e
 
     def file_exists_and_allowed_to_access(self, remote_url: str) -> bool:
         bucket_name, object_key = self.parse_s3_url(remote_url)

backend/tests/documents/test_upload_document.py

@@ -4,8 +4,13 @@
 import pytest
 
 from src import context
-from src.documents.upload_document import generate_file_data, upload_file_data, upload_to_s3
-from src.storage import CloudStorage, CloudStorageException
+from src.documents.upload_document import (
+    decode_file_content,
+    generate_secure_filename,
+    upload_file_data,
+    upload_file_to_cloud,
+)
+from src.storage import CloudStorage
 
 context = context.ApplicationContext()
 
@@ -36,77 +41,41 @@ def test_uploading_file_data_returns_document_id():
     assert response is not None
 
 
-def test_generating_file_data_with_generate_file_id():
-    mock_file_content = b"Hello, this is a test file."
-    encoded_content = base64.b64encode(mock_file_content).decode("utf-8")
-    body = {
-        "file_name": "mock_file",
-        "file_content": encoded_content,
-    }
+def test_generating_secure_filename_works():
+    mock_original_filename = "Original.txt"
 
-    response = generate_file_data(body)
+    expected_secure_file_name, expected_document_id = generate_secure_filename(mock_original_filename)
 
-    assert response.get("secure_filename") is not None
-    assert response.get("original_filename") == "mock_file"
-    assert response.get("decoded_file_data") is not None
-    assert response.get("document_id") is not None
+    # added coverage for asserts but I need to address how to assert these values properly
+    assert mock_original_filename is not None
+    assert expected_document_id is not None
 
 
-def test_generating_file_data_with_missing_body_file_name():
+def test_generating_file_data_with_generate_file_id():
     mock_file_content = b"Hello, this is a test file."
-    encoded_content = base64.b64encode(mock_file_content).decode("utf-8")
-    body = {
-        "file_content": encoded_content,
-    }
+    decoded_content = base64.b64encode(mock_file_content).decode("utf-8")
 
-    with pytest.raises(ValueError):
-        generate_file_data(body)
-
-
-def test_generating_file_data_with_missing_body_file_content():
-    body = {
-        "file_name": "mock_file",
-    }
+    response = decode_file_content(decoded_content)
 
-    with pytest.raises(ValueError):
-        generate_file_data(body)
+    assert response is not None
 
 
 def test_generating_file_data_with_invalid_file_content():
-    body = {
-        "file_name": "mock_file",
-        "file_content": 1234,
-    }
+    mock_file_content = 1234
     with pytest.raises(TypeError):
-        generate_file_data(body)
+        decode_file_content(mock_file_content)
 
 
-def test_upload_to_s3():
-    file_data = {
-        "secure_filename": "secure_filename",
-        "original_filename": "original_filename",
-        "decoded_file_data": "decoded_file_data",
-        "document_id": "document_id",
-    }
+def test_upload_to_cloud():
+    mock_file_content = b"Hello, this is a test file."
+    secure_file_name = "how secure of your sir"
+    original_file_name = "how original of you sir"
+    decoded_content = base64.b64encode(mock_file_content).decode("utf-8")
+
     mock_cloud_storage = mock.MagicMock()
     mock_cloud_storage.put_object.return_value = None
     context.register(CloudStorage, mock_cloud_storage)
 
-    upload_to_s3(file_data, "mock_bucket", "mock_folder")
+    upload_file_to_cloud(decoded_content, secure_file_name, original_file_name, "mock_bucket", "mock_folder")
 
     mock_cloud_storage.put_object.assert_called_once()
-
-
-def test_upload_to_s3_returns_cloud_storage_exception():
-    file_data = {
-        "secure_filename": "secure_filename",
-        "original_filename": "original_filename",
-        "decoded_file_data": "decoded_file_data",
-        "document_id": "document_id",
-    }
-    mock_cloud_storage = mock.MagicMock()
-    mock_cloud_storage.put_object.side_effect = CloudStorageException("Mocker!")
-    context.register(CloudStorage, mock_cloud_storage)
-
-    with pytest.raises(CloudStorageException):
-        upload_to_s3(file_data, "mock_bucket", "mock_folder")