zeroshade commented on code in PR #1662:
URL: https://github.com/apache/iceberg-go/pull/1662#discussion_r3732117422
##########
partitions_test.go:
##########
@@ -317,6 +317,27 @@ func TestSerializePartitionSpec(t *testing.T) {
assert.True(t, spec.Equals(outspec))
}
+func TestDeserializePartitionSpecRequiresTopLevelFields(t *testing.T) {
+ for _, tt := range []struct {
+ name string
+ data string
+ }{
+ {name: "missing spec id", data: `{"fields": []}`},
Review Comment:
This case encodes the behavior that needs to change. `{"fields": []}` is a
**valid** REST `PartitionSpec` — the OpenAPI schema requires `fields` and
nothing else — so asserting that it returns `ErrInvalidPartitionSpec` locks in
the request-path regression described at `partitions.go:464`.
Suggested fix: move this case out of the error table and into a positive
test asserting that it decodes successfully with `ID() ==
InitialPartitionSpecID` and zero fields. The other four rows here are all
correct and should stay: `null` `spec-id` is genuinely invalid even where the
field is optional, and the three `fields` cases match the schema. Their
assertions on receiver preservation are a nice touch and worth keeping in the
positive test too — starting from `NewPartitionSpecID(7)` and confirming the
decoded ID is `0`, not `7`, is exactly the check that catches the
receiver-inheritance trap.
##########
partitions.go:
##########
@@ -454,20 +455,34 @@ func (ps PartitionSpec) MarshalJSON() ([]byte, error) {
}
func (ps *PartitionSpec) UnmarshalJSON(b []byte) error {
- aux := struct {
- ID int `json:"spec-id"`
- Fields []json.RawMessage `json:"fields"`
- }{ID: ps.id}
-
- if err := json.Unmarshal(b, &aux); err != nil {
+ var raw map[string]json.RawMessage
+ if err := json.Unmarshal(b, &raw); err != nil {
return err
Review Comment:
Non-blocking, for consistency. Every other failure path added in this PR
wraps its error with `ErrInvalidPartitionSpec`, so callers can use `errors.Is`
uniformly. This one returns the bare `json.Unmarshal` error, which means a
malformed top-level value — a string or an array where an object was expected —
is the one shape error that does not match the sentinel.
Suggested fix: wrap it the same way as the others, e.g. `fmt.Errorf("%w:
%w", ErrInvalidPartitionSpec, err)`. The same applies to the two bare `return
err` paths inside the field loop, at `:488` and `:500`.
##########
partitions.go:
##########
@@ -454,20 +455,34 @@ func (ps PartitionSpec) MarshalJSON() ([]byte, error) {
}
func (ps *PartitionSpec) UnmarshalJSON(b []byte) error {
- aux := struct {
- ID int `json:"spec-id"`
- Fields []json.RawMessage `json:"fields"`
- }{ID: ps.id}
-
- if err := json.Unmarshal(b, &aux); err != nil {
+ var raw map[string]json.RawMessage
+ if err := json.Unmarshal(b, &raw); err != nil {
return err
}
- if aux.ID < 0 {
- return fmt.Errorf("%w: spec ID must be non-negative: %d",
ErrInvalidPartitionSpec, aux.ID)
+
+ rawID, ok := raw["spec-id"]
+ if !ok || bytes.Equal(bytes.TrimSpace(rawID), []byte("null")) {
Review Comment:
This is the blocking issue. Requiring `spec-id` is correct for stored table
metadata, but this decoder is not only used for stored table metadata.
The REST OpenAPI `PartitionSpec` schema requires only `fields`; `spec-id` is
optional and marked read-only, deliberately, so that a client can send a spec
and let the server assign the ID. `PartitionSpec.UnmarshalJSON` is the single
entry point for both shapes, so applying the stored-metadata requirement here
applies it to requests too.
The concrete regression is at `table/updates.go:242` —
`addPartitionSpecUpdate.Spec` is an `*iceberg.PartitionSpec`, so it decodes
through this function. A spec-valid update that worked before this change:
```json
{"action": "add-spec", "spec": {"fields": []}}
```
now fails with `partition spec is missing required spec-id`. Create-table
requests whose spec omits the ID are exposed identically. A shared unmarshaller
cannot impose the stored-metadata requirement globally without breaking the
request path.
Suggested fix, in two parts:
1. **Here:** keep `fields` required — that part is right and matches both
schemas — but allow `spec-id` to be absent, defaulting to
`InitialPartitionSpecID`. Note that the default must be the constant, *not* the
receiver's current `ps.id`: the old code seeded `aux` from `ps.id`, and
reintroducing that would make decoding depend on whatever the receiver happened
to hold, which is the inheritance problem the `decoded` local at `:507`
otherwise fixes. Explicit `null` should still be rejected, as should a negative
or non-integer value.
2. **In the metadata decoder:** enforce the requirement where it actually
belongs, in the code that parses full table metadata and therefore knows both
the format version and the context. That is the only place with enough
information to distinguish "stored metadata missing a required field" from "a
request legitimately omitting a read-only one."
--
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]
