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: [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]
