zeroshade commented on code in PR #1669:
URL: https://github.com/apache/iceberg-go/pull/1669#discussion_r3737477803
##########
table/requirement_test.go:
##########
@@ -93,7 +93,11 @@ func TestParseRequirementBytes(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
actual, err := table.ParseRequirementBytes(tc.data)
assert.Equal(t, tc.expected, actual)
- assert.Equal(t, tc.expectedErr, err)
+ if tc.expectedErr != nil {
+ require.ErrorIs(t, err, tc.expectedErr)
Review Comment:
Non-blocking: this loosening was not required by the PR, and it gives up a
real bit of coverage.
Only one case in this table has a non-nil `expectedErr` — `"invalid
requirement"` at line 89 — and its value is the **bare unwrapped sentinel**,
since `ParseRequirementBytes` returns `ErrInvalidRequirement` verbatim
(`table/requirements.go:476`). So no case here was ever pinning a wrapped or
formatted message that `ErrorIs` would now miss. I confirmed the change was
unnecessary: restoring `assert.Equal(t, tc.expectedErr, err)` against the
production code at this commit passes 9/9.
What now slips through: mutate `ParseRequirementBytes`'s unknown-type
handler to `requiredRequirementField("type")`, so an unknown requirement type
is misreported as a *missing-`type`-field* error. The new assertion passes 9/9;
the old one catches it:
```
expected: *errors.errorString{s:"invalid requirement"}
actual : *fmt.wrapError{msg:"invalid requirement: missing required field
\"type\"", ...}
```
Note this compounds with the `ErrorContains` note above — that exact
misclassification is also what would let the `"type"` cases pass for the wrong
reason, and after this change nothing in the suite pins that the unknown-type
error is bare.
The `else { require.NoError(...) }` branch is a genuine improvement over
`assert.Equal` on nil, so keep that. Suggest either keeping `assert.Equal` for
the error case, or adding an `expectedErrMsg string` column and asserting both
the sentinel and the exact message.
##########
view/requirements_test.go:
##########
@@ -95,3 +96,34 @@ func TestParseRequirementListReplacesExistingSlice(t
*testing.T) {
require.NoError(t, json.Unmarshal([]byte(`[]`), &requirements))
assert.Empty(t, requirements)
}
+
+func TestParseRequirementRejectsMissingUUID(t *testing.T) {
+ for _, data := range []string{
+ `{"type":"assert-view-uuid"}`,
+ `{"type":"assert-view-uuid","uuid":null}`,
+ } {
+ _, err := view.ParseRequirementBytes([]byte(data))
+ require.ErrorIs(t, err, table.ErrInvalidRequirement)
+ require.ErrorContains(t, err, "uuid")
+
+ var requirements view.Requirements
+ err = json.Unmarshal([]byte("["+data+"]"), &requirements)
+ require.ErrorIs(t, err, table.ErrInvalidRequirement)
+ require.ErrorContains(t, err, "uuid")
+ }
+}
+
+func TestParseRequirementRejectsMissingOrNullType(t *testing.T) {
+ for _, data := range []string{`{}`, `{"type":null}`} {
+ t.Run(data, func(t *testing.T) {
+ _, err := view.ParseRequirementBytes([]byte(data))
+ require.ErrorIs(t, err, table.ErrInvalidRequirement)
+ require.ErrorContains(t, err, "type")
Review Comment:
Non-blocking, same as the table-side note: `"type"` as a bare substring is
incidentally satisfiable — a wrapped `json.UnmarshalTypeError` or any message
mentioning "unknown requirement type" satisfies it without the missing-field
rejection this test is meant to pin. `require.ErrorContains(t, err,
fmt.Sprintf("missing required field %q", "type"))` closes it. Applies to line
126 too, and the `"uuid"` assertions at lines 107/112 would benefit from the
same full-message form (note `json: cannot unmarshal number into Go value of
type *uuid.UUID` contains both "type" and "uuid").
The new coverage itself is correct and was a real gap — both cases fail on a
production revert, and the common discriminator check now gets exercised on
this decoder rather than only the table one. Confirmed the `errors.Is` chain
works here: `view.requiredRequirementField` wraps
`table.ErrInvalidRequirement`, which was already the view parser's public
sentinel before this PR, so importing `table` for it in the test is correct
rather than an odd coupling — there is no separate view sentinel to prefer.
##########
table/requirement_test.go:
##########
@@ -171,6 +175,87 @@ 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) {
+ _, err := table.ParseRequirementBytes([]byte(tt.data))
+ require.ErrorIs(t, err, table.ErrInvalidRequirement)
+ require.ErrorContains(t, err, tt.expectedField)
+
+ var requirements table.Requirements
+ err = json.Unmarshal([]byte("["+tt.data+"]"),
&requirements)
+ require.ErrorIs(t, err, table.ErrInvalidRequirement)
+ require.ErrorContains(t, err, tt.expectedField)
+ })
+ }
+}
+
+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:
This is the case I most wanted from the last round, and it is the right one
to have added — `snapshot-id: 0` is the only value that must be distinguished
from both an omitted member *and* an explicit `null`, and it is now pinned
alongside the `null` case at line 241 and the rejection case at line 190. Those
three together fully cover the nullable-not-optional semantics.
Worth knowing (not a defect): none of the six subtests here fail if the
production fix is reverted, because the old decoder accepted explicit zero too.
Their value is in the other direction — they fail against a naive `!Set ||
Value == nil` presence check, which is the most likely way someone would
reimplement this and accidentally reject legitimate zeros. Confirmed by
mutation. Good regression guards to keep.
`ptr` is the pre-existing generic helper in this package
(`table/snapshots_test.go`), not a new duplicate — no action needed.
##########
table/requirement_test.go:
##########
@@ -171,6 +175,87 @@ 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) {
+ _, err := table.ParseRequirementBytes([]byte(tt.data))
+ require.ErrorIs(t, err, table.ErrInvalidRequirement)
+ require.ErrorContains(t, err, tt.expectedField)
Review Comment:
Non-blocking, and the fix is one line — but this assertion is weaker than it
looks for the two `type` cases at lines 184-185.
For the 15 field cases it is solidly discriminating. I built a
cross-satisfaction matrix over all 17 actual error strings and found **zero**
instances of one case's error satisfying another case's `expectedField`, and
mutation-tested it: changing `current schema id`'s expectation to
`"default-spec-id"`, or `last assigned field id`'s to
`"last-assigned-partition-id"`, both correctly fail. So field identity is
genuinely pinned.
The `"type"` token is the problem — it is short and appears in unrelated
error text. Three experiments, production otherwise at this commit:
1. A wrong-reason rejection that happens to mention type — `fmt.Errorf("%w:
unknown requirement type", ErrInvalidRequirement)` → both `type` cases **pass**.
2. A leaked `json.UnmarshalTypeError` wrapped in the sentinel → both
**pass**. That error's text always contains "type" (`json: cannot unmarshal
object into Go value of type string`).
3. The realistic one: full pre-PR production revert **plus** one plausible
independent improvement (sentinel-wrapping the unknown-type errors) → `missing
type` and `null type` both **pass** on a build containing none of this PR's
fixes, while the other 15 correctly fail.
Asserting the whole message closes all three at once and also removes a
latent substring hazard (`"spec-id"` is a substring of `"default-spec-id"`, so
a future author writing the short form would get a silently loose test —
confirmed it still passes today):
```go
require.ErrorContains(t, err, fmt.Sprintf("missing required field %q",
tt.expectedField))
```
Same change applies to line 212 for the list path.
--
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]
