reflect: preserve oneof members on decode errors#303
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
…f clear The branch predated the fix for dropping an existing singular message on a decode error, which reworked the same function this change rewrote. Resolve in favour of main's in-place merge: it keeps a partially merged value on error, which is what the owned decoder leaves behind, whereas decoding into a clone and discarding it on error restores the pre-merge value and reintroduces the divergence that fix removed. The clone also cost a deep copy on every repeated occurrence of a message field, on the success path. The oneof bug does not need that path at all. Reaching the merge branch means this member already holds the oneof, so no sibling is set and the clear has nothing to do. Only the fresh-decode path could destroy a live member, so the clear moves after the decode there and the rest stands as main has it. Both sets of tests are kept and both hold: the four oneof cases fail against main without the reorder, and the five message-merge cases still pass, so the resolution preserves the earlier fix rather than reverting it.
iainmcgin
left a comment
iainmcgin
left a comment
There was a problem hiding this comment.
[claude code] The bug here is real and your four tests are good — I measured each one against the merge-base and all four fail without the fix, which is exactly what you want from a regression test.
The complication is timing: #299 landed earlier today and reworked the same function this PR rewrites, so I've resolved that maintainer-side in 718b044 rather than hand you a conflict.
Why the resolution went main's way on the merge path. #299 wrapped the in-place merge in a closure so the taken value is always reinserted, deliberately keeping the partially merged value on error — that matches what the owned decoder leaves behind, and its changelog fragment says so explicitly. This PR's decode_merged_message clones the existing message, merges into the clone, and discards it on error, which restores the pre-merge value. That's the opposite semantic, and it would have re-opened the owned/reflective divergence #299 had just closed — silently, one commit later. A naive "take this PR's side" rebase would have reverted #299 with nothing failing to catch it. The clone also cost a deep copy on every repeated occurrence of a message field, on the success path, where #299 mutates in place with none.
The good news: the oneof bug never needed that path. Reaching the merge-into-existing branch means the field is already set, so for a oneof that member is already the active one and no sibling can be lost — the clear there is a no-op. Only the fresh-decode path could destroy a live member. So the fix reduces to moving the clear after decode_element on that path, leaving #299's function untouched. Same bug fixed, no conflict, no clone, no divergence.
Both test suites are kept and both hold, which is the check that matters here:
| vs main (no reorder) | |
|---|---|
| your 4 oneof tests | fail — they guard the reorder |
| #299's 5 merge tests | pass — the resolution preserves #299 |
Also added a Fixed changelog fragment: this is user-visible and belongs in the 0.9.0 notes. (check-changelog only verifies CHANGELOG.md is in sync with existing fragments, so its passing wasn't evidence one wasn't needed.)
Approving. Thanks for finding this — clearing oneof siblings before the replacement decoded was a genuine data-loss path, and the four error shapes you picked cover it well.
2 participants
What this does
DynamicMessage now decodes a singular oneof replacement into a temporary value before clearing sibling members. If decoding fails, the previously valid oneof member remains set. Existing singular message values are cloned before wire merging as well, so malformed repeated message occurrences do not mutate the stored value.
Coverage
Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspacecargo check --workspace --all-features