From bd6d2151241425d024f5de1ac6a0b4121afef02f Mon Sep 17 00:00:00 2001 From: Kyle Barron Date: Tue, 21 Jul 2026 16:10:45 +0200 Subject: [PATCH] fix: Only strip config key prefix when present MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `IntoPyObject` for the S3/GCS/Azure config keys assumed every key `as_ref()`s to a prefixed string and used `.expect()` to strip it. But object_store delegates client and encryption keys to inner enums — `Self::Client(opt) => opt.as_ref()` — which return unprefixed names like `allow_http`. Reading `.config` on a store built with any client option therefore panicked: S3Store("bucket", allow_http=True).config PanicException: Expected config prefix to start with aws_ Strip the prefix only when it is actually present, matching what object_store's own `FromStr` does (`strip_prefix("aws_").unwrap_or(s)`). Separately, `aws_endpoint_url_s3` is the one key with no unprefixed alias upstream, so the `endpoint_url_s3` we emit could not be parsed back in — breaking `S3Store(config=store.config)` and pickling. Retry the parse with the `aws_` prefix on failure. Co-Authored-By: Claude Opus 4.8 (1M context) --- pyo3-object_store/src/aws/store.rs | 18 +++++++++++------- pyo3-object_store/src/azure/store.rs | 8 +++----- pyo3-object_store/src/gcp/store.rs | 10 ++++------ tests/store/test_azure.py | 8 ++++++++ tests/store/test_gcs.py | 8 ++++++++ tests/store/test_s3.py | 16 ++++++++++++++++ 6 files changed, 50 insertions(+), 18 deletions(-) diff --git a/pyo3-object_store/src/aws/store.rs b/pyo3-object_store/src/aws/store.rs index f8041c4a..b49b5b36 100644 --- a/pyo3-object_store/src/aws/store.rs +++ b/pyo3-object_store/src/aws/store.rs @@ -230,7 +230,13 @@ impl<'py> FromPyObject<'_, 'py> for PyAmazonS3ConfigKey { fn extract(obj: Borrowed<'_, 'py, pyo3::PyAny>) -> PyResult { let s = obj.extract::()?.to_lowercase(); - let key = s.parse().map_err(PyObjectStoreError::ObjectStoreError)?; + // Some keys (e.g. `aws_endpoint_url_s3`) are only accepted upstream in their prefixed + // form, but we strip the `aws_` prefix when converting keys back to Python. Retry with the + // prefix so that any key we emit can be parsed back in. + let key = s + .parse::() + .or_else(|err| format!("aws_{s}").parse().map_err(|_| err)) + .map_err(PyObjectStoreError::ObjectStoreError)?; Ok(Self(key)) } } @@ -247,12 +253,10 @@ impl<'py> IntoPyObject<'py> for &PyAmazonS3ConfigKey { type Error = PyErr; fn into_pyobject(self, py: Python<'py>) -> Result { - let s = self - .0 - .as_ref() - .strip_prefix("aws_") - .expect("Expected config prefix to start with aws_"); - Ok(PyString::new(py, s)) + // Client and encryption config keys are not `aws_`-prefixed upstream, so only strip the + // prefix when it is actually present. + let s = self.0.as_ref(); + Ok(PyString::new(py, s.strip_prefix("aws_").unwrap_or(s))) } } diff --git a/pyo3-object_store/src/azure/store.rs b/pyo3-object_store/src/azure/store.rs index d572b53c..18741fae 100644 --- a/pyo3-object_store/src/azure/store.rs +++ b/pyo3-object_store/src/azure/store.rs @@ -274,11 +274,9 @@ impl<'py> IntoPyObject<'py> for &PyAzureConfigKey { if let Some(stripped) = s.strip_prefix("azure_storage_") { return Ok(PyString::new(py, stripped)); } - Ok(PyString::new( - py, - s.strip_prefix("azure_") - .expect("Expected config prefix to start with azure_"), - )) + // Client config keys are not `azure_`-prefixed upstream, so only strip the prefix when it + // is actually present. + Ok(PyString::new(py, s.strip_prefix("azure_").unwrap_or(s))) } } diff --git a/pyo3-object_store/src/gcp/store.rs b/pyo3-object_store/src/gcp/store.rs index 260708df..0163193d 100644 --- a/pyo3-object_store/src/gcp/store.rs +++ b/pyo3-object_store/src/gcp/store.rs @@ -248,12 +248,10 @@ impl<'py> IntoPyObject<'py> for &PyGoogleConfigKey { type Error = PyErr; fn into_pyobject(self, py: Python<'py>) -> Result { - let s = self - .0 - .as_ref() - .strip_prefix("google_") - .expect("Expected config prefix to start with google_"); - Ok(PyString::new(py, s)) + // Client config keys are not `google_`-prefixed upstream, so only strip the prefix when it + // is actually present. + let s = self.0.as_ref(); + Ok(PyString::new(py, s.strip_prefix("google_").unwrap_or(s))) } } diff --git a/tests/store/test_azure.py b/tests/store/test_azure.py index ed1a60bd..a7aeaeb7 100644 --- a/tests/store/test_azure.py +++ b/tests/store/test_azure.py @@ -34,6 +34,14 @@ def test_eq(): assert store != store3 +def test_client_config_key_does_not_panic(): + # Client config keys are not `azure_`-prefixed upstream, so reading `.config` + # used to panic. + store = AzureStore("container", account_name="account_name", allow_http=True) + assert store.config["allow_http"] == "true" + assert AzureStore(config=store.config).config == store.config + + def test_from_url(): # https://github.com/developmentseed/obstore/issues/477 url = "https://overturemapswestus2.blob.core.windows.net/release" diff --git a/tests/store/test_gcs.py b/tests/store/test_gcs.py index 4df150a5..7a5f0f57 100644 --- a/tests/store/test_gcs.py +++ b/tests/store/test_gcs.py @@ -21,6 +21,14 @@ def test_eq(): assert store != store3 +def test_client_config_key_does_not_panic(): + # Client config keys are not `google_`-prefixed upstream, so reading `.config` + # used to panic. + store = GCSStore("bucket", allow_http=True) + assert store.config["allow_http"] == "true" + assert GCSStore(config=store.config).config == store.config + + def test_application_credentials(): # The application_credentials parameter should be correctly passed down # Finalizing the GCSBuilder should try to load and parse those credentials, which diff --git a/tests/store/test_s3.py b/tests/store/test_s3.py index dd9061f2..7159ccc0 100644 --- a/tests/store/test_s3.py +++ b/tests/store/test_s3.py @@ -85,6 +85,22 @@ def test_pickle(): _objects = next(restored.list()) +def test_client_config_key_does_not_panic(): + # Client config keys are not `aws_`-prefixed upstream, so reading `.config` used + # to panic. + store = S3Store("bucket", allow_http=True) + assert store.config["allow_http"] == "true" + + +def test_prefixed_only_config_key_round_trip(): + # `aws_endpoint_url_s3` has no unprefixed alias upstream, but we emit it unprefixed. + store = S3Store("bucket", aws_endpoint_url_s3="https://example.com") # type: ignore + assert store.config["endpoint_url_s3"] == "https://example.com" + + assert S3Store(config=store.config).config == store.config + assert pickle.loads(pickle.dumps(store)) == store + + def test_config_round_trip(): store = S3Store.from_url( "s3://ookla-open-data/parquet/performance/type=fixed/year=2024/quarter=1",