zeroshade commented on PR #1646:
URL: https://github.com/apache/iceberg-go/pull/1646#issuecomment-5219502081
Follow-up on the one non-blocking point from my re-review, now with a
verified snippet — approval above still stands, this is the only thing I'd like
changed before merge.
`TestMetadataUnmarshalDoesNotMutateInputAndRoundTripsAfterReuse` passes with
the production fix in `UnmarshalJSON` reverted, so it currently protects
nothing. The cause is the final assertion:
```go
assert.JSONEq(t, string(encoded), string(reencoded))
```
That compares `encoded` against a decode-then-re-encode of itself, which
tests encoder idempotence rather than whether the reused decode preserved every
field from `replacementData`.
I initially wanted to suggest simply comparing `replacementData` against
`encoded`, but I verified that and it does **not** work, for two reasons worth
recording so this isn't re-attempted:
1. A literal substitution doesn't compile — `reencoded` becomes unused
(`declared and not used: reencoded`), so the round-trip block has to go with it.
2. Even after removing it, the compare fails at `f258696` on exactly one
key: `NestedField.Doc` is `json:"doc,omitempty"`, so the fixture's `"doc": ""`
is omitted on encode. `version-log` was **not** a problem here — the fixture
has it present, so `init()`'s nil-to-empty normalization never triggers.
3. And once `doc` is normalized away, the compare passes *both* at the head
and with the fix reverted — still vacuous. Fixing the tautology alone isn't
enough to give the test teeth.
What makes it non-vacuous is dropping `properties` from the replacement
document: the fixed decoder clears the omitted state, the old one retains stale
`{"prop":"value"}`. That's the assertion doing real work.
Verified minimal form:
```go
var replacement map[string]any
require.NoError(t, json.Unmarshal([]byte(exampleViewJSON), &replacement))
replacement["current-version-id"] = int64(2)
replacement["versions"].([]any)[0].(map[string]any)["version-id"] = int64(2)
replacement["versions"].([]any)[0].(map[string]any)["schema-id"] = 2
replacement["schemas"].([]any)[0].(map[string]any)["schema-id"] = 2
delete(replacement["schemas"].([]any)[0].(map[string]any)["fields"].([]any)[0].(map[string]any),
"doc")
delete(replacement, "properties")
replacementData, err := json.Marshal(replacement)
require.NoError(t, err)
require.NoError(t, json.Unmarshal(replacementData, &md))
encoded, err := json.Marshal(&md)
require.NoError(t, err)
assert.JSONEq(t, string(replacementData), string(encoded))
```
The trailing `roundTripped` / `reencoded` block is then deleted. Keep the
existing input-mutation assertion at the top as-is.
Confirmed empirically:
- At `f258696`, targeted test: **passes**.
- With pre-PR `UnmarshalJSON` restored: **fails**, and on the right thing —
the only diff is a surviving `"properties": {"prop": "value"}` in the actual
output.
- `go test ./view/ -count=1` with the snippet in place: **green**.
Two notes on the `delete` for `doc`: it's papering over an encode/decode
asymmetry rather than testing anything, so if you'd rather avoid it, giving
that fixture field a non-empty `doc` would remove the need for the line. Either
is fine by me.
Not worth blocking a correct production fix over — if you'd prefer to land
this as-is and tighten the test in a follow-up, I'm happy with that too. The
`.([]any)` / `.(map[string]any)` assertions stay unchecked either way, which
remains ordinary test brittleness.
--
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]
