GH-84: Collect an unknown key under the spelling the payload used - #137
Merged
Conversation
UnknownPropertyCollector promises to preserve unmapped payload keys, but stored the CONVERTED name: with the camelCase converter active, an unknown `favourite_colour` was preserved as `favouriteColour`. The value was kept raw, the key was not - so the collected map was not the faithful copy of the unmapped payload the feature exists to provide, and a consumer re-serialising it silently emitted a key the sender never sent. Applying a property-name converter to a key that by definition matches no property is what made this wrong. The two questions are now separated: whether a key is UNKNOWN is still decided on the converted name - `full_name` that camelises onto a declared `fullName` is mapped, not collected - while the key it is STORED under is the original payload spelling. Only the stored key changed; which keys count as unknown did not. This was never covered because every payload in the collector's test file used camelCase keys, so the conversion was a no-op in all of them while the suite runs with the converter enabled. Two tests now pin both halves: a snake_case unknown key survives verbatim, and a snake_case key that converts onto a declared property is still mapped rather than collected. Behavioural change for a consumer that relied on the converted key. The attribute docblock, AGENTS.md and docs/recipes/using-attributes.md described the old behaviour and now describe this one. Verified: composer ci:test green (507 tests, 1484 assertions), exit code 0.
UnknownPropertyCollector promises to preserve unmapped payload keys, but stored the CONVERTED name: with the camelCase converter active, an unknown `favourite_colour` was preserved as `favouriteColour`. The value was kept raw, the key was not - so the collected map was not the copy of the unmapped payload the feature exists to provide, and a consumer re-serialising it emitted a key the sender never sent. Applying a rewrite to a key that by definition matches no property is what made this wrong. The two questions are now separated: whether a key is UNKNOWN is still decided on the converted name - `full_name` that camelises onto a declared `fullName` is mapped, not collected - while the key it is STORED under is the original payload spelling. Both rewrite sources are bypassed, the name converter and a ReplaceProperty alias. This was never covered because every payload in the collector's test file used camelCase keys, so the conversion was a no-op in all of them while the suite runs with the converter enabled. Three tests now pin it, each red against the previous behaviour: a snake_case unknown key survives verbatim; a snake_case key that converts onto a declared property is still mapped while a second snake_case key beside it is collected; and two spellings that normalise to the same unknown name are kept as two entries. That last one is a second behavioural change the review surfaced: collected keys can no longer collide, so a payload that previously lost one of two colliding spellings now keeps both. The "two keys that normalize to the same name collide" note applied to the collector as well and was wrong for it - it is now scoped to declared properties in the attribute docblock, AGENTS.md and the recipe. A pre-existing gap the review also surfaced is filed as #136 rather than folded in: a payload key PHP canonicalises to an integer is dropped entirely, neither collected nor reported. The documentation carries an explicit caveat for it instead of claiming a round trip the code does not deliver. Behavioural change for a consumer that relied on the converted key. Attribute docblock, AGENTS.md and docs/recipes/using-attributes.md updated. Verified: composer ci:test green (508 tests, 1486 assertions), exit code 0.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Owner
Author
|
/gemini review |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Closes #84.
UnknownPropertyCollectorpromises to preserve unmapped payload keys, but stored the converted name: with the camelCase converter active an unknownfavourite_colourwas preserved asfavouriteColour. The value was kept raw, the key was not — so the collected map was not the copy of the unmapped payload the feature exists to provide, and a consumer re-serialising it emitted a key the sender never sent.Change
The two questions are separated:
full_namethat camelises onto a declaredfullNameis mapped, not collected.ReplacePropertyalias, whose target is by definition not a declared property here either.Second behavioural change the review surfaced
Collected keys can no longer collide. Previously
favourite_colourandfavouriteColourmerged into one entry (last one wins) and a value was lost; they are now two entries. The "two source keys that normalize to the same name collide" note applied to the collector too and was wrong for it — it is now scoped to declared properties in the attribute docblock,AGENTS.mdand the recipe.Why this was never caught
Every payload in the collector's test file used camelCase keys, so conversion was a no-op in all of them — while the suite runs with the converter enabled. Three tests now pin it, each red against the previous behaviour:
Filed, not folded in
A pre-existing gap the review surfaced is tracked as #136: a payload key PHP canonicalises to an integer (
"42") is dropped entirely — neither collected nor reported, even in strict mode. The documentation carries an explicit caveat for it rather than claiming a round trip the code does not deliver.Compatibility
Behavioural change for a consumer that relied on the converted key, or on two colliding spellings merging. Attribute docblock,
AGENTS.mdanddocs/recipes/using-attributes.mdupdated.Verification
composer ci:testgreen (508 tests, 1486 assertions), exit 0. Reviewed by the correctness, test-quality and Codex reviewers; all their findings (the collision prose, the stale merge comment and baseline docblock, the non-discriminating control test, the undocumentedReplacePropertybypass) are folded in.🤖 Generated with Claude Code