laskoviymishka commented on code in PR #3090:
URL: https://github.com/apache/iceberg-rust/pull/3090#discussion_r3892690476
##########
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:
This is the accurate version of the "hard to change later" worry from
round-1 — the disclaimer covers the JSON shape, but `impl Serialize/Deserialize
for FileIO` landing in `public-api.txt` is a semver commitment on its own,
independent of the format. If we ever swap the mechanism (typetag → something
else) and drop the impls, that's a breaking change for anyone doing
`serde_json::to_string(&file_io)` or deriving over a wrapper.
I'd gate the derives behind a `serde` feature now while it's a one-liner —
`#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]` — so the impls
are opt-in rather than a permanent part of the surface. Not blocking, but much
cheaper now than walking it back.
##########
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:
The loader fail-fast landed cleanly, and I flagged the lock-in risk last
round — the unstable-format disclaimer plus dropping the shape-pin tests
settles that half, so I'm no longer worried the format is hard to change.
What's still here is the asymmetry: the loader now hard-errors on serialize,
but the actual secret strings — `s3.secret-access-key`, `s3.session-token`, GCS
service-account JSON, `hf.token` — pass straight through in plaintext, and this
test pins that as intended. It reads backwards — we fail fast on the opaque
process-local handle and silently emit the values most dangerous to leak.
Someone who sees the loader path refuse to serialize will reasonably assume the
path is credential-aware, and it isn't.
A FileIO written to a log, telemetry, or disk unencrypted then leaks those
verbatim (and for vended `s3.session-token` there's a second trap — it's
short-lived, so a blob deserialized after the TTL silently fails auth with no
expiry signal). I'd like safe-by-default before merge: redact or opaque-wrap
the well-known credential keys in the serialized props, with full-fidelity
passthrough as an explicit opt-in — a flag or a `serialize_with_credentials`
path. That keeps the roundtrip usable for plain configs while making the
dangerous case safe unless you ask for it. wdyt?
##########
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:
The intra-doc link to `OpenDalStorageFactory::S3` here points to a variant
that only exists under `opendal-s3`. Under `--no-default-features` docs (which
ASF CI tends to run with `-D warnings`), rustdoc's broken-intra-doc-links lint
turns this into a hard error.
Easiest is to drop the link and use a plain back-tick name, or gate the
paragraph with `#[cfg_attr(feature = "opendal-s3", doc = "...")]`.
##########
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:
Small thing — the `None` arm here is never reached: `skip_serializing_if =
"Option::is_none"` at the call-site short-circuits the field before
`serialize_with` runs, so only the `Some(_)` error path ever fires. The
fail-fast is correct, this arm is just dead.
I'd either drop the `skip_serializing_if` and let this fn own the None case,
or keep the attribute and collapse the body to the `Some` path with a comment
that None never gets here — right now the two don't quite agree.
##########
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:
This new doc block covers the loader case well. One thing it doesn't cover:
the variants here are feature-gated, so a blob serialized in a binary built
with `opendal-gcs` fails to deserialize in one without it — serde reports
`unknown variant "Gcs", expected one of [...]` listing only the compiled-in
variants, with no hint that a feature flag is the cause.
Worth a sentence noting the receiving binary must be built with the same
`opendal-*` features. A `#[serde(other)]` catch-all would also give a cleaner
error, but the doc note is the cheap part.
--
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]
