laskoviymishka commented on code in PR #1669:
URL: https://github.com/apache/iceberg-go/pull/1669#discussion_r3739095433
##########
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:
This is the one spot I'd want a second look at before it lands. Making an
absent `snapshot-id` an error is spec-correct — the OpenAPI
`AssertRefSnapshotId` lists it in `required` — but it's stricter than the Java
and rust reference implementations, which both treat an absent key as
equivalent to `null` (`getLongOrNull`/`hasNonNull` on the Java side,
`Option<i64>` on rust).
It's also a behavior change for us specifically:
`{"type":"assert-ref-snapshot-id","ref":"branch"}` with no `snapshot-id` used
to parse to `AssertRefSnapshotID("branch", nil)`, and this PR removes that test
case and now rejects the input. Java and PyIceberg always send explicit `null`,
so I don't think this breaks live catalog traffic — but any caller that
hand-builds requirement docs and omits the key will start getting an error.
I'm fine landing it strict since that matches the spec, I'd just want us to
pick "strict" knowingly. wdyt — keep it as-is, or fall back to treating absent
as null like the other clients?
##########
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
Review Comment:
`Set` and `Value` are exported on an unexported type, so nothing outside the
package can reach them anyway — I'd lowercase them to `set`/`value` to match
the usual convention. Small thing.
##########
view/requirements.go:
##########
@@ -93,6 +73,43 @@ type baseRequirement struct {
Type string `json:"type"`
}
+type requirementWire struct {
Review Comment:
`requirementWire` and `requiredRequirementField` here are byte-for-byte
identical to the ones in `table` (which `view` already imports). Not worth
blocking over, but it's the kind of thing that quietly drifts out of sync —
worth a shared internal helper if we're touching this area again.
##########
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")) {
Review Comment:
`encoding/json` never hands `UnmarshalJSON` whitespace-padded input — the
token's already trimmed by the time it reaches here, so the `bytes.TrimSpace`
is dead work, and it's the only reason this file needs the `bytes` import.
`bytes.Equal(data, []byte("null"))` or just `string(data) == "null"` does the
same thing.
##########
view/requirements_test.go:
##########
@@ -95,3 +97,36 @@ func TestParseRequirementListReplacesExistingSlice(t
*testing.T) {
require.NoError(t, json.Unmarshal([]byte(`[]`), &requirements))
assert.Empty(t, requirements)
}
+
+func TestParseRequirementRejectsMissingUUID(t *testing.T) {
Review Comment:
This loop isn't wrapped in `t.Run`, so if the first input fails the
`require` call stops the whole test and the second case (`"uuid":null`) never
runs. `TestParseRequirementRejectsMissingOrNullType` right below it uses
`t.Run(data, ...)` — I'd match that here so both cases always execute.
##########
table/requirement_test.go:
##########
@@ -171,6 +176,89 @@ func TestParseRequirementListReplacesExistingSlice(t
*testing.T) {
assert.Empty(t, requirements)
}
+func TestParseRequirementRejectsMissingRequiredFields(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ expectedField string
+ }{
+ {name: "missing type", data: `{}`, expectedField: "type"},
+ {name: "null type", data: `{"type":null}`, expectedField:
"type"},
+ {name: "table uuid", data: `{"type":"assert-table-uuid"}`,
expectedField: "uuid"},
+ {name: "null table uuid", data:
`{"type":"assert-table-uuid","uuid":null}`, expectedField: "uuid"},
+ {name: "missing ref", data:
`{"type":"assert-ref-snapshot-id"}`, expectedField: "ref"},
+ {name: "null ref", data:
`{"type":"assert-ref-snapshot-id","ref":null}`, expectedField: "ref"},
+ {name: "missing snapshot id", data:
`{"type":"assert-ref-snapshot-id","ref":"main"}`, expectedField: "snapshot-id"},
+ {name: "default spec id", data:
`{"type":"assert-default-spec-id"}`, expectedField: "default-spec-id"},
+ {name: "null default spec id", data:
`{"type":"assert-default-spec-id","default-spec-id":null}`, expectedField:
"default-spec-id"},
+ {name: "current schema id", data:
`{"type":"assert-current-schema-id"}`, expectedField: "current-schema-id"},
+ {name: "null current schema id", data:
`{"type":"assert-current-schema-id","current-schema-id":null}`, expectedField:
"current-schema-id"},
+ {name: "default sort order id", data:
`{"type":"assert-default-sort-order-id"}`, expectedField:
"default-sort-order-id"},
+ {name: "null default sort order id", data:
`{"type":"assert-default-sort-order-id","default-sort-order-id":null}`,
expectedField: "default-sort-order-id"},
+ {name: "last assigned field id", data:
`{"type":"assert-last-assigned-field-id"}`, expectedField:
"last-assigned-field-id"},
+ {name: "null last assigned field id", data:
`{"type":"assert-last-assigned-field-id","last-assigned-field-id":null}`,
expectedField: "last-assigned-field-id"},
+ {name: "last assigned partition id", data:
`{"type":"assert-last-assigned-partition-id"}`, expectedField:
"last-assigned-partition-id"},
+ {name: "null last assigned partition id", data:
`{"type":"assert-last-assigned-partition-id","last-assigned-partition-id":null}`,
expectedField: "last-assigned-partition-id"},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ expectedError := fmt.Sprintf("missing required field
%q", tt.expectedField)
+
Review Comment:
Seconding the substring point from the earlier round —
`requiredRequirementField` formats the name with `%q`, so the error reads
`missing required field "type"`. Asserting the bare `tt.expectedField` (`type`,
`uuid`, ...) passes on any error text that happens to contain those letters.
Matching the quoted form (assert on `"`+tt.expectedField+`"`) pins it to the
actual field name.
##########
table/requirement_test.go:
##########
@@ -171,6 +176,89 @@ func TestParseRequirementListReplacesExistingSlice(t
*testing.T) {
assert.Empty(t, requirements)
}
+func TestParseRequirementRejectsMissingRequiredFields(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ expectedField string
+ }{
+ {name: "missing type", data: `{}`, expectedField: "type"},
+ {name: "null type", data: `{"type":null}`, expectedField:
"type"},
+ {name: "table uuid", data: `{"type":"assert-table-uuid"}`,
expectedField: "uuid"},
+ {name: "null table uuid", data:
`{"type":"assert-table-uuid","uuid":null}`, expectedField: "uuid"},
+ {name: "missing ref", data:
`{"type":"assert-ref-snapshot-id"}`, expectedField: "ref"},
+ {name: "null ref", data:
`{"type":"assert-ref-snapshot-id","ref":null}`, expectedField: "ref"},
+ {name: "missing snapshot id", data:
`{"type":"assert-ref-snapshot-id","ref":"main"}`, expectedField: "snapshot-id"},
+ {name: "default spec id", data:
`{"type":"assert-default-spec-id"}`, expectedField: "default-spec-id"},
+ {name: "null default spec id", data:
`{"type":"assert-default-spec-id","default-spec-id":null}`, expectedField:
"default-spec-id"},
+ {name: "current schema id", data:
`{"type":"assert-current-schema-id"}`, expectedField: "current-schema-id"},
+ {name: "null current schema id", data:
`{"type":"assert-current-schema-id","current-schema-id":null}`, expectedField:
"current-schema-id"},
+ {name: "default sort order id", data:
`{"type":"assert-default-sort-order-id"}`, expectedField:
"default-sort-order-id"},
+ {name: "null default sort order id", data:
`{"type":"assert-default-sort-order-id","default-sort-order-id":null}`,
expectedField: "default-sort-order-id"},
+ {name: "last assigned field id", data:
`{"type":"assert-last-assigned-field-id"}`, expectedField:
"last-assigned-field-id"},
+ {name: "null last assigned field id", data:
`{"type":"assert-last-assigned-field-id","last-assigned-field-id":null}`,
expectedField: "last-assigned-field-id"},
+ {name: "last assigned partition id", data:
`{"type":"assert-last-assigned-partition-id"}`, expectedField:
"last-assigned-partition-id"},
+ {name: "null last assigned partition id", data:
`{"type":"assert-last-assigned-partition-id","last-assigned-partition-id":null}`,
expectedField: "last-assigned-partition-id"},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ expectedError := fmt.Sprintf("missing required field
%q", tt.expectedField)
+
+ _, err := table.ParseRequirementBytes([]byte(tt.data))
+ require.ErrorIs(t, err, table.ErrInvalidRequirement)
+ require.ErrorContains(t, err, expectedError)
+
+ var requirements table.Requirements
+ err = json.Unmarshal([]byte("["+tt.data+"]"),
&requirements)
+ require.ErrorIs(t, err, table.ErrInvalidRequirement)
+ require.ErrorContains(t, err, expectedError)
+ })
+ }
+}
+
+func TestParseRequirementAcceptsExplicitZero(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ expected table.Requirement
+ }{
+ {name: "default spec id", data:
`{"type":"assert-default-spec-id","default-spec-id":0}`, expected:
table.AssertDefaultSpecID(0)},
+ {name: "current schema id", data:
`{"type":"assert-current-schema-id","current-schema-id":0}`, expected:
table.AssertCurrentSchemaID(0)},
+ {name: "default sort order id", data:
`{"type":"assert-default-sort-order-id","default-sort-order-id":0}`, expected:
table.AssertDefaultSortOrderID(0)},
+ {name: "last assigned field id", data:
`{"type":"assert-last-assigned-field-id","last-assigned-field-id":0}`,
expected: table.AssertLastAssignedFieldID(0)},
+ {name: "last assigned partition id", data:
`{"type":"assert-last-assigned-partition-id","last-assigned-partition-id":0}`,
expected: table.AssertLastAssignedPartitionID(0)},
+ {name: "snapshot id", data:
`{"type":"assert-ref-snapshot-id","ref":"main","snapshot-id":0}`, expected:
table.AssertRefSnapshotID("main", ptr(int64(0)))},
+ }
+
Review Comment:
Every other new test here exercises both `ParseRequirementBytes` and
`Requirements.UnmarshalJSON`, but this one only hits `ParseRequirementBytes`.
They converge on the same path so it's not a correctness gap, just an asymmetry
— a parallel `json.Unmarshal([]byte("["+tt.data+"]"), &requirements)` assertion
would keep it consistent with the rest.
--
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]
