laskoviymishka commented on code in PR #1406:
URL: https://github.com/apache/iceberg-go/pull/1406#discussion_r3570254056
##########
table/encryption.go:
##########
@@ -27,6 +31,18 @@ type EncryptionKey struct {
Properties map[string]string `json:"properties,omitempty"`
}
+func (e EncryptionKey) Validate() error {
+ if strings.TrimSpace(e.KeyID) == "" {
+ return errors.New("encryption key-id must be non-empty")
+ }
+
+ if strings.TrimSpace(e.EncryptedKeyMetadata) == "" {
Review Comment:
`TrimSpace` on `EncryptedKeyMetadata` doesn't really buy us anything — this
field is a base64 blob, and whitespace is never valid base64, so the check
collapses to `== ""` in practice.
If we actually want to validate the field the way the spec describes it
(base64-encoded), I'd do a real decode check —
`base64.StdEncoding.DecodeString(...)` and reject on error. That catches
genuinely malformed values, and it matches iceberg-rust, which hard-errors
decoding this field on read — so a table we write with a non-base64 value would
be unreadable there.
Separately on `KeyID`: `TrimSpace` there is defensible, but the dedup loop
just below compares raw `k.KeyID == key.KeyID`, so `" key "` and `" key "`
land as distinct entries and you can't `RemoveEncryptionKey` with the trimmed
form. If we're going to trim for validation, I'd store the trimmed value too
(or just reject padded ids). Whichever we pick, worth a test pinning it. wdyt?
##########
table/encryption.go:
##########
@@ -27,6 +31,18 @@ type EncryptionKey struct {
Properties map[string]string `json:"properties,omitempty"`
}
+func (e EncryptionKey) Validate() error {
Review Comment:
`Validate()` is exported but nothing outside the package calls it — callers
hand keys to `AddEncryptionKey` and let it validate. Every other validation
surface in here (`commonMetadata.validate`, `metadataV3.validate`) is
unexported, so this widens the public API for something internal, and the rules
become a breaking change to touch later.
The errors it returns are also plain `errors.New`, which makes the
`fmt.Errorf("%w: %w", iceberg.ErrInvalidArgument, err)` at the call site a bit
of a dead end — the second `%w` promises `errors.Is` matchability on a value no
caller can reference.
I'd either unexport it to `validate()`, or if there's a reason to keep it
public, have it wrap with `ErrInvalidArgument` itself and drop the re-wrap.
wdyt?
##########
table/updates_test.go:
##########
@@ -884,6 +884,37 @@ func TestAddEncryptionKeyUpdate_Apply_RejectsV2(t
*testing.T) {
assert.Contains(t, err.Error(), "format version 3")
}
+func TestAddEncryptionKeyUpdate_Apply_RejectsMissingKeyID(t *testing.T) {
+ b := buildFromBaseV3(t)
+ key := EncryptionKey{KeyID: "", EncryptedKeyMetadata: "dGVzdA=="}
+
+ err := NewAddEncryptionKeyUpdate(key).Apply(b)
+ require.Error(t, err)
+ assert.ErrorIs(t, err, iceberg.ErrInvalidArgument)
+ assert.Contains(t, err.Error(), "key-id")
+}
+
+func TestAddEncryptionKeyUpdate_Apply_RejectsMissingEncryptedKeyMetadata(t
*testing.T) {
+ b := buildFromBaseV3(t)
+ key := EncryptionKey{KeyID: "my-key", EncryptedKeyMetadata: ""}
+
+ err := NewAddEncryptionKeyUpdate(key).Apply(b)
+ require.Error(t, err)
+ assert.ErrorIs(t, err, iceberg.ErrInvalidArgument)
+ assert.Contains(t, err.Error(), "metadata")
+}
+
+func TestAddEncryptionKeyUpdate_UnmarshalMissingFields_ApplyRejects(t
*testing.T) {
+ data :=
[]byte(`[{"action":"add-encryption-key","encryption-key":{"key-id":"my-key"}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ err := updates[0].Apply(buildFromBaseV3(t))
+ require.ErrorIs(t, err, iceberg.ErrInvalidArgument)
Review Comment:
The other two tests assert both `ErrorIs` and a `Contains` on the field name
— this one only checks `ErrorIs`. If the error ever fires for a different
reason (a future version guard, say), this still passes while testing the wrong
thing. I'd add `assert.Contains(t, err.Error(), "key-id")` to pin that it's the
missing field being rejected.
Minor while we're here: the three new tests use `assert.ErrorIs` where the
rest of the file uses `require.ErrorIs` — worth matching.
##########
table/updates_test.go:
##########
@@ -884,6 +884,37 @@ func TestAddEncryptionKeyUpdate_Apply_RejectsV2(t
*testing.T) {
assert.Contains(t, err.Error(), "format version 3")
}
+func TestAddEncryptionKeyUpdate_Apply_RejectsMissingKeyID(t *testing.T) {
Review Comment:
All three cases go through `NewAddEncryptionKeyUpdate(key).Apply` — none
calls `b.AddEncryptionKey` directly, which is the exported, documented entry
point the validation actually lives on. If `Apply` ever gets refactored to not
route through `AddEncryptionKey`, these still pass while the builder contract
silently breaks. I'd add one case hitting `b.AddEncryptionKey(key)` directly.
--
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]
