You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Revanth14 (via GitHub)" <gi...@apache.org> on 2026/08/23 16:21:40 UTC
[PR] fix(table): validate required snapshot fields [iceberg-go]
Revanth14 opened a new pull request, #1871:
URL: https://github.com/apache/iceberg-go/pull/1871
### Description
Validate required snapshot fields during JSON decoding:
- Reject missing or null `snapshot-id`.
- Reject missing or null `timestamp-ms`.
- Reject null snapshot entries.
- Return errors wrapping `table.ErrInvalidMetadata`.
- Preserve explicit zero values and leave the receiver unchanged on failure.
The validation also propagates through table metadata and `add-snapshot` decoding.
### Why?
Previously, malformed snapshots silently decoded missing IDs and timestamps as `0`. This could create invalid snapshot identities and Unix-epoch timestamps.
Both fields are required by the Iceberg specification, and Java rejects them when missing or null.
### Tests
Added coverage for missing, null, and explicit-zero values, null snapshots, metadata parsing, and receiver preservation.
Validated with:
- `go test ./... -count=1`
- `go vet -tags=integration ./...`
- `golangci-lint run ./table/...`
Closes #1869
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3860265667
##########
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"`
Review Comment:
Thanks for pointing these out. `SnapshotLogEntry` and `MetadataLogEntry` appear to need similar required-field validation, but I’d prefer to address them separately to keep this PR focused on #1869.
The manifest requirement is also version-dependent: v1 may use the legacy `manifests` field, while `manifest-list` is required in v2 and v3. Since `Snapshot.UnmarshalJSON` does not know the table format version, this validation would be better handled at the metadata level in a follow-up.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "ferhatelmas (via GitHub)" <gi...@apache.org>.
ferhatelmas commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3848031729
##########
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"`
Review Comment:
similar problem exists in other types (`SnapshotLogEntry`, `MetadataLogEntry`, etc.) as well
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3860326318
##########
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"`
Review Comment:
Thanks, you’re right that `SnapshotLogEntry` and `MetadataLogEntry` have analogous required-field validation gaps. Since #1869 is specifically scoped to `Snapshot.snapshot-id` and `timestamp-ms`, I’d prefer to track those separately in a follow-up issue.
For manifest locations, v1 snapshots may use `manifests`, while newer snapshots normally use `manifest-list`. Although `Snapshot.UnmarshalJSON` does not know the table format version, it can still require at least one of those representations to be present. Format-version-specific enforcement would need to remain at the metadata level to preserve compatibility with legacy snapshots in upgraded tables.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3860265667
##########
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"`
Review Comment:
Thanks for pointing these out. `SnapshotLogEntry` and `MetadataLogEntry` appear to need similar required-field validation, but I’d prefer to address them separately to keep this PR focused on #1869.
The manifest requirement is also version-dependent: v1 may use the legacy `manifests` field, while `manifest-list` is required in v2 and v3. Since `Snapshot.UnmarshalJSON` does not know the table format version, this validation would be better handled at the metadata level in a follow-up.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3855268139
##########
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)
Review Comment:
`ParseMetadataBytes` wraps every `json.Unmarshal` failure with `ErrInvalidMetadata`, so wrapping the same sentinel here produces duplicated public text: `invalid metadata: invalid metadata: snapshot-id is absent or null`. Please avoid the outer wrap when the nested error already satisfies `errors.Is(err, ErrInvalidMetadata)` (and add an exact error-text assertion so this does not regress).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3860334519
##########
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}
Review Comment:
Thanks, that makes sense. I’ll decode into the alias first and only inspect the raw fields when either `snapshot-id` or `timestamp-ms` has its zero value.
The fallback will check both field presence and JSON `null`, so missing or null values remain invalid while explicit numeric zero remains accepted. This keeps the normal nonzero path on the existing direct decode.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "ferhatelmas (via GitHub)" <gi...@apache.org>.
ferhatelmas commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3848052579
##########
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}
Review Comment:
nit: perf
I would suggest letting it parse the type itself first but if there is a default value then check existence (via `map[string]json.RawMessage`) so that price is only paid for wrong payload and regular branch isn't affected
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3860204087
##########
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)
+ }
Review Comment:
Fixed. `ParseMetadataBytes` now returns errors that already match `ErrInvalidMetadata` without wrapping them again. I also updated the tests to assert the exact error text, so a duplicate prefix will be caught. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "ferhatelmas (via GitHub)" <gi...@apache.org>.
ferhatelmas commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3848005988
##########
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)
+ }
Review Comment:
this will be doing double prefix
```
invalid metadata: invalid metadata: snapshot-id is absent or null
```
tests can't catch since only checking suffix
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "ferhatelmas (via GitHub)" <gi...@apache.org>.
ferhatelmas commented on code in PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871#discussion_r3848035873
##########
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"`
Review Comment:
specifically for snapshot, how about manifests or manifest-list required check here?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate required snapshot fields [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1871:
URL: https://github.com/apache/iceberg-go/pull/1871
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org