laskoviymishka commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3844838741
##########
table/snapshots.go:
##########
@@ -272,9 +272,27 @@ type Snapshot struct {
func (s *Snapshot) UnmarshalJSON(data []byte) error {
type Alias Snapshot
var next Alias
- if err := json.Unmarshal(data, &next); err != nil {
+ // snapshot-id and timestamp-ms are required by the spec for every
format
+ // version. Decode them through pointers so an absent or null field
stays
+ // distinguishable from an explicit zero, which would otherwise be
accepted
+ // as a valid identity or as the Unix epoch.
+ aux := struct {
+ SnapshotID *int64 `json:"snapshot-id"`
+ TimestampMs *int64 `json:"timestamp-ms"`
+ *Alias
+ }{Alias: &next}
+
+ if err := json.Unmarshal(data, &aux); err != nil {
return err
}
+ if aux.SnapshotID == nil {
+ return fmt.Errorf("%w: snapshot-id is absent or null",
ErrInvalidMetadata)
+ }
+ if aux.TimestampMs == nil {
+ return fmt.Errorf("%w: timestamp-ms is absent or null",
ErrInvalidMetadata)
+ }
+ next.SnapshotID, next.TimestampMs = *aux.SnapshotID, *aux.TimestampMs
Review Comment:
This write-back is load-bearing in a way that's easy to miss. The `aux`
`*int64` fields shadow the promoted `Alias` fields, so `json.Unmarshal` writes
`snapshot-id`/`timestamp-ms` into `aux` and never into `next`. Drop this line
and every valid snapshot silently decodes with `SnapshotID==0` and
`TimestampMs==0`, with no compile error and nothing in the current tests
catching it.
I'd pin the invariant with a round-trip assertion on a known non-zero id
(decode `{"snapshot-id": 1234, "timestamp-ms": 5678, ...}` and assert both come
back), and add a one-line comment here noting that `aux` shadows the promoted
fields so the decoder never touches `next` directly. That way a future refactor
can't quietly bring the zero-value bug back.
##########
table/snapshots_test.go:
##########
@@ -421,3 +422,126 @@ func TestValidateRowLineage(t *testing.T) {
func ptr[T any](v T) *T {
return &v
}
+
+func TestSnapshotUnmarshalRequiresSnapshotIDAndTimestamp(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ wantErr string
+ }{
+ {
+ name: "missing snapshot-id",
+ data: `{"timestamp-ms": 1602638573590, "manifests":
[]}`,
+ wantErr: "snapshot-id is absent or null",
+ },
+ {
+ name: "null snapshot-id",
+ data: `{"snapshot-id": null, "timestamp-ms":
1602638573590, "manifests": []}`,
+ wantErr: "snapshot-id is absent or null",
+ },
+ {
+ name: "missing timestamp-ms",
+ data: `{"snapshot-id": 25, "manifests": []}`,
+ wantErr: "timestamp-ms is absent or null",
+ },
+ {
+ name: "null timestamp-ms",
+ data: `{"snapshot-id": 25, "timestamp-ms": null,
"manifests": []}`,
+ wantErr: "timestamp-ms is absent or null",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ var snapshot table.Snapshot
+ err := json.Unmarshal([]byte(tt.data), &snapshot)
+ require.ErrorIs(t, err, table.ErrInvalidMetadata)
+ assert.ErrorContains(t, err, tt.wantErr)
+ })
+ }
+}
+
+// Zero is a legal value for both fields, so presence must be tracked
+// separately from the decoded value.
+func TestSnapshotUnmarshalAcceptsExplicitZeroValues(t *testing.T) {
+ var snapshot table.Snapshot
+ require.NoError(t, json.Unmarshal([]byte(`{
+ "snapshot-id": 0,
+ "timestamp-ms": 0,
+ "manifests": []
+ }`), &snapshot))
+
+ assert.Zero(t, snapshot.SnapshotID)
+ assert.Zero(t, snapshot.TimestampMs)
+ assert.NotNil(t, snapshot.ManifestLocations)
+}
+
+func TestSnapshotUnmarshalFailureLeavesSnapshotUnchanged(t *testing.T) {
+ snapshot := Snapshot()
+ original := Snapshot()
+
+ err := json.Unmarshal([]byte(`{
+ "timestamp-ms": 1602638573590,
+ "manifest-list": "s3:/a/b/new.avro"
+ }`), &snapshot)
+ require.ErrorIs(t, err, table.ErrInvalidMetadata)
+ assert.True(t, snapshot.Equals(original), "expected snapshot to be
untouched, got %s", snapshot)
+}
+
+// A null snapshot carries neither identity nor timestamp, so it is rejected
+// rather than decoded into a zero-value snapshot. Java's SnapshotParser
+// likewise refuses a null node.
Review Comment:
Small accuracy note on the Java comparison. Java's `SnapshotParser` does
reject a null node, but earlier and for a different reason: `fromJson` fails
the `node.isObject()` precondition ("Cannot parse ... non-object"), not a
missing `snapshot-id`. Go reaches the same outcome via `aux.SnapshotID` being
nil after the no-op unmarshal. I'd reword the comment so it doesn't imply the
two hit the same check.
##########
table/snapshots_test.go:
##########
@@ -421,3 +422,126 @@ func TestValidateRowLineage(t *testing.T) {
func ptr[T any](v T) *T {
return &v
}
+
+func TestSnapshotUnmarshalRequiresSnapshotIDAndTimestamp(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ wantErr string
+ }{
+ {
+ name: "missing snapshot-id",
+ data: `{"timestamp-ms": 1602638573590, "manifests":
[]}`,
+ wantErr: "snapshot-id is absent or null",
+ },
+ {
+ name: "null snapshot-id",
+ data: `{"snapshot-id": null, "timestamp-ms":
1602638573590, "manifests": []}`,
+ wantErr: "snapshot-id is absent or null",
+ },
+ {
+ name: "missing timestamp-ms",
+ data: `{"snapshot-id": 25, "manifests": []}`,
+ wantErr: "timestamp-ms is absent or null",
+ },
+ {
+ name: "null timestamp-ms",
+ data: `{"snapshot-id": 25, "timestamp-ms": null,
"manifests": []}`,
+ wantErr: "timestamp-ms is absent or null",
+ },
Review Comment:
One more case worth adding: both fields absent, e.g. `{"manifests": []}`,
asserting the `snapshot-id` error wins. The ordering is deterministic today,
but a case here documents that contract so a reorder of the two nil-checks
shows up as a test change rather than a silent behavior shift.
##########
table/snapshots_test.go:
##########
@@ -421,3 +422,126 @@ func TestValidateRowLineage(t *testing.T) {
func ptr[T any](v T) *T {
return &v
}
+
+func TestSnapshotUnmarshalRequiresSnapshotIDAndTimestamp(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ wantErr string
+ }{
+ {
+ name: "missing snapshot-id",
+ data: `{"timestamp-ms": 1602638573590, "manifests":
[]}`,
+ wantErr: "snapshot-id is absent or null",
+ },
+ {
+ name: "null snapshot-id",
+ data: `{"snapshot-id": null, "timestamp-ms":
1602638573590, "manifests": []}`,
+ wantErr: "snapshot-id is absent or null",
+ },
+ {
+ name: "missing timestamp-ms",
+ data: `{"snapshot-id": 25, "manifests": []}`,
+ wantErr: "timestamp-ms is absent or null",
+ },
+ {
+ name: "null timestamp-ms",
+ data: `{"snapshot-id": 25, "timestamp-ms": null,
"manifests": []}`,
+ wantErr: "timestamp-ms is absent or null",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ var snapshot table.Snapshot
+ err := json.Unmarshal([]byte(tt.data), &snapshot)
+ require.ErrorIs(t, err, table.ErrInvalidMetadata)
+ assert.ErrorContains(t, err, tt.wantErr)
+ })
+ }
+}
+
+// Zero is a legal value for both fields, so presence must be tracked
+// separately from the decoded value.
+func TestSnapshotUnmarshalAcceptsExplicitZeroValues(t *testing.T) {
+ var snapshot table.Snapshot
+ require.NoError(t, json.Unmarshal([]byte(`{
+ "snapshot-id": 0,
+ "timestamp-ms": 0,
+ "manifests": []
+ }`), &snapshot))
+
+ assert.Zero(t, snapshot.SnapshotID)
+ assert.Zero(t, snapshot.TimestampMs)
+ assert.NotNil(t, snapshot.ManifestLocations)
+}
+
+func TestSnapshotUnmarshalFailureLeavesSnapshotUnchanged(t *testing.T) {
+ snapshot := Snapshot()
+ original := Snapshot()
+
+ err := json.Unmarshal([]byte(`{
+ "timestamp-ms": 1602638573590,
+ "manifest-list": "s3:/a/b/new.avro"
+ }`), &snapshot)
+ require.ErrorIs(t, err, table.ErrInvalidMetadata)
+ assert.True(t, snapshot.Equals(original), "expected snapshot to be
untouched, got %s", snapshot)
Review Comment:
The `Equals` check here passes a bit coincidentally. The failing input sets
`manifest-list`, which the decoder does write into the intermediate `next`
before the `snapshot-id` nil-check fires, so what we actually want to prove is
that none of that leaked into the receiver. `Equals` only gets us there because
the receiver happens to still match `original` on the fields it compares.
I'd assert directly on a field the partial decode would have touched, e.g.
`assert.Equal(t, "", snapshot.ManifestList)` alongside the `Equals`, so the
intent is unambiguous. And since `timestamp-ms` goes through a different branch
than `snapshot-id`, a second variant here (snapshot-id present, timestamp-ms
absent) would cover the symmetric case. wdyt?
--
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]
