laskoviymishka commented on code in PR #3206: URL: https://github.com/apache/iceberg-rust/pull/3206#discussion_r4008217672
########## bindings/python/src/encryption.rs: ########## @@ -0,0 +1,79 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use iceberg::encryption::StandardKeyMetadata; +use pyo3::prelude::*; +use pyo3::types::PyBytes; + +use crate::error::to_py_err; + +/// The encryption key, AAD prefix and file length held by `StandardKeyMetadata`. +type DecodedKeyMetadata<'py> = ( Review Comment: The bare 3-tuple leaves Python callers with positional access only, and for an encryption API the confusion between `encryption_key` and `aad_prefix` (both bytes) is a real footgun — `decoded[0]` vs `decoded[1]` is easy to get wrong, and switching to a named type later is a breaking change. I know manifest/transform expose named classes (`PyManifest`, `PyTransform`) while this stays a tuple, so there's precedent both ways. I'd lean toward a small `#[pyclass]` with named getters here given what these bytes are, but at minimum I'd document the positions on the type alias and in the `decode_standard_key_metadata` docstring: `(encryption_key, aad_prefix, file_length)`. wdyt? ########## bindings/python/src/encryption.rs: ########## @@ -0,0 +1,79 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use iceberg::encryption::StandardKeyMetadata; +use pyo3::prelude::*; +use pyo3::types::PyBytes; + +use crate::error::to_py_err; + +/// The encryption key, AAD prefix and file length held by `StandardKeyMetadata`. +type DecodedKeyMetadata<'py> = ( + Bound<'py, PyBytes>, + Option<Bound<'py, PyBytes>>, + Option<u64>, +); + +/// Decode `StandardKeyMetadata` from its wire format. +#[pyfunction] +pub fn decode_standard_key_metadata<'py>( + py: Python<'py>, + data: &[u8], +) -> PyResult<DecodedKeyMetadata<'py>> { + let metadata = StandardKeyMetadata::decode(data).map_err(to_py_err)?; Review Comment: `to_py_err` collapses everything to `PyValueError`, so the unsupported-version path (which is `FeatureUnsupported`) surfaces as a `ValueError` a caller can't distinguish from malformed data. It's the one error kind here with a natural Python counterpart, so I'd map it through a small local helper: ```rust fn key_metadata_err(err: iceberg::Error) -> PyErr { match err.kind() { ErrorKind::FeatureUnsupported => PyNotImplementedError::new_err(err.message().to_string()), _ => to_py_err(err), } } ``` While we're here — `err.to_string()` prepends the kind, so `str(exc)` reads `FeatureUnsupported => Unsupported key metadata version: …`, which diverges from pyiceberg's plain message; `err.message()` drops the prefix. No regression for the pyiceberg migration since it also raises `ValueError`, so this is forward-looking, not blocking. ########## bindings/python/tests/test_encryption.py: ########## @@ -0,0 +1,69 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import pytest +from pyiceberg_core import encryption + +AES128_KEY = b"0123456789012345" + + +def test_encode_decode_round_trip(): + encoded = encryption.encode_standard_key_metadata(AES128_KEY, b"ad", 1024) + + assert encryption.decode_standard_key_metadata(encoded) == (AES128_KEY, b"ad", 1024) + + +def test_encode_decode_without_optional_fields(): + encoded = encryption.encode_standard_key_metadata(AES128_KEY) + + assert encryption.decode_standard_key_metadata(encoded) == (AES128_KEY, None, None) + + +def test_encoded_wire_format(): + # A version byte, then the Avro datum. Pinned so the encoding stays compatible + # with the Java and Python implementations. + assert encryption.encode_standard_key_metadata(AES128_KEY, b"ad", 1024) == ( + b"\x01\x20" + AES128_KEY + b"\x02\x04ad\x02\x80\x10" Review Comment: This pins only the encode direction with both optionals present, but the decode direction against known-good bytes is the actual interop guarantee (us reading Java-produced bytes), and the null-union encoding for absent fields — the `\x00` byte a Java/PyIceberg reader expects — is never exercised. A null-branch bug would round-trip fine here but break cross-client. pyiceberg #3948 pins four cases; I'd mirror them: ```python assert encryption.decode_standard_key_metadata( b"\x01\x20" + AES128_KEY + b"\x02\x04ad\x02\x80\x10" ) == (AES128_KEY, b"ad", 1024) assert encryption.encode_standard_key_metadata(AES128_KEY) == b"\x01\x20" + AES128_KEY + b"\x00\x00" assert encryption.encode_standard_key_metadata(AES128_KEY, b"ad") == b"\x01\x20" + AES128_KEY + b"\x02\x04ad\x00" ``` The last one also covers the aad-present/file-length-absent combination that isn't tested today. ########## bindings/python/tests/test_encryption.py: ########## @@ -0,0 +1,69 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import pytest +from pyiceberg_core import encryption + +AES128_KEY = b"0123456789012345" + + +def test_encode_decode_round_trip(): + encoded = encryption.encode_standard_key_metadata(AES128_KEY, b"ad", 1024) + + assert encryption.decode_standard_key_metadata(encoded) == (AES128_KEY, b"ad", 1024) + + +def test_encode_decode_without_optional_fields(): + encoded = encryption.encode_standard_key_metadata(AES128_KEY) + + assert encryption.decode_standard_key_metadata(encoded) == (AES128_KEY, None, None) + + +def test_encoded_wire_format(): + # A version byte, then the Avro datum. Pinned so the encoding stays compatible + # with the Java and Python implementations. + assert encryption.encode_standard_key_metadata(AES128_KEY, b"ad", 1024) == ( + b"\x01\x20" + AES128_KEY + b"\x02\x04ad\x02\x80\x10" + ) + + [email protected]("key_length", [16, 24, 32]) +def test_encode_accepts_aes_key_lengths(key_length): + encoded = encryption.encode_standard_key_metadata(bytes(key_length)) + + assert encryption.decode_standard_key_metadata(encoded) == ( + bytes(key_length), + None, + None, + ) + + [email protected]("key_length", [0, 4, 15, 20, 33]) +def test_encode_rejects_invalid_key_length(key_length): + with pytest.raises(ValueError): + encryption.encode_standard_key_metadata(bytes(key_length)) + + [email protected]("data", [b"\x02", b"\x02\x20" + AES128_KEY + b"\x00\x00"]) +def test_decode_rejects_unsupported_version(data): + with pytest.raises(ValueError, match="Unsupported key metadata version: 2"): + encryption.decode_standard_key_metadata(data) + + +def test_decode_rejects_empty_buffer(): + with pytest.raises(ValueError): + encryption.decode_standard_key_metadata(b"") Review Comment: Your PR body notes apache-avro 0.21 decodes a missing/truncated union tag as null rather than erroring, so `b"\x01\x20" + AES128_KEY` (version + valid key, union tags omitted) likely decodes as `(key, None, None)` — indistinguishable from a legit key-only payload. Without a test anchoring that current behavior, there's no regression signal when the upstream fix lands. I'd add it as a strict xfail alongside these rejection tests so it flips to passing once fixed: ```python @pytest.mark.xfail(reason="apache-avro#NNN: truncated union tags decode as null", strict=True) def test_decode_rejects_truncated_union_tags(): with pytest.raises(ValueError): encryption.decode_standard_key_metadata(b"\x01\x20" + AES128_KEY) ``` The PR-body TODO says the upstream issue isn't filed yet — worth filing it so this reason can link somewhere real. Pre-existing core behavior, not something to hold merge on. ########## bindings/python/tests/test_encryption.py: ########## @@ -0,0 +1,69 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import pytest +from pyiceberg_core import encryption + +AES128_KEY = b"0123456789012345" + + +def test_encode_decode_round_trip(): + encoded = encryption.encode_standard_key_metadata(AES128_KEY, b"ad", 1024) + + assert encryption.decode_standard_key_metadata(encoded) == (AES128_KEY, b"ad", 1024) + + +def test_encode_decode_without_optional_fields(): + encoded = encryption.encode_standard_key_metadata(AES128_KEY) + + assert encryption.decode_standard_key_metadata(encoded) == (AES128_KEY, None, None) + + +def test_encoded_wire_format(): + # A version byte, then the Avro datum. Pinned so the encoding stays compatible + # with the Java and Python implementations. + assert encryption.encode_standard_key_metadata(AES128_KEY, b"ad", 1024) == ( + b"\x01\x20" + AES128_KEY + b"\x02\x04ad\x02\x80\x10" + ) + + [email protected]("key_length", [16, 24, 32]) +def test_encode_accepts_aes_key_lengths(key_length): + encoded = encryption.encode_standard_key_metadata(bytes(key_length)) + + assert encryption.decode_standard_key_metadata(encoded) == ( + bytes(key_length), + None, + None, + ) + + [email protected]("key_length", [0, 4, 15, 20, 33]) +def test_encode_rejects_invalid_key_length(key_length): + with pytest.raises(ValueError): + encryption.encode_standard_key_metadata(bytes(key_length)) + + [email protected]("data", [b"\x02", b"\x02\x20" + AES128_KEY + b"\x00\x00"]) +def test_decode_rejects_unsupported_version(data): + with pytest.raises(ValueError, match="Unsupported key metadata version: 2"): Review Comment: The error-string assertions are pulling in two directions here — this one pins nearly the whole sentence (now owned in three places: the Rust `format!`, the Rust `assert_eq!`, and this `match=`), while `test_decode_rejects_empty_buffer` and `test_encode_rejects_invalid_key_length` pin nothing and pass on any `ValueError`. I'd match a minimal stable fragment throughout — `match=r"version: 2"` here, and add `match="Empty key metadata buffer"` / `match="key length"` to the two that currently assert type only. That gets you a message check on all three without the full-sentence coupling. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
