Apply last-key-wins semantics to duplicate map keys in DynamicMessage - #29513
Apply last-key-wins semantics to duplicate map keys in DynamicMessage#29513itzikch wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@googlebot I signed it! |
When a map field arrives on the wire with the same key more than once, a generated message keeps only the last value, but DynamicMessage kept every entry. The two therefore produced different field contents and different serializations for identical, well-formed wire input. Last-key-wins is required behavior, covered by the REQUIRED conformance test ValidDataMap*.DuplicateKey in conformance/binary_json_conformance_suite.cc. The C++ implementation already honors it for dynamic messages via DynamicMapField, which is backed by a real map container; the Java DynamicMessage had no equivalent and stored map entries in a plain List inside FieldSet. The conformance suite did not catch this because ConformanceJava.java parses exclusively through generated message parsers, so the DynamicMessage path is never exercised. Normalize map fields once in buildPartial() rather than on every addRepeatedField() call: de-duplicating at insertion time would make parsing an n-entry map O(n^2), and map entries arrive one at a time during a parse. The pass is skipped unless a map field holds at least two entries, and the field is only rewritten when a duplicate was actually found. Adds two tests to MapTest: one asserting a duplicate key collapses to the last value and matches the generated message byte-for-byte (fails without this change), and one asserting distinct keys are all preserved (guards against over-collapsing).
9d6940f to
1c1a307
Compare
|
Amended the commit to drop a @googlebot I signed it! |
|
Update on testing: I mentioned in the description that I could not run the Bazel Java suite locally (macOS toolchain wanted an Xcode version I do not have). I have since run it in a Linux container, so here are real results for With this change — 43/43 pass: With the tests kept but So the new test genuinely reproduces the bug rather than passing vacuously, and the remaining 42 tests in Environment: |
1 participant
Fixes #29512.
What
DynamicMessage.Builder.buildPartial()now normalizes map fields: entries are collapsed by key, keeping the last value seen, matching generated messages and the REQUIREDValidDataMap*.DuplicateKeyconformance test.Before this change, identical well-formed wire input produced different results from a generated message and a
DynamicMessage:32050a016b1002(7 B)DynamicMessage(before)32050a016b100132050a016b1002(14 B)DynamicMessage(after)32050a016b1002(7 B)The C++ implementation already handles this for dynamic messages via
DynamicMapField(backed by a real map container); the JavaDynamicMessagestored map entries in a plainListinsideFieldSetand appended unconditionally.Why in
buildPartial()rather thanaddRepeatedField()De-duplicating on every insertion would make parsing an n-entry map O(n²), since map entries arrive one at a time during a parse. Normalizing once during
buildPartial()is a single pass and keeps parsing linear.buildPartial()already contains map-entry-specific handling (defaultingMapEntryfields), so this sits with related logic.The pass is skipped entirely unless a map field holds at least two entries, and the field is only rewritten when a duplicate was actually found, so messages without duplicate keys are untouched. If a
Message.Builderis encountered in the list (possible while a caller is mid-mutation), the method returns without changing anything, preserving existing behavior.Testing
Two tests added to
MapTest.java:testDuplicateMapKey_dynamicMessageMatchesGeneratedMessage— a duplicate key collapses to the last value and theDynamicMessagematches the generated message byte-for-byte. Fails without this change.testDuplicateMapKey_dynamicMessagePreservesDistinctKeys— distinct keys are all preserved, guarding against over-collapsing. Passes before and after.The wire bytes are hand-built because a builder already applies map semantics; only the wire format can express a repeated key.
Additionally verified against released
protobuf-java4.36.0 with a standalone harness comparingDynamicMessageto a generated message across: empty message, single entry, two distinct keys, duplicate key x2 and x3, duplicates interleaved with distinct keys, 50 distinct keys, and 50 keys each duplicated. 4 of 8 diverged before the change; 0 after. Parse time stays linear on a worst-case all-duplicates message (10k to 80k entries, per-doubling ratios 0.83 / 1.38 / 1.12).I was unable to run the full Bazel Java suite locally (the macOS toolchain here requires an Xcode version I don't have), so CI coverage on this PR is appreciated.