zeroshade commented on code in PR #1669:
URL: https://github.com/apache/iceberg-go/pull/1669#discussion_r3732168332
##########
table/requirement_test.go:
##########
@@ -171,6 +171,52 @@ func TestParseRequirementListReplacesExistingSlice(t
*testing.T) {
assert.Empty(t, requirements)
}
+func TestParseRequirementRejectsMissingRequiredFields(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ }{
+ {name: "table uuid", data: `{"type":"assert-table-uuid"}`},
+ {name: "missing ref", data:
`{"type":"assert-ref-snapshot-id"}`},
+ {name: "missing snapshot id", data:
`{"type":"assert-ref-snapshot-id","ref":"main"}`},
+ {name: "default spec id", data:
`{"type":"assert-default-spec-id"}`},
+ {name: "current schema id", data:
`{"type":"assert-current-schema-id"}`},
+ {name: "default sort order id", data:
`{"type":"assert-default-sort-order-id"}`},
+ {name: "last assigned field id", data:
`{"type":"assert-last-assigned-field-id"}`},
+ {name: "last assigned partition id", data:
`{"type":"assert-last-assigned-partition-id"}`},
+ {name: "null current schema id", data:
`{"type":"assert-current-schema-id","current-schema-id":null}`},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ _, err := table.ParseRequirementBytes([]byte(tt.data))
+ require.Error(t, err)
+
+ var requirements table.Requirements
+ err = json.Unmarshal([]byte("["+tt.data+"]"),
&requirements)
+ require.Error(t, err)
+ })
+ }
+}
+
+func TestParseRequirementAcceptsExplicitZero(t *testing.T) {
+ actual, err :=
table.ParseRequirementBytes([]byte(`{"type":"assert-current-schema-id","current-schema-id":0}`))
+ require.NoError(t, err)
+ assert.Equal(t, table.AssertCurrentSchemaID(0), actual)
+}
+
+func TestParseRequirementRefRequiresRefButAllowsNullSnapshotID(t *testing.T) {
Review Comment:
Non-blocking: this exercises the valid `snapshot-id: null` case through
`ParseRequirementBytes` only.
`TestParseRequirementRejectsMissingRequiredFields` checks both entry points;
consider doing the same here, so `Requirements.UnmarshalJSON` is confirmed to
*accept* the valid-null document rather than only to reject the invalid ones.
##########
table/requirements.go:
##########
@@ -105,6 +82,151 @@ type baseRequirement struct {
Type string `json:"type"`
}
+type requirementWire struct {
+ Type *string `json:"type"`
+}
+
+type assertTableUUIDWire struct {
+ UUID *uuid.UUID `json:"uuid"`
+}
+
+type nullableInt64 struct {
+ Set bool
+ Value *int64
+}
+
+func (n *nullableInt64) UnmarshalJSON(data []byte) error {
+ n.Set = true
+ n.Value = nil
+ if bytes.Equal(bytes.TrimSpace(data), []byte("null")) {
+ return nil
+ }
+
+ var value int64
+ if err := json.Unmarshal(data, &value); err != nil {
+ return err
+ }
+ n.Value = &value
+
+ return nil
+}
+
+type assertRefSnapshotIDWire struct {
+ Ref *string `json:"ref"`
+ SnapshotID nullableInt64 `json:"snapshot-id"`
+}
+
+func requiredRequirementField(name string) error {
+ return fmt.Errorf("%w: missing required field %q",
ErrInvalidRequirement, name)
+}
+
+func parseRequirementBytes(b []byte, unknown func(string) error) (Requirement,
error) {
+ var base requirementWire
+ if err := json.Unmarshal(b, &base); err != nil {
+ return nil, err
+ }
+ if base.Type == nil {
+ return nil, requiredRequirementField("type")
+ }
+
+ switch *base.Type {
+ case reqAssertCreate:
+ return AssertCreate(), nil
+
+ case reqAssertTableUUID:
+ var req assertTableUUIDWire
+ if err := json.Unmarshal(b, &req); err != nil {
+ return nil, err
+ }
+ if req.UUID == nil {
+ return nil, requiredRequirementField("uuid")
+ }
+
+ return AssertTableUUID(*req.UUID), nil
+
+ case reqAssertRefSnapshotID:
+ var req assertRefSnapshotIDWire
+ if err := json.Unmarshal(b, &req); err != nil {
+ return nil, err
+ }
+ if req.Ref == nil {
+ return nil, requiredRequirementField("ref")
+ }
+ if !req.SnapshotID.Set {
Review Comment:
Worth calling out as the thing this PR gets right. `snapshot-id` is
nullable-not-optional in the spec — a required member with `nullable: true`,
where an explicit `null` asserts that the ref must **not** exist. Treating it
as merely optional (the natural `*int64` reading) would silently accept a
document that omits it and quietly turn a "ref must not exist" assertion into a
no-op.
The `nullableInt64` presence flag (`table/requirements.go:93-116`) plus this
rejection is the correct implementation, and
`TestParseRequirementRefRequiresRefButAllowsNullSnapshotID` pins both halves of
it.
##########
table/requirement_test.go:
##########
@@ -171,6 +171,52 @@ func TestParseRequirementListReplacesExistingSlice(t
*testing.T) {
assert.Empty(t, requirements)
}
+func TestParseRequirementRejectsMissingRequiredFields(t *testing.T) {
Review Comment:
Non-blocking: consider asserting `errors.Is(err,
table.ErrInvalidRequirement)` rather than bare `require.Error`, and that the
message names the missing field. As written, an unrelated decode failure — a
malformed body, a renamed type constant — would satisfy these subtests just as
well as the rejection they are meant to pin.
Two cases are also worth adding to the table: a missing `type`, and
`{"type":null}`. The common discriminator is checked at
`table/requirements.go:128-130` but nothing exercises it.
`{"type":"assert-table-uuid","uuid":null}` and
`{"type":"assert-ref-snapshot-id","ref":null}` would round out the
explicit-null coverage the same way the `null current-schema-id` case does.
##########
table/requirement_test.go:
##########
@@ -171,6 +171,52 @@ func TestParseRequirementListReplacesExistingSlice(t
*testing.T) {
assert.Empty(t, requirements)
}
+func TestParseRequirementRejectsMissingRequiredFields(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ }{
+ {name: "table uuid", data: `{"type":"assert-table-uuid"}`},
+ {name: "missing ref", data:
`{"type":"assert-ref-snapshot-id"}`},
+ {name: "missing snapshot id", data:
`{"type":"assert-ref-snapshot-id","ref":"main"}`},
+ {name: "default spec id", data:
`{"type":"assert-default-spec-id"}`},
+ {name: "current schema id", data:
`{"type":"assert-current-schema-id"}`},
+ {name: "default sort order id", data:
`{"type":"assert-default-sort-order-id"}`},
+ {name: "last assigned field id", data:
`{"type":"assert-last-assigned-field-id"}`},
+ {name: "last assigned partition id", data:
`{"type":"assert-last-assigned-partition-id"}`},
+ {name: "null current schema id", data:
`{"type":"assert-current-schema-id","current-schema-id":null}`},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ _, err := table.ParseRequirementBytes([]byte(tt.data))
+ require.Error(t, err)
+
+ var requirements table.Requirements
+ err = json.Unmarshal([]byte("["+tt.data+"]"),
&requirements)
+ require.Error(t, err)
+ })
+ }
+}
+
+func TestParseRequirementAcceptsExplicitZero(t *testing.T) {
Review Comment:
Non-blocking: this covers `current-schema-id: 0` only, but the explicit-zero
distinction applies to every integer requirement — `default-spec-id`,
`default-sort-order-id`, `last-assigned-field-id`,
`last-assigned-partition-id`, and `snapshot-id: 0`.
Consider turning it into a table so each scalar decoder is exercised.
`snapshot-id: 0` is the most valuable addition, since it is the one value that
also has to be distinguished from `null`.
##########
view/requirements_test.go:
##########
@@ -95,3 +95,16 @@ func TestParseRequirementListReplacesExistingSlice(t
*testing.T) {
require.NoError(t, json.Unmarshal([]byte(`[]`), &requirements))
assert.Empty(t, requirements)
}
+
+func TestParseRequirementRejectsMissingUUID(t *testing.T) {
Review Comment:
Non-blocking, mirroring the table-side note: consider asserting the sentinel
(`errors.Is(err, table.ErrInvalidRequirement)`, which
`view.requiredRequirementField` wraps at `view/requirements.go:84-86`) rather
than bare `require.Error`, and adding a missing-`type` and `{"type":null}` case
so the common discriminator check at `view/requirements.go:93-94` is exercised
on this decoder 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: [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]
