From b4bb5a43fb5af46617609e8e6983b7be2effd79d Mon Sep 17 00:00:00 2001 From: Graham Hukill Date: Fri, 15 May 2026 12:56:19 -0400 Subject: [PATCH 1/2] Improve AWS credential error reporting Why these changes are being introduced: There are two main buckets (no pun intended) in which AWS credentials can fail: 1. They are not set or stale 2. They are pointing at the wrong environment The error reporting for both is poor, with opaque DuckDB or boto3 errors. For DuckDB it's during secret construction, and makes me mention of AWS. For boto3, it's checking that destination bucket exists, which is better but still confusing. How this addresses that need: * Catches AWS related errors in very different parts of the codebase, surfacing a normalized error message that something is wrong with AWS credentials. Side effects of this change: * None Relevant ticket(s): * None --- tests/test_aws_credentials.py | 84 ++++++++++++++++++++++++++++++++ timdex_dataset_api/exceptions.py | 13 +++++ timdex_dataset_api/utils.py | 31 ++++++++---- 3 files changed, 119 insertions(+), 9 deletions(-) create mode 100644 tests/test_aws_credentials.py diff --git a/tests/test_aws_credentials.py b/tests/test_aws_credentials.py new file mode 100644 index 0000000..7826fca --- /dev/null +++ b/tests/test_aws_credentials.py @@ -0,0 +1,84 @@ +"""tests/test_aws_credentials.py""" + +# ruff: noqa: SLF001 + +import duckdb +import pytest +from botocore.stub import Stubber + +from timdex_dataset_api.exceptions import AWSCredentialsError +from timdex_dataset_api.utils import DuckDBConnectionFactory, S3Client + + +@pytest.fixture +def no_aws_credentials(monkeypatch, tmp_path): + """Isolate a test from all ambient AWS credential sources.""" + for env_var in [ + "AWS_PROFILE", + "AWS_DEFAULT_PROFILE", + "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY", + "AWS_SESSION_TOKEN", + "AWS_SECURITY_TOKEN", + ]: + monkeypatch.delenv(env_var, raising=False) + + aws_config_file = tmp_path / "config" + aws_credentials_file = tmp_path / "credentials" + aws_config_file.write_text("") + aws_credentials_file.write_text("") + + monkeypatch.setenv("AWS_CONFIG_FILE", str(aws_config_file)) + monkeypatch.setenv("AWS_SHARED_CREDENTIALS_FILE", str(aws_credentials_file)) + monkeypatch.setenv("AWS_EC2_METADATA_DISABLED", "true") + + +def test_s3client_object_exists_access_denied_raises_aws_credentials_error(monkeypatch): + """Test that 403 authorization errors bubble up as helpful TDA exceptions. + + This covers the scenario where AWS credentials ARE present, but point at the + wrong account/environment or otherwise lack permission to read the dataset. + Stubber exercises boto3/botocore's normal ClientError path without making a + real AWS request. + """ + monkeypatch.setenv("AWS_ACCESS_KEY_ID", "bad_access_key") + monkeypatch.setenv("AWS_SECRET_ACCESS_KEY", "bad_secret_key") + monkeypatch.setenv("AWS_SESSION_TOKEN", "bad_session_token") + + client = S3Client() + + stubber = Stubber(client.resource.meta.client) + stubber.add_client_error( + "head_object", + service_error_code="403", + service_message="Forbidden", + http_status_code=403, # <------ simulates 403 authorization error + expected_params={ + "Bucket": "timdex", + "Key": "metadata/timdex_metadata.duckdb", + }, + ) + + with stubber, pytest.raises(AWSCredentialsError, match="AWS credentials absent"): + client.object_exists("s3://timdex/metadata/timdex_metadata.duckdb") + + +def test_duckdb_s3_secret_without_credentials_raises_aws_credentials_error( + no_aws_credentials, +): + """Test that we get a helpful mesesage when DuckDB can't find AWS credentials. + + This test uses the local fixture 'no_aws_credentials' which completely unsets all + traces of AWS credentials in the environment. This is what expired credentials also + look like to DuckDB when consructing a secret. + """ + factory = DuckDBConnectionFactory(location_scheme="s3") + + with ( + duckdb.connect(":memory:") as conn, + pytest.raises( + AWSCredentialsError, + match="AWS credentials absent", + ), + ): + factory._configure_s3_secret(conn) diff --git a/timdex_dataset_api/exceptions.py b/timdex_dataset_api/exceptions.py index deadb4a..472b674 100644 --- a/timdex_dataset_api/exceptions.py +++ b/timdex_dataset_api/exceptions.py @@ -1,5 +1,18 @@ """timdex_dataset_api/exceptions.py""" +class AWSCredentialsError(Exception): + """Raised when AWS credentials are absent or insufficient.""" + + def __init__( + self, + message: str = ( + "AWS credentials absent or insufficient. Please ensure they are set " + "and pointing at the right TIMDEX dataset environment." + ), + ) -> None: + super().__init__(message) + + class InvalidDatasetRecordError(Exception): """Custom exception for invalid DatasetRecord instances.""" diff --git a/timdex_dataset_api/utils.py b/timdex_dataset_api/utils.py index 627fb8d..4e628e8 100644 --- a/timdex_dataset_api/utils.py +++ b/timdex_dataset_api/utils.py @@ -11,6 +11,7 @@ import boto3 import duckdb +from botocore.exceptions import ClientError, NoCredentialsError, PartialCredentialsError from duckdb import DuckDBPyConnection from duckdb_engine import ConnectionWrapper from sqlalchemy import ( @@ -20,11 +21,14 @@ create_engine, ) +from timdex_dataset_api.exceptions import AWSCredentialsError + if TYPE_CHECKING: from mypy_boto3_s3.service_resource import S3ServiceResource logger = logging.getLogger(__name__) +HTTP_FORBIDDEN = 403 class S3Client: @@ -55,10 +59,16 @@ def object_exists(self, s3_uri: str) -> bool: try: self.resource.Object(bucket, key).load() return True # noqa: TRY300 - except self.resource.meta.client.exceptions.ClientError as e: - if e.response["Error"]["Code"] == "404": + except ClientError as e: + error_code = e.response["Error"]["Code"] + http_status_code = e.response["ResponseMetadata"]["HTTPStatusCode"] + if error_code == "404": return False + if http_status_code == HTTP_FORBIDDEN: + raise AWSCredentialsError from e raise + except (NoCredentialsError, PartialCredentialsError) as e: + raise AWSCredentialsError from e def list_objects(self, s3_prefix: str) -> list[str]: bucket, _ = self._split_s3_uri(s3_prefix) @@ -186,13 +196,16 @@ def _configure_s3_secret(self, conn: DuckDBPyConnection) -> None: """) elif self.location_scheme == "s3": - conn.execute(""" - create or replace secret aws_s3_secret ( - type s3, - provider credential_chain, - refresh true - ); - """) + try: + conn.execute(""" + create or replace secret aws_s3_secret ( + type s3, + provider credential_chain, + refresh true + ); + """) + except duckdb.Error as e: + raise AWSCredentialsError from e def _configure_memory_profile(self, conn: DuckDBPyConnection) -> None: """Configure DuckDB memory and thread settings.""" From 57556e0025738c3096bf73907e7a5c5a0aa29d72 Mon Sep 17 00:00:00 2001 From: Graham Hukill Date: Fri, 15 May 2026 12:59:37 -0400 Subject: [PATCH 2/2] Bump version to 5.0.1 --- pyproject.toml | 2 +- uv.lock | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index cd7848f..e8a62d5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "timdex_dataset_api" -version = "5.0.0" +version = "5.0.1" description = "Python library for interacting with a TIMDEX parquet dataset" readme = "README.md" requires-python = ">=3.12" diff --git a/uv.lock b/uv.lock index 14e13d4..f32ca86 100644 --- a/uv.lock +++ b/uv.lock @@ -1612,7 +1612,7 @@ wheels = [ [[package]] name = "timdex-dataset-api" -version = "5.0.0" +version = "5.0.1" source = { editable = "." } dependencies = [ { name = "attrs" },