laskoviymishka commented on code in PR #3090:
URL: https://github.com/apache/iceberg-rust/pull/3090#discussion_r3881046822
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -59,13 +60,14 @@ use crate::Result;
/// .with_prop("key", "value")
/// .build();
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct FileIO {
/// Storage configuration containing properties
config: StorageConfig,
/// Factory for creating storage instances
factory: Arc<dyn StorageFactory>,
Review Comment:
`OpenDalResolvingStorageFactory` marks `customized_credential_load` as
`#[serde(skip)]`, so a `FileIO` built with a custom credential loader
serializes fine but comes back with the loader gone. The deserialized one then
silently falls back to the default opendal credential chain — wrong principal,
or an auth failure with nothing pointing at what got dropped.
The roundtrip reads like an identity transform but isn't. I'd at least
document that process-local factory state isn't preserved, and ideally have the
factory's serialize error out when a loader is actually set rather than
discarding it quietly. wdyt?
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -59,13 +60,14 @@ use crate::Result;
/// .with_prop("key", "value")
/// .build();
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct FileIO {
/// Storage configuration containing properties
config: StorageConfig,
Review Comment:
Once `FileIO` is serializable, this field carries the entire
`StorageConfig.props` map into the output — `s3.secret-access-key`,
`s3.session-token`, the GCS oauth token, Azure account keys, whatever's set —
with no redaction. A serialized `FileIO` is plaintext credentials in whatever
transport or store it lands in, which is a lot of exposure given the whole
point is moving these between processes.
Since these impls are public API now, the format is hard to change once it
ships, so I'd want it safe by default from the start: either a custom
`Serialize` that redacts the known sensitive suffixes (`*secret*`, `*token*`,
`*key*`, `*password*`), or at minimum a very loud doc that the output contains
credentials and the caller owns protecting it. What's the intended handling
here?
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -59,13 +60,14 @@ use crate::Result;
/// .with_prop("key", "value")
/// .build();
/// ```
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
Review Comment:
Since these impls are now public API, I'd add a `# Serialization` doc
section for the sharp edges. Two that'll bite: the factory goes through
typetag's global registry, so a blob only deserializes in a binary that links
and registers the concrete factory — an `OpenDalStorageFactory` payload won't
load in a process that doesn't link opendal, and you get a runtime "unknown
type" error with no compile-time signal. And the cached storage is dropped on
deserialize and rebuilt lazily.
Third-party factories also need `#[typetag::serde]` to participate at all.
None of that is discoverable today, so it'll surface as confusing runtime
failures — worth writing down before it's locked in.
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -544,4 +546,42 @@ 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_file_io_serialization_roundtrip() {
+ let file_io = FileIOBuilder::new(Arc::new(MemoryStorageFactory))
+ .with_prop("key", "value")
+ .build();
+
+ // Initialize the storage cache before serializing. The cache is
process-local and should
+ // be rebuilt from the factory and configuration after deserialization.
+ file_io
+ .new_output("memory://test/file.txt")
+ .unwrap()
+ .write("test".into())
+ .await
+ .unwrap();
+
+ let serialized = serde_json::to_string(&file_io).unwrap();
+ let deserialized: FileIO = serde_json::from_str(&serialized).unwrap();
+
+ assert_eq!(deserialized.config().get("key"),
Some(&"value".to_string()));
+ assert!(!deserialized.exists("memory://test/file.txt").await.unwrap());
Review Comment:
This assertion passes because `MemoryStorageFactory::build` always hands
back a fresh empty `MemoryStorage`, not because the `OnceLock` cache was reset
— it'd stay green even if serde were subtly broken, as long as `build()`
returns fresh storage. So it doesn't actually verify config fidelity or that
the right concrete factory came back.
A `LocalFs` case would be more convincing: write a file, serialize,
deserialize, then assert `exists == true` and the config props survived. Maybe
also pin the serialized JSON with a snapshot assertion, so a rename of the type
tag or a config field name doesn't silently pass.
--
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]
