feat(io): make FileIO serializable - #3090
Conversation
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice, the mechanical wiring here is clean. The #[serde(skip)] on the cache with the OnceLock rebuilding lazily is exactly right, and the roundtrip does what it says.
The one thing I'd want to settle before this merges is the credential story. Serializing a FileIO emits the whole StorageConfig.props map verbatim, so an S3-configured one writes its secret access key and session token into the output in plaintext. The stated use case is shipping these across process boundaries, which is exactly where a plaintext credential blob is most dangerous — and because these serde impls are now public API, the format is hard to change later. I'd like the serialized form to be safe by default before we commit to it.
Stepping back a little: did we consider serializing just the StorageConfig plus a factory discriminant and reconstructing through FileIOBuilder, rather than deriving on FileIO directly? That's the pattern the REST catalog already uses to rebuild FileIO from properties, and it's how Java/PyIceberg/iceberg-go all handle it — config in, live object rebuilt. It keeps runtime state out of the wire format and gives us a natural spot to redact. Not a hard blocker, but worth weighing before this becomes API.
A few smaller things I left inline: the custom credential loader gets silently dropped on roundtrip, the new serde behavior (typetag registry, ephemeral storage) needs docs, and the test only exercises MemoryStorageFactory so it doesn't really prove roundtrip fidelity.
Once the credential handling's settled, happy to take another pass and approve.
|
cc @laskoviymishka Comments address, PTAL |
laskoviymishka
left a comment
There was a problem hiding this comment.
This is really close now — thanks for the thorough revision.
The one thing still holding me is credential safe-by-default, and it's narrower than last round. I flagged the lock-in risk before; the unstable-format disclaimer plus dropping the exact-shape test pins settles that, so I'm no longer worried the format is hard to change. What's left is that the loader now hard-errors on serialize while the secret strings — s3.secret-access-key, s3.session-token, GCS service-account JSON, hf.token — still pass through in plaintext (and test_memory_file_io_serialization_roundtrip pins that). The asymmetry reads backwards: we fail fast on the opaque loader handle and silently emit the values most dangerous to leak. I'd really like by-default redaction of the well-known credential keys before we merge, with full-fidelity passthrough as an explicit opt-in.
Everything else I asked for last round is in:
- loader no longer silently dropped — it hard-fails serialization now, with fail-fast tests for both
OpenDalStorageFactory::S3andOpenDalResolvingStorageFactory(stronger than I asked for) - serialization docs on
FileIO,OpenDalStorageFactory, andOpenDalResolvingStorageFactory, including the ephemeral-storage and credential-sensitivity notes - real-I/O roundtrip tests across memory / fs / s3 / gcs / hf / resolving, replacing the old shape-pinning tests
- the unstable-format disclaimer
I left a few smaller notes inline — a serde feature gate I'd like while it's cheap, a couple of doc/CI catches, and one dead match arm — but none of those block. Sort the credential default and I'm happy to approve.
9b57574 to
5b50b1f
Compare
|
hi, @laskoviymishka thanks for review, I addressed all comments, PTAL.
While I agree that we should redact sensitive credentials, I don't think maintaining a predefine set of well know keys is the right direction. I think #3129 is the right direction to go, e.g. replacing raw hash map with a config struct. For now I've removed the derived easy to use ser/de traits, and ask user to use explict |
|
Thanks @laskoviymishka and @CTTY for review. |
Which issue does this PR close?
What changes are included in this PR?
FileIO::serialize_allandFileIO::deserialize_allbyte APIs without implementing serde's publicSerializeorDeserializetraits forFileIO.Are these changes tested?
cargo test -p iceberg io::file_io::tests --libcargo test -p iceberg-storage-opendal --all-features --lib custom_credential_loadercargo test -p iceberg-storage-opendal --all-features --test file_io_memory_test --test file_io_fs_testRUSTDOCFLAGS='-D warnings' cargo doc -p iceberg-storage-opendal --no-default-features --no-depscargo check -p iceberg-storage-opendal --no-default-features --all-targetscargo clippy --all-targets --all-features --workspace -- -D warningscargo test --doc --all-features --workspacecargo machetecargo fmt --all -- --checkcargo public-api -p iceberg --all-features -ss | diff - crates/iceberg/public-api.txtAI Disclosure
Codex was used to help implement the change, add the regression tests and documentation, and run verification. The resulting code, documentation, and test behavior were reviewed before submission.