You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by ze...@apache.org on 2026/08/26 18:02:17 UTC
(iceberg-go) branch main updated: fix(table): validate required snapshot fields (#1871)
This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new 34970d42c fix(table): validate required snapshot fields (#1871)
34970d42c is described below
commit 34970d42ccc406dd6a943889e33cc6b1f61dd4b2
Author: Revanth <re...@gmail.com>
AuthorDate: Wed Aug 26 13:02:11 2026 -0500
fix(table): validate required snapshot fields (#1871)
* fix(table): validate required snapshot fields
* fix(table): address snapshot validation feedback
---
table/metadata.go | 4 ++
table/snapshots.go | 21 ++++++-
table/snapshots_test.go | 163 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 187 insertions(+), 1 deletion(-)
diff --git a/table/metadata.go b/table/metadata.go
index 837db81ed..53b781c07 100644
--- a/table/metadata.go
+++ b/table/metadata.go
@@ -1747,6 +1747,10 @@ func ParseMetadataBytes(b []byte) (Metadata, error) {
}
if err := json.Unmarshal(normalized, ret); err != nil {
+ if errors.Is(err, ErrInvalidMetadata) {
+ return nil, err
+ }
+
return nil, fmt.Errorf("%w: %w", ErrInvalidMetadata, err)
}
diff --git a/table/snapshots.go b/table/snapshots.go
index 1e66bc088..8869515c1 100644
--- a/table/snapshots.go
+++ b/table/snapshots.go
@@ -272,9 +272,28 @@ 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)
+ }
+ // The pointer fields shadow Alias, so json.Unmarshal does not populate next.
+ next.SnapshotID, next.TimestampMs = *aux.SnapshotID, *aux.TimestampMs
+
if next.ManifestList != "" {
next.ManifestLocations = nil
}
diff --git a/table/snapshots_test.go b/table/snapshots_test.go
index 6f46d63b7..b6b5f2311 100644
--- a/table/snapshots_test.go
+++ b/table/snapshots_test.go
@@ -20,6 +20,7 @@ package table_test
import (
"bytes"
"encoding/json"
+ "os"
"testing"
"github.com/apache/iceberg-go"
@@ -421,3 +422,165 @@ 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",
+ },
+ {
+ name: "both fields missing",
+ data: `{"manifests": []}`,
+ wantErr: "snapshot-id 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 TestSnapshotUnmarshalPreservesRequiredFields(t *testing.T) {
+ var snapshot table.Snapshot
+ require.NoError(t, json.Unmarshal([]byte(`{
+ "snapshot-id": 1234,
+ "timestamp-ms": 5678,
+ "manifests": []
+ }`), &snapshot))
+
+ assert.Equal(t, int64(1234), snapshot.SnapshotID)
+ assert.Equal(t, int64(5678), snapshot.TimestampMs)
+}
+
+func TestSnapshotUnmarshalFailureLeavesSnapshotUnchanged(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ }{
+ {
+ name: "missing snapshot-id",
+ data: `{
+ "timestamp-ms": 1602638573590,
+ "manifest-list": "s3:/a/b/new.avro"
+ }`,
+ },
+ {
+ name: "missing timestamp-ms",
+ data: `{
+ "snapshot-id": 26,
+ "manifest-list": "s3:/a/b/new.avro"
+ }`,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ snapshot := Snapshot()
+ original := Snapshot()
+
+ err := json.Unmarshal([]byte(tt.data), &snapshot)
+ require.ErrorIs(t, err, table.ErrInvalidMetadata)
+ assert.True(t, snapshot.Equals(original), "expected snapshot to be untouched, got %s", snapshot)
+ assert.Equal(t, original.ManifestList, snapshot.ManifestList)
+ })
+ }
+}
+
+// A null snapshot carries neither identity nor timestamp, so it is rejected
+// rather than decoded into a zero-value snapshot. Java's SnapshotParser also
+// rejects null, but at its earlier object-node precondition.
+func TestSnapshotUnmarshalRejectsNullDocument(t *testing.T) {
+ var snapshot table.Snapshot
+ err := json.Unmarshal([]byte(`null`), &snapshot)
+ require.ErrorIs(t, err, table.ErrInvalidMetadata)
+ assert.ErrorContains(t, err, "snapshot-id is absent or null")
+}
+
+func TestParseMetadataRejectsSnapshotMissingRequiredFields(t *testing.T) {
+ raw, err := os.ReadFile("testdata/TableMetadataV2Valid.json")
+ require.NoError(t, err)
+
+ tests := []struct {
+ name string
+ mutate func(snapshots []any)
+ wantErr string
+ }{
+ {
+ name: "missing snapshot-id",
+ mutate: func(snapshots []any) { delete(snapshots[0].(map[string]any), "snapshot-id") },
+ wantErr: "invalid metadata: snapshot-id is absent or null",
+ },
+ {
+ name: "missing timestamp-ms",
+ mutate: func(snapshots []any) { delete(snapshots[0].(map[string]any), "timestamp-ms") },
+ wantErr: "invalid metadata: timestamp-ms is absent or null",
+ },
+ {
+ name: "null snapshot",
+ mutate: func(snapshots []any) { snapshots[0] = nil },
+ wantErr: "invalid metadata: snapshot-id is absent or null",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ var metadata map[string]any
+ decoder := json.NewDecoder(bytes.NewReader(raw))
+ decoder.UseNumber() // Preserve snapshot IDs larger than float64 can represent exactly.
+ require.NoError(t, decoder.Decode(&metadata))
+
+ snapshots, ok := metadata["snapshots"].([]any)
+ require.True(t, ok)
+ require.NotEmpty(t, snapshots)
+ tt.mutate(snapshots)
+
+ mutated, err := json.Marshal(metadata)
+ require.NoError(t, err)
+
+ _, err = table.ParseMetadataBytes(mutated)
+ require.ErrorIs(t, err, table.ErrInvalidMetadata)
+ assert.EqualError(t, err, tt.wantErr)
+ })
+ }
+}