Skip to content

Commit 799556e

Browse files
protobuf-github-botcopybara-github
protobuf-github-bot
authored and
copybara-github
committed
Align DynamicMessage.hashCode() with GeneratedMessage.hashCode()
someDynamicMessage.equals(someGeneratedMessage) can return true if the DynamicMessage was created with SomeGeneratedMessage.getDesciptor() verbatim (by reference identity _not_ a second copy of the same descriptor parsed) However, the hashcodes don't necessarily always match. This breaks the hashcode/equals contract in Java and can result in broken collections if you e.g. insert both kinds of messages as keys in the same hashset. Changing gencode would cause skew oddities, so this changes the DynamicMessage hashcode to be aligned with the preexisting gencode hashcode structure. This has a small performance hit on DynamicMessage hashcode, but it is necessary for correctness. Fixes #19080 PiperOrigin-RevId: 992456152
1 parent bf5a8e7 commit 799556e

3 files changed

Lines changed: 264 additions & 16 deletions

File tree

‎java/core/src/main/java/com/google/protobuf/AbstractMessage.java‎

Lines changed: 80 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -155,15 +155,75 @@ public boolean equals(final Object other) {
155155
public int hashCode() {
156156
int hash = memoizedHashCode;
157157
if (hash == 0) {
158+
Descriptors.Descriptor descriptor = getDescriptorForType();
158159
hash = 41;
159-
hash = (19 * hash) + getDescriptorForType().hashCode();
160-
hash = hashFields(hash, getAllFields());
160+
hash = (19 * hash) + descriptor.hashCode();
161+
hash = hashFieldsInGeneratedOrder(hash, descriptor, getAllFields());
161162
hash = (29 * hash) + getUnknownFields().hashCode();
162163
memoizedHashCode = hash;
163164
}
164165
return hash;
165166
}
166167

168+
/**
169+
* Hashes {@code allFields} in the same order and with the same default handling as the generated
170+
* {@code hashCode()} of {@code descriptor}, so that a reflection-based message hashes the same as
171+
* the equivalent generated message.
172+
*
173+
* <p>Only depends on {@code allFields} and {@code descriptor} (not on oneof reflection methods),
174+
* so it remains consistent with {@link #equals} for any subclass.
175+
*/
176+
private static int hashFieldsInGeneratedOrder(
177+
int hash, Descriptors.Descriptor descriptor, Map<FieldDescriptor, Object> allFields) {
178+
// Non-oneof fields in declaration order. Singular implicit-presence fields are always hashed
179+
// (using the default value when unset), matching gencode.
180+
int fieldCount = descriptor.getFieldCount();
181+
for (int i = 0; i < fieldCount; i++) {
182+
FieldDescriptor field = descriptor.getField(i);
183+
if (field.getRealContainingOneof() != null) {
184+
continue;
185+
}
186+
Object value = allFields.get(field);
187+
if (field.hasPresence()) {
188+
if (value != null) {
189+
hash = hashField(hash, field, value);
190+
}
191+
} else if (field.isRepeated()) {
192+
if (value != null && !((List<?>) value).isEmpty()) {
193+
hash = hashField(hash, field, value);
194+
}
195+
} else {
196+
hash = hashField(hash, field, value != null ? value : field.getDefaultValue());
197+
}
198+
}
199+
200+
// Real oneofs in oneof declaration order.
201+
int realOneofCount = descriptor.getRealOneofCount();
202+
for (int i = 0; i < realOneofCount; i++) {
203+
OneofDescriptor oneof = descriptor.getRealOneof(i);
204+
int oneofFieldCount = oneof.getFieldCount();
205+
for (int j = 0; j < oneofFieldCount; j++) {
206+
FieldDescriptor field = oneof.getField(j);
207+
Object value = allFields.get(field);
208+
if (value != null) {
209+
hash = hashField(hash, field, value);
210+
break;
211+
}
212+
}
213+
}
214+
215+
// Extensions, in the map's iteration order (field number order for all runtime maps).
216+
if (descriptor.isExtendable()) {
217+
for (Map.Entry<FieldDescriptor, Object> entry : allFields.entrySet()) {
218+
FieldDescriptor field = entry.getKey();
219+
if (field.isExtension()) {
220+
hash = hashField(hash, field, entry.getValue());
221+
}
222+
}
223+
}
224+
return hash;
225+
}
226+
167227
private static ByteString toByteString(Object value) {
168228
if (value instanceof byte[]) {
169229
return ByteString.copyFrom((byte[]) value);
@@ -275,22 +335,26 @@ private static int hashMapField(Object value) {
275335
}
276336

277337
/** Get a hash code for given fields and values, using the given seed. */
278-
@SuppressWarnings("unchecked")
279338
protected static int hashFields(int hash, Map<FieldDescriptor, Object> map) {
280339
for (Map.Entry<FieldDescriptor, Object> entry : map.entrySet()) {
281-
FieldDescriptor field = entry.getKey();
282-
Object value = entry.getValue();
283-
hash = (37 * hash) + field.getNumber();
284-
if (field.isMapField()) {
285-
hash = (53 * hash) + hashMapField(value);
286-
} else if (field.getType() != FieldDescriptor.Type.ENUM) {
287-
hash = (53 * hash) + value.hashCode();
288-
} else if (field.isRepeated()) {
289-
List<? extends EnumLite> list = (List<? extends EnumLite>) value;
290-
hash = (53 * hash) + Internal.hashEnumList(list);
291-
} else {
292-
hash = (53 * hash) + Internal.hashEnum((EnumLite) value);
293-
}
340+
hash = hashField(hash, entry.getKey(), entry.getValue());
341+
}
342+
return hash;
343+
}
344+
345+
/** Get a hash code for a given field and value, using the given seed. */
346+
@SuppressWarnings("unchecked")
347+
private static int hashField(int hash, FieldDescriptor field, Object value) {
348+
hash = (37 * hash) + field.getNumber();
349+
if (field.isMapField()) {
350+
hash = (53 * hash) + hashMapField(value);
351+
} else if (field.getType() != FieldDescriptor.Type.ENUM) {
352+
hash = (53 * hash) + value.hashCode();
353+
} else if (field.isRepeated()) {
354+
List<? extends EnumLite> list = (List<? extends EnumLite>) value;
355+
hash = (53 * hash) + Internal.hashEnumList(list);
356+
} else {
357+
hash = (53 * hash) + Internal.hashEnum((EnumLite) value);
294358
}
295359
return hash;
296360
}

‎java/core/src/test/java/com/google/protobuf/AbstractMessageTest.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,19 @@
1313
import static com.google.protobuf.TestUtil.TEST_REQUIRED_UNINITIALIZED;
1414

1515
import com.google.protobuf.Descriptors.FieldDescriptor;
16+
import com.google.protobuf.testing.proto.TestProto3Optional;
17+
import map_test.MapTestProto.TestMap;
1618
import proto2_unittest.UnittestOptimizeFor.TestOptimizedForSize;
1719
import proto2_unittest.UnittestProto;
1820
import proto2_unittest.UnittestProto.ForeignMessage;
1921
import proto2_unittest.UnittestProto.TestAllExtensions;
2022
import proto2_unittest.UnittestProto.TestAllTypes;
23+
import proto2_unittest.UnittestProto.TestOneof2;
2124
import proto2_unittest.UnittestProto.TestPackedTypes;
2225
import proto2_unittest.UnittestProto.TestRequired;
2326
import proto2_unittest.UnittestProto.TestRequiredForeign;
2427
import proto2_unittest.UnittestProto.TestUnpackedTypes;
28+
import proto3_unittest.UnittestProto3;
2529
import java.util.Map;
2630
import org.junit.Test;
2731
import org.junit.runner.RunWith;
@@ -539,6 +543,39 @@ private void checkEqualsIsConsistent(Message message1, Message message2) {
539543
assertThat(message2.hashCode()).isEqualTo(message1.hashCode());
540544
}
541545

546+
@Test
547+
public void testHashCodeMatchesGeneratedForSubclassWithoutOneofReflection() {
548+
// AbstractMessageWrapper does not override hasOneof/getOneofFieldDescriptor, so this checks
549+
// that AbstractMessage.hashCode() only relies on getAllFields().
550+
UnittestProto3.TestAllTypes proto3Message =
551+
UnittestProto3.TestAllTypes.newBuilder()
552+
.setOptionalInt32(5)
553+
.setOneofString("oneof")
554+
.addRepeatedString("r")
555+
.build();
556+
assertThat(new AbstractMessageWrapper(proto3Message).hashCode())
557+
.isEqualTo(proto3Message.hashCode());
558+
assertThat(
559+
new AbstractMessageWrapper(UnittestProto3.TestAllTypes.getDefaultInstance()).hashCode())
560+
.isEqualTo(UnittestProto3.TestAllTypes.getDefaultInstance().hashCode());
561+
562+
TestProto3Optional proto3Optional =
563+
TestProto3Optional.newBuilder()
564+
.setOptionalInt32(0)
565+
.setSingularInt32(0)
566+
.setSingularInt64(99L)
567+
.build();
568+
assertThat(new AbstractMessageWrapper(proto3Optional).hashCode())
569+
.isEqualTo(proto3Optional.hashCode());
570+
571+
TestOneof2 oneof2 =
572+
TestOneof2.newBuilder().setFooInt(0).setBarString("bar").setBazInt(10).build();
573+
assertThat(new AbstractMessageWrapper(oneof2).hashCode()).isEqualTo(oneof2.hashCode());
574+
575+
TestMap testMap = TestMap.newBuilder().putStringToInt32Field("k", 1).build();
576+
assertThat(new AbstractMessageWrapper(testMap).hashCode()).isEqualTo(testMap.hashCode());
577+
}
578+
542579
/**
543580
* Asserts that the given protos are not equal and have different hash codes.
544581
*

‎java/core/src/test/java/com/google/protobuf/DynamicMessageTest.java‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,22 @@
1313
import com.google.protobuf.Descriptors.EnumDescriptor;
1414
import com.google.protobuf.Descriptors.FieldDescriptor;
1515
import com.google.protobuf.Descriptors.OneofDescriptor;
16+
import com.google.protobuf.test.UnittestImport.ImportMessage;
17+
import com.google.protobuf.testing.proto.TestProto3Optional;
1618
import dynamicmessagetest.DynamicMessageTestProto.EmptyMessage;
1719
import dynamicmessagetest.DynamicMessageTestProto.MessageWithMapFields;
20+
import map_test.MapTestProto.TestMap;
1821
import proto2_unittest.UnittestMset.TestMessageSetExtension2;
1922
import proto2_unittest.UnittestProto;
2023
import proto2_unittest.UnittestProto.TestAllExtensions;
2124
import proto2_unittest.UnittestProto.TestAllTypes;
2225
import proto2_unittest.UnittestProto.TestAllTypes.NestedMessage;
2326
import proto2_unittest.UnittestProto.TestEmptyMessage;
27+
import proto2_unittest.UnittestProto.TestFieldOrderings;
28+
import proto2_unittest.UnittestProto.TestOneof2;
2429
import proto2_unittest.UnittestProto.TestPackedTypes;
2530
import proto2_wireformat_unittest.UnittestMsetWireFormat.TestMessageSet;
31+
import proto3_unittest.UnittestProto3;
2632
import java.util.ArrayList;
2733
import org.junit.Test;
2834
import org.junit.function.ThrowingRunnable;
@@ -425,4 +431,145 @@ public void serialize_lazyFieldInMessageSet() throws Exception {
425431
assertThat(complicatedlyBuiltMessage).isEqualTo(expectedMessage);
426432
assertThat(roundtrippedMessage).isEqualTo(expectedMessage);
427433
}
434+
435+
@Test
436+
public void hashCode_matchesGeneratedMessage_defaultInstanceWithImplicitPresence() {
437+
UnittestProto3.TestAllTypes proto3Default = UnittestProto3.TestAllTypes.getDefaultInstance();
438+
assertThat(
439+
DynamicMessage.getDefaultInstance(UnittestProto3.TestAllTypes.getDescriptor())
440+
.hashCode())
441+
.isEqualTo(proto3Default.hashCode());
442+
assertThat(
443+
DynamicMessage.newBuilder(UnittestProto3.TestAllTypes.getDescriptor())
444+
.build()
445+
.hashCode())
446+
.isEqualTo(proto3Default.hashCode());
447+
448+
TestProto3Optional proto3OptionalDefault = TestProto3Optional.getDefaultInstance();
449+
assertThat(DynamicMessage.getDefaultInstance(TestProto3Optional.getDescriptor()).hashCode())
450+
.isEqualTo(proto3OptionalDefault.hashCode());
451+
assertThat(DynamicMessage.newBuilder(TestProto3Optional.getDescriptor()).build().hashCode())
452+
.isEqualTo(proto3OptionalDefault.hashCode());
453+
}
454+
455+
@Test
456+
public void hashCode_matchesGeneratedMessage_proto3ImplicitAndExplicitPresenceAndOneof()
457+
throws Exception {
458+
UnittestProto3.TestAllTypes generated =
459+
UnittestProto3.TestAllTypes.newBuilder()
460+
.setOptionalInt32(100)
461+
.setOptionalInt64(9999999999L)
462+
.setOptionalFloat(2.5f)
463+
.setOptionalDouble(3.14)
464+
.setOptionalBool(true)
465+
.setOptionalString("hello")
466+
.setOptionalBytes(ByteString.copyFromUtf8("world"))
467+
.setOptionalNestedMessage(
468+
UnittestProto3.TestAllTypes.NestedMessage.getDefaultInstance())
469+
.setOptionalNestedEnum(UnittestProto3.TestAllTypes.NestedEnum.BAR)
470+
// Field 115 is declared before repeated fields 31..57 and oneof 111..114.
471+
.setOptionalLazyImportMessage(ImportMessage.newBuilder().setD(42).build())
472+
.addRepeatedString("a")
473+
.addRepeatedString("b")
474+
.addRepeatedNestedEnum(UnittestProto3.TestAllTypes.NestedEnum.FOO)
475+
.addRepeatedNestedEnumValue(999)
476+
.setOneofString("oneof_val")
477+
.setUnknownFields(
478+
UnknownFieldSet.newBuilder()
479+
.addField(999, UnknownFieldSet.Field.newBuilder().addVarint(77).build())
480+
.build())
481+
.build();
482+
483+
DynamicMessage dynamicFromCopy = DynamicMessage.newBuilder(generated).build();
484+
DynamicMessage dynamicFromBytes =
485+
DynamicMessage.parseFrom(
486+
UnittestProto3.TestAllTypes.getDescriptor(), generated.toByteString());
487+
488+
assertThat(dynamicFromCopy.hashCode()).isEqualTo(generated.hashCode());
489+
assertThat(dynamicFromBytes.hashCode()).isEqualTo(generated.hashCode());
490+
491+
TestProto3Optional proto3Optional =
492+
TestProto3Optional.newBuilder()
493+
.setOptionalInt32(0)
494+
.setOptionalString("")
495+
.setOptionalNestedMessage(TestProto3Optional.NestedMessage.getDefaultInstance())
496+
.setSingularInt32(0)
497+
.setSingularInt64(123L)
498+
.build();
499+
assertThat(DynamicMessage.newBuilder(proto3Optional).build().hashCode())
500+
.isEqualTo(proto3Optional.hashCode());
501+
assertThat(
502+
DynamicMessage.parseFrom(
503+
TestProto3Optional.getDescriptor(), proto3Optional.toByteString())
504+
.hashCode())
505+
.isEqualTo(proto3Optional.hashCode());
506+
}
507+
508+
@Test
509+
public void hashCode_matchesGeneratedMessage_outOfOrderFieldsOneofsExtensionsAndMaps()
510+
throws Exception {
511+
ExtensionRegistry registry = TestUtil.getFullExtensionRegistry();
512+
513+
TestFieldOrderings fieldOrderings =
514+
TestFieldOrderings.newBuilder()
515+
.setMyString("str")
516+
.setMyInt(12345L)
517+
.setMyFloat(1.5f)
518+
.setOptionalNestedMessage(
519+
TestFieldOrderings.NestedMessage.newBuilder().setOo(2).setBb(1).build())
520+
.setExtension(UnittestProto.myExtensionInt, 5)
521+
.setExtension(UnittestProto.myExtensionString, "ext")
522+
.setUnknownFields(
523+
UnknownFieldSet.newBuilder()
524+
.addField(999, UnknownFieldSet.Field.newBuilder().addVarint(77).build())
525+
.build())
526+
.build();
527+
assertThat(DynamicMessage.newBuilder(fieldOrderings).build().hashCode())
528+
.isEqualTo(fieldOrderings.hashCode());
529+
assertThat(
530+
DynamicMessage.parseFrom(
531+
TestFieldOrderings.getDescriptor(), fieldOrderings.toByteString(), registry)
532+
.hashCode())
533+
.isEqualTo(fieldOrderings.hashCode());
534+
535+
TestOneof2 oneof2 =
536+
TestOneof2.newBuilder()
537+
.setFooBytesCord(ByteString.copyFromUtf8("cord"))
538+
.setBarString("bar")
539+
.setBazInt(42)
540+
.setBazString("baz")
541+
.build();
542+
assertThat(DynamicMessage.newBuilder(oneof2).build().hashCode()).isEqualTo(oneof2.hashCode());
543+
assertThat(
544+
DynamicMessage.parseFrom(TestOneof2.getDescriptor(), oneof2.toByteString()).hashCode())
545+
.isEqualTo(oneof2.hashCode());
546+
547+
TestMap testMap =
548+
TestMap.newBuilder()
549+
.putInt32ToInt32Field(1, 10)
550+
.putInt32ToInt32Field(2, 20)
551+
.putInt32ToStringField(1, "a")
552+
.putInt32ToEnumField(1, TestMap.EnumValue.BAR)
553+
.putInt32ToMessageField(1, TestMap.MessageValue.getDefaultInstance())
554+
.putInt32ToMessageField(2, TestMap.MessageValue.newBuilder().setValue(99).build())
555+
.putStringToInt32Field("k", 7)
556+
.build();
557+
assertThat(DynamicMessage.newBuilder(testMap).build().hashCode()).isEqualTo(testMap.hashCode());
558+
assertThat(DynamicMessage.parseFrom(TestMap.getDescriptor(), testMap.toByteString()).hashCode())
559+
.isEqualTo(testMap.hashCode());
560+
}
561+
562+
@Test
563+
public void hashCode_matchesGeneratedMessage_unknownEnumValueInImplicitPresenceField()
564+
throws Exception {
565+
UnittestProto3.TestAllTypes generated =
566+
UnittestProto3.TestAllTypes.newBuilder().setOptionalNestedEnumValue(12345).build();
567+
DynamicMessage dynamicFromCopy = DynamicMessage.newBuilder(generated).build();
568+
DynamicMessage dynamicFromBytes =
569+
DynamicMessage.parseFrom(
570+
UnittestProto3.TestAllTypes.getDescriptor(), generated.toByteString());
571+
572+
assertThat(dynamicFromCopy.hashCode()).isEqualTo(generated.hashCode());
573+
assertThat(dynamicFromBytes.hashCode()).isEqualTo(generated.hashCode());
574+
}
428575
}

0 commit comments

Comments
 (0)