Merge pull request '[breaking] Clarify MinIO get() error semantics to match Redis behavior' (#33) from cursor/minio-error-semantics into main

Reviewed-on: https://gitea.lille-vemmelund.dk/brian/python-repositories/pulls/33

python_repositories/adapters/minio_adapter.py

@@ -172,15 +172,12 @@ class MinioAdapter(ObjectRepositoryInterface, ConnectionAwareAdapter):
                 self.logger.warning(
                     f"Object '{object_name}' not found in bucket '{self._bucket_name}'"
                 )
-            else:
+                return None
-                self.logger.error(repr(exc))
+            raise
-        except Exception as exc:  # pylint: disable=broad-except
-            self.logger.error(repr(exc))
         finally:
             if response is not None:
                 response.close()
                 response.release_conn()
-        return None
 
     def delete(self, object_name: str) -> None:
         """Delete an object from the Minio bucket."""

python_repositories/interfaces/object_repository_interface.py

@@ -9,7 +9,11 @@ class ObjectRepositoryInterface(ABC):
 
     @abstractmethod
     def get(self, object_name: str) -> BytesIO | None:
-        """Get an object by name."""
+        """Get an object by name.
+
+        Returns None when the object does not exist. Raises ConnectionError when
+        not connected. Other backend errors propagate to the caller.
+        """
         ...
 
     @abstractmethod

tests/integration/minio_adapter_test.py

@@ -218,10 +218,8 @@ def test_should_log_warning_when_getting_nonexistent_object(
     )
 
 
-def test_should_log_error_when_getting_with_s3error_other_than_no_such_key(
+def test_should_reraise_s3error_other_than_no_such_key() -> None:
-    caplog: pytest.LogCaptureFixture,
+    """Test that the MinioAdapter re-raises unhandled S3 errors."""
-) -> None:
-    """Test that the MinioAdapter logs an error for unhandled S3 errors."""
     mock_client = MagicMock(spec=Minio)
     mock_client.bucket_exists.return_value = True
     other_s3error = S3Error(
@@ -236,25 +234,20 @@ def test_should_log_error_when_getting_with_s3error_other_than_no_such_key(
     )
     mock_client.get_object.side_effect = other_s3error
     adapter = MinioAdapter(config=TEST_MINIO_CONFIG, client=mock_client)
-    with caplog.at_level("ERROR"):
+    with pytest.raises(S3Error) as exc_info:
-        result = adapter.get("missing-object")
+        adapter.get("missing-object")
-    assert result is None
+    assert exc_info.value.code == "UnhandledError"
-    assert repr(other_s3error) in caplog.text
 
 
-def test_should_log_error_when_getting_with_general_exception(
+def test_should_reraise_general_exception() -> None:
-    caplog: pytest.LogCaptureFixture,
+    """Test that the MinioAdapter re-raises general exceptions during get."""
-) -> None:
-    """Test that the MinioAdapter logs an error on general exceptions during get."""
     mock_client = MagicMock(spec=Minio)
     mock_client.bucket_exists.return_value = True
     general_exception = Exception("General failure")
     mock_client.get_object.side_effect = general_exception
     adapter = MinioAdapter(config=TEST_MINIO_CONFIG, client=mock_client)
-    with caplog.at_level("ERROR"):
+    with pytest.raises(Exception, match="General failure"):
-        result = adapter.get("missing-object")
+        adapter.get("missing-object")
-    assert result is None
-    assert repr(general_exception) in caplog.text
 
 
 def test_should_put_data(

tests/unit/minio_adapter_test.py

@@ -5,7 +5,8 @@ from __future__ import annotations
 from unittest.mock import MagicMock
 
 import pytest
-from minio import Minio
+from minio import Minio, S3Error
+from urllib3.response import BaseHTTPResponse
 
 from python_repositories.adapters.minio_adapter import MinioAdapter
 from python_repositories.interfaces import ObjectRepositoryInterface
@@ -119,8 +120,63 @@ def test_get_closes_response_when_read_fails() -> None:
     mock_client.get_object.return_value = mock_response
     adapter = MinioAdapter(config=TEST_MINIO_CONFIG, client=mock_client)
 
-    result = adapter.get("some-object")
+    with pytest.raises(OSError, match="connection reset"):
+        adapter.get("some-object")
 
-    assert result is None
     mock_response.close.assert_called_once()
     mock_response.release_conn.assert_called_once()
+
+
+def test_get_returns_none_for_no_such_key() -> None:
+    """get() returns None when the object does not exist."""
+    mock_client = MagicMock(spec=Minio)
+    mock_client.bucket_exists.return_value = True
+    mock_client.get_object.side_effect = S3Error(
+        MagicMock(spec=BaseHTTPResponse),
+        "NoSuchKey",
+        "",
+        "",
+        "",
+        "",
+        bucket_name="test-bucket",
+        object_name="missing-object",
+    )
+    adapter = MinioAdapter(config=TEST_MINIO_CONFIG, client=mock_client)
+
+    result = adapter.get("missing-object")
+
+    assert result is None
+
+
+def test_get_reraises_other_s3_errors() -> None:
+    """get() re-raises S3 errors other than NoSuchKey."""
+    mock_client = MagicMock(spec=Minio)
+    mock_client.bucket_exists.return_value = True
+    other_s3error = S3Error(
+        MagicMock(spec=BaseHTTPResponse),
+        "AccessDenied",
+        "",
+        "",
+        "",
+        "",
+        bucket_name="test-bucket",
+        object_name="some-object",
+    )
+    mock_client.get_object.side_effect = other_s3error
+    adapter = MinioAdapter(config=TEST_MINIO_CONFIG, client=mock_client)
+
+    with pytest.raises(S3Error) as exc_info:
+        adapter.get("some-object")
+
+    assert exc_info.value.code == "AccessDenied"
+
+
+def test_get_reraises_general_exception_from_get_object() -> None:
+    """get() re-raises unexpected exceptions from get_object."""
+    mock_client = MagicMock(spec=Minio)
+    mock_client.bucket_exists.return_value = True
+    mock_client.get_object.side_effect = Exception("General failure")
+    adapter = MinioAdapter(config=TEST_MINIO_CONFIG, client=mock_client)
+
+    with pytest.raises(Exception, match="General failure"):
+        adapter.get("some-object")