You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/06/27 10:30:22 UTC
[PR] fix(table): reject nil uuid assignment updates [iceberg-go]
fallintoplace opened a new pull request, #1329:
URL: https://github.com/apache/iceberg-go/pull/1329
Summary
reject uuid.Nil in the table metadata builder's SetUUID path
add regression coverage for direct builder calls and assign-uuid updates applied through the builder
preserve the existing NewMetadata/NewMetadataWithUUID behavior where uuid.Nil means "generate a fresh table UUID" for brand-new metadata
Why
New table creation already treats uuid.Nil as a sentinel for auto-generation, but the mutable table metadata builder still accepted uuid.Nil later through SetUUID and assign-uuid updates. That made it possible to write the zero UUID into table metadata even though the table UUID is supposed to uniquely identify the table.
Testing
go test ./table -run 'Test(SetUUIDRejectsNil|AssignUUIDUpdate_ApplyRejectsNilUUID|SetFormatVersionV1ToV2AssignsUUID)$' -count=1
go test ./table -count=1
go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.4 run --timeout=10m ./table/...
git diff --check
--
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): reject nil uuid assignment updates [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1329:
URL: https://github.com/apache/iceberg-go/pull/1329#discussion_r3491221909
##########
table/metadata.go:
##########
@@ -902,13 +902,17 @@ func (b *MetadataBuilder) RemoveSnapshotRef(name string) error {
return nil
}
-func (b *MetadataBuilder) SetUUID(uuid uuid.UUID) error {
- if b.uuid == uuid {
+func (b *MetadataBuilder) SetUUID(newUUID uuid.UUID) error {
+ if newUUID == uuid.Nil {
+ return errors.New("cannot set uuid to null")
Review Comment:
This is the one thing I'd want fixed before merge. Every other input-validation error in `MetadataBuilder` (`SetCurrentSchemaID`, `SetDefaultSortOrderID`, `SetDefaultSpecID`, `SetFormatVersion`, …) wraps `iceberg.ErrInvalidArgument`, so this is the only public validation path that breaks the `errors.Is` contract — a catalog layer can't classify it without string-matching.
```suggestion
return fmt.Errorf("%w: cannot set uuid to nil", iceberg.ErrInvalidArgument)
```
With that, both new tests can move from `require.ErrorContains` to `require.ErrorIs(t, err, iceberg.ErrInvalidArgument)`, which is the more robust assertion anyway.
##########
table/metadata.go:
##########
@@ -902,13 +902,17 @@ func (b *MetadataBuilder) RemoveSnapshotRef(name string) error {
return nil
}
-func (b *MetadataBuilder) SetUUID(uuid uuid.UUID) error {
- if b.uuid == uuid {
+func (b *MetadataBuilder) SetUUID(newUUID uuid.UUID) error {
+ if newUUID == uuid.Nil {
+ return errors.New("cannot set uuid to null")
Review Comment:
Minor, but the Go zero value here is `uuid.Nil`, so "null" reads as a port of the Java message — I'd say "nil" to match the language. The view builder uses "null" too, so this is a wash on internal consistency, but if you take the wrapping suggestion above this folds into the same line.
##########
table/metadata_builder_internal_test.go:
##########
@@ -2317,6 +2317,20 @@ func TestSetFormatVersionPreservesExistingUUID(t *testing.T) {
require.Equal(t, existingUUID, meta.TableUUID())
}
+func TestSetUUIDRejectsNil(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ originalUUID := builder.uuid
+
+ err := builder.SetUUID(uuid.Nil)
+ require.ErrorContains(t, err, "cannot set uuid to null")
+ require.False(t, builder.HasChanges())
+ require.Equal(t, originalUUID, builder.uuid)
+
+ meta, err := builder.Build()
+ require.NoError(t, err)
+ require.Equal(t, originalUUID, meta.TableUUID())
Review Comment:
The whole design rests on `NewMetadata`/`NewMetadataWithUUID` converting `uuid.Nil` to `uuid.New()` *before* reaching the new guard, but nothing here asserts it. A future regression where that conversion gets dropped would now turn into a hard "cannot set uuid" error instead of a generated UUID, and we'd have no test catching it. Could we add a quick case calling `NewMetadataWithUUID(..., uuid.Nil)` and asserting `meta.TableUUID() != uuid.Nil`? Cheap, and it locks down the path the guard relies on.
##########
table/updates_test.go:
##########
@@ -569,6 +569,18 @@ func buildFromBase(t *testing.T) *MetadataBuilder {
return b
}
+func TestAssignUUIDUpdate_ApplyRejectsNilUUID(t *testing.T) {
+ b := buildFromBase(t)
+
+ err := NewAssignUUIDUpdate(uuid.Nil).Apply(b)
+ require.ErrorContains(t, err, "cannot set uuid to null")
+ require.False(t, b.HasChanges())
+
+ meta, err := b.Build()
+ require.NoError(t, err)
+ require.NotEqual(t, uuid.Nil, meta.TableUUID())
Review Comment:
`NotEqual(uuid.Nil, ...)` passes for any non-nil value, so this doesn't actually prove the nil update was a no-op — the builder could have mutated to some other UUID and this still goes green. Since `buildFromBase` parses a real metadata file, I'd capture that original UUID up front and assert `meta.TableUUID()` equals it, the way `TestSetUUIDRejectsNil` checks `originalUUID`. That nails down "rejected and left untouched" rather than just "not nil".
--
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): reject nil uuid assignment updates [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1329:
URL: https://github.com/apache/iceberg-go/pull/1329#discussion_r3486531274
##########
table/metadata.go:
##########
@@ -902,13 +902,17 @@ func (b *MetadataBuilder) RemoveSnapshotRef(name string) error {
return nil
}
-func (b *MetadataBuilder) SetUUID(uuid uuid.UUID) error {
- if b.uuid == uuid {
+func (b *MetadataBuilder) SetUUID(newUUID uuid.UUID) error {
+ if newUUID == uuid.Nil {
+ return errors.New("cannot set uuid to null")
+ }
+
+ if b.uuid == newUUID {
Review Comment:
Optional / possibly out of scope: Java's assignUUID enforces a second invariant beyond non-nul, "Cannot reassign uuid", i.e. once a non-nil UUID is set you can't change it to a different one. Here SetUUID
still allows reassigning to a different non-nil UUID. Given the rationale ("the table UUID uniquely identifies the table"), do you want to reject that case too, or keep it for a separate PR?
##########
table/metadata.go:
##########
@@ -902,13 +902,17 @@ func (b *MetadataBuilder) RemoveSnapshotRef(name string) error {
return nil
}
-func (b *MetadataBuilder) SetUUID(uuid uuid.UUID) error {
Review Comment:
Nice, routing the guard through SetUUID means the assign-uuid update path (updates.go:177 -> SetUUID) is covered by the same check, so no second guard is needed. And the uuid -> newUUID rename is actually required here, since the old `uuid` param shadowed the package and `uuid.Nil` wasn't referenceable. Matches Java's assignUUID precondition too.
--
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