blackmwk commented on code in PR #3090:
URL: https://github.com/apache/iceberg-rust/pull/3090#discussion_r3901050742
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -544,4 +564,81 @@ mod tests {
assert_eq!(file_io.config().get("key1"), Some(&"value1".to_string()));
assert_eq!(file_io.config().get("key2"), Some(&"value2".to_string()));
}
+
+ #[tokio::test]
+ async fn test_memory_file_io_serialization_roundtrip() {
+ let file_io = FileIOBuilder::new(Arc::new(MemoryStorageFactory))
+ .with_prop("test-property", "test-value")
+ .with_prop("s3.session-token", "test-token")
+ .build();
Review Comment:
Addressed in
[5b50b1f0](https://github.com/apache/iceberg-rust/pull/3090/commits/5b50b1f0f21cf994709991596e765877eeb2c1a2).
`FileIO` no longer implements the public serde `Serialize`/`Deserialize`
traits, so generic serialization cannot emit credentials implicitly.
Full-fidelity transport is now an explicit `serialize_all` call paired with
`deserialize_all`. The method documentation states that every storage property,
including credentials, is included and that the returned bytes must be
protected in transit and at rest. This avoids an incomplete key-name denylist
while making the sensitive operation explicit.
##########
crates/iceberg/public-api.txt:
##########
@@ -739,6 +739,10 @@ impl core::clone::Clone for iceberg::io::FileIO
pub fn iceberg::io::FileIO::clone(&self) -> iceberg::io::FileIO
impl core::fmt::Debug for iceberg::io::FileIO
pub fn iceberg::io::FileIO::fmt(&self, f: &mut core::fmt::Formatter<'_>) ->
core::fmt::Result
+impl serde_core::ser::Serialize for iceberg::io::FileIO
+pub fn iceberg::io::FileIO::serialize<__S>(&self, __serializer: __S) ->
core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as
serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer
Review Comment:
Addressed in
[5b50b1f0](https://github.com/apache/iceberg-rust/pull/3090/commits/5b50b1f0f21cf994709991596e765877eeb2c1a2).
The derived public serde implementations were removed entirely rather than
feature-gated. `FileIO` now exposes only `serialize_all` and `deserialize_all`,
and the public API baseline confirms that `Serialize`/`Deserialize` are no
longer part of its surface.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -104,6 +104,12 @@ pub use resolving::{OpenDalResolvingStorage,
OpenDalResolvingStorageFactory};
///
/// Maps scheme to the corresponding OpenDalStorage storage variant.
/// Use this factory with `FileIOBuilder::new(factory)` to create FileIO
instances.
+///
+/// # Serialization
+///
+/// Serialization fails when the [`OpenDalStorageFactory::S3`] variant
contains a custom AWS
Review Comment:
Addressed in
[5b50b1f0](https://github.com/apache/iceberg-rust/pull/3090/commits/5b50b1f0f21cf994709991596e765877eeb2c1a2).
The feature-gated intra-doc link is now plain code text. Verified with
`RUSTDOCFLAGS='-D warnings' cargo doc -p iceberg-storage-opendal
--no-default-features --no-deps`.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -133,6 +143,22 @@ pub enum OpenDalStorageFactory {
Hf,
}
+#[cfg(feature = "opendal-s3")]
+pub(crate) fn serialize_custom_credential_loader<S>(
+ loader: &Option<CustomAwsCredentialLoader>,
+ serializer: S,
+) -> std::result::Result<S::Ok, S::Error>
+where
+ S: serde::Serializer,
+{
+ match loader {
+ Some(_) => Err(serde::ser::Error::custom(
+ "custom AWS credential loaders cannot be serialized",
+ )),
+ None => serializer.serialize_none(),
Review Comment:
Addressed in
[5b50b1f0](https://github.com/apache/iceberg-rust/pull/3090/commits/5b50b1f0f21cf994709991596e765877eeb2c1a2).
Kept `skip_serializing_if = "Option::is_none"` and collapsed the helper to the
only reachable behavior: an unconditional error when serde invokes it for
`Some`.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -104,6 +104,12 @@ pub use resolving::{OpenDalResolvingStorage,
OpenDalResolvingStorageFactory};
///
/// Maps scheme to the corresponding OpenDalStorage storage variant.
/// Use this factory with `FileIOBuilder::new(factory)` to create FileIO
instances.
+///
+/// # Serialization
Review Comment:
Addressed in
[5b50b1f0](https://github.com/apache/iceberg-rust/pull/3090/commits/5b50b1f0f21cf994709991596e765877eeb2c1a2).
`OpenDalStorageFactory` now documents that the receiving binary must enable
the feature corresponding to the serialized backend variant, including the
concrete `opendal-s3` example.
--
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]
