From 5311d49fa69e5fc47dabd36a8c16101ac9d3d341 Mon Sep 17 00:00:00 2001 From: Brian Bjarke Jensen Date: Wed, 8 Jul 2026 20:02:03 +0200 Subject: [PATCH] Clarify MinIO get() error semantics to match Redis behavior. Return None only for missing objects and re-raise other S3 and network failures so callers can distinguish not-found from real errors. Co-authored-by: Cursor --- python_repositories/adapters/minio_adapter.py | 7 +-- .../interfaces/object_repository_interface.py | 6 +- tests/integration/minio_adapter_test.py | 25 +++----- tests/unit/minio_adapter_test.py | 62 ++++++++++++++++++- 4 files changed, 75 insertions(+), 25 deletions(-) diff --git a/python_repositories/adapters/minio_adapter.py b/python_repositories/adapters/minio_adapter.py index 1f3cb1e..64c8b2e 100644 --- a/python_repositories/adapters/minio_adapter.py +++ b/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: - self.logger.error(repr(exc)) - except Exception as exc: # pylint: disable=broad-except - self.logger.error(repr(exc)) + return None + raise 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.""" diff --git a/python_repositories/interfaces/object_repository_interface.py b/python_repositories/interfaces/object_repository_interface.py index 3fd58c0..54cb9b4 100644 --- a/python_repositories/interfaces/object_repository_interface.py +++ b/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 diff --git a/tests/integration/minio_adapter_test.py b/tests/integration/minio_adapter_test.py index dd4b31b..5cb56ca 100644 --- a/tests/integration/minio_adapter_test.py +++ b/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( - caplog: pytest.LogCaptureFixture, -) -> None: - """Test that the MinioAdapter logs an error for unhandled S3 errors.""" +def test_should_reraise_s3error_other_than_no_such_key() -> None: + """Test that the MinioAdapter re-raises 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"): - result = adapter.get("missing-object") - assert result is None - assert repr(other_s3error) in caplog.text + with pytest.raises(S3Error) as exc_info: + adapter.get("missing-object") + assert exc_info.value.code == "UnhandledError" -def test_should_log_error_when_getting_with_general_exception( - caplog: pytest.LogCaptureFixture, -) -> None: - """Test that the MinioAdapter logs an error on general exceptions during get.""" +def test_should_reraise_general_exception() -> None: + """Test that the MinioAdapter re-raises 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"): - result = adapter.get("missing-object") - assert result is None - assert repr(general_exception) in caplog.text + with pytest.raises(Exception, match="General failure"): + adapter.get("missing-object") def test_should_put_data( diff --git a/tests/unit/minio_adapter_test.py b/tests/unit/minio_adapter_test.py index 5dbb18b..3de59b6 100644 --- a/tests/unit/minio_adapter_test.py +++ b/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")