From 1c1a30705dcb4819b691627826d7dbdd0f7f99cf Mon Sep 17 00:00:00 2001 From: itzikch Date: Sat, 29 Aug 2026 21:08:27 +0300 Subject: [PATCH] Apply last-key-wins semantics to duplicate map keys in DynamicMessage 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). --- .../com/google/protobuf/DynamicMessage.java | 54 +++++++++++++++++++ .../java/com/google/protobuf/MapTest.java | 46 ++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/java/core/src/main/java/com/google/protobuf/DynamicMessage.java b/java/core/src/main/java/com/google/protobuf/DynamicMessage.java index f25aa23e7302f..f518284147ead 100644 --- a/java/core/src/main/java/com/google/protobuf/DynamicMessage.java +++ b/java/core/src/main/java/com/google/protobuf/DynamicMessage.java @@ -15,8 +15,10 @@ import com.google.protobuf.Descriptors.OneofDescriptor; import java.io.IOException; import java.io.InputStream; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -415,6 +417,8 @@ public DynamicMessage buildPartial() { } } + normalizeMapFields(); + DynamicMessage result = new DynamicMessage( type, @@ -424,6 +428,56 @@ public DynamicMessage buildPartial() { return result; } + + /** + * Collapses duplicate keys within map fields, keeping the last value seen. + * + *

The language guide requires that when a map is parsed from the wire and the same key + * appears more than once, the last value wins. Generated messages get this for free because + * they store map fields in a {@code MapField}, but {@code DynamicMessage} keeps them in a + * {@code FieldSet} as a plain list of {@code MapEntry} messages that is appended to + * unconditionally, so duplicate keys survived a parse. That made {@code DynamicMessage} and a + * generated message produce different results, and different serializations, for identical and + * well-formed wire input. + * + *

Normalizing once here rather than on every {@code addRepeatedField} keeps parsing linear; + * de-duplicating at insertion time would make parsing an n-entry map O(n^2). + */ + private void normalizeMapFields() { + int numFields = type.getFieldCount(); + for (int i = 0; i < numFields; i++) { + FieldDescriptor field = type.getField(i); + if (!field.isMapField()) { + continue; + } + // Map fields are repeated, so hasField() does not apply; getField() returns null when the + // field was never populated. + Object fieldValue = fields.getField(field); + if (!(fieldValue instanceof List)) { + continue; + } + List entries = (List) fieldValue; + if (entries.size() < 2) { + continue; + } + FieldDescriptor keyField = field.getMessageType().findFieldByNumber(1); + if (keyField == null) { + continue; + } + Map lastValueByKey = new LinkedHashMap<>(); + for (Object entry : entries) { + if (!(entry instanceof Message)) { + // A builder can be present while a caller is still mutating the field; leave it alone. + return; + } + lastValueByKey.put(((Message) entry).getField(keyField), entry); + } + if (lastValueByKey.size() != entries.size()) { + fields.setField(field, new ArrayList(lastValueByKey.values())); + } + } + } + @Override public Builder clone() { Builder result = new Builder(type); diff --git a/java/core/src/test/java/com/google/protobuf/MapTest.java b/java/core/src/test/java/com/google/protobuf/MapTest.java index 270d246ee1f02..d65554b49f213 100644 --- a/java/core/src/test/java/com/google/protobuf/MapTest.java +++ b/java/core/src/test/java/com/google/protobuf/MapTest.java @@ -1626,4 +1626,50 @@ public void getAllFields_mapEntryListMutability() { builder.clearField(int2MessageMapField); assertThat(mapEntries).hasSize(1); } + + @Test + public void testDuplicateMapKey_dynamicMessageMatchesGeneratedMessage() throws Exception { + // Hand-built wire bytes: string_to_int32_field (field 6) carrying the key "k" twice, with + // value 1 and then value 2. These cannot be produced through a builder, because a builder + // already applies map semantics; only the wire format can express a repeated key. + // 32 05 0a 01 6b 10 01 -> {"k": 1} + // 32 05 0a 01 6b 10 02 -> {"k": 2} + ByteString wire = + ByteString.copyFrom( + new byte[] { + 0x32, 0x05, 0x0a, 0x01, 0x6b, 0x10, 0x01, + 0x32, 0x05, 0x0a, 0x01, 0x6b, 0x10, 0x02 + }); + + TestMap generated = TestMap.parseFrom(wire); + DynamicMessage dynamic = DynamicMessage.parseFrom(TestMap.getDescriptor(), wire); + FieldDescriptor mapField = TestMap.getDescriptor().findFieldByName("string_to_int32_field"); + + // The language guide requires the last value for a repeated key to win. + assertThat(generated.getStringToInt32FieldMap()).containsExactly("k", 2); + assertThat(dynamic.getRepeatedFieldCount(mapField)).isEqualTo(1); + + // A DynamicMessage and a generated message must not disagree about identical wire input. + assertThat(dynamic.toByteString()).isEqualTo(generated.toByteString()); + } + + @Test + public void testDuplicateMapKey_dynamicMessagePreservesDistinctKeys() throws Exception { + // Same shape, but the two entries use different keys, so both must survive. + ByteString wire = + ByteString.copyFrom( + new byte[] { + 0x32, 0x05, 0x0a, 0x01, 0x61, 0x10, 0x01, + 0x32, 0x05, 0x0a, 0x01, 0x62, 0x10, 0x02 + }); + + TestMap generated = TestMap.parseFrom(wire); + DynamicMessage dynamic = DynamicMessage.parseFrom(TestMap.getDescriptor(), wire); + FieldDescriptor mapField = TestMap.getDescriptor().findFieldByName("string_to_int32_field"); + + assertThat(generated.getStringToInt32FieldMap()).containsExactly("a", 1, "b", 2); + assertThat(dynamic.getRepeatedFieldCount(mapField)).isEqualTo(2); + assertThat(dynamic.toByteString()).isEqualTo(generated.toByteString()); + } + }