fix(table): reject nil uuid assignment updates#1329
Conversation
tanmayrauth
left a comment
tanmayrauth
left a comment
There was a problem hiding this comment.
LGTM, clean fix and the guard sits in the right place. One optional question about reassignment semantics, inline.
| return nil | ||
| } | ||
|
|
||
| func (b *MetadataBuilder) SetUUID(uuid uuid.UUID) error { |
There was a problem hiding this 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.
| return errors.New("cannot set uuid to null") | ||
| } | ||
|
|
||
| if b.uuid == newUUID { |
There was a problem hiding this 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?
2 participants
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