Skip to content

Commit e49d0cd

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Align protobuf message equality and hashing with IEEE 754 spec
PiperOrigin-RevId: 1008426293
1 parent 361c017 commit e49d0cd

4 files changed

Lines changed: 197 additions & 23 deletions

File tree

‎common/src/main/java/dev/cel/common/internal/ProtoEquality.java‎

Lines changed: 47 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
import com.google.auto.value.AutoValue;
1818
import com.google.common.collect.ImmutableMap;
19+
import com.google.common.collect.Lists;
20+
import com.google.common.collect.Maps;
1921
import com.google.errorprone.annotations.CheckReturnValue;
2022
import com.google.errorprone.annotations.Immutable;
2123
import com.google.protobuf.Any;
@@ -26,6 +28,7 @@
2628
import dev.cel.common.annotations.Internal;
2729
import java.util.List;
2830
import java.util.Map;
31+
import java.util.Objects;
2932
import java.util.Optional;
3033

3134
/**
@@ -35,9 +38,11 @@
3538
* <p>These are the key differences between CEL's proto equality and Java's proto equality:
3639
*
3740
* <ul>
38-
* <li/>NaN is not equal to itself.
39-
* <li/>Any values are unpacked before comparison.
40-
* <li/>If two Any values cannot be unpacked, they are compared by bytes.
41+
* <li>NaN is not equal to itself.
42+
* <li>Positive and negative zero (+0.0 and -0.0) compare as equal when both fields are present
43+
* (under proto3 implicit presence, -0.0 is serialized as present while +0.0 is unset).
44+
* <li>Any values are unpacked before comparison.
45+
* <li>If two Any values cannot be unpacked, they are compared by bytes.
4146
* </ul>
4247
*
4348
* <p>CEL Library Internals. Do Not Use.
@@ -56,9 +61,6 @@ public ProtoEquality(DynamicProto dynamicProto) {
5661
}
5762

5863
public boolean equals(Message message1, Message message2) {
59-
if (message1 == message2) {
60-
return true;
61-
}
6264
if (!message1.getDescriptorForType().equals(message2.getDescriptorForType())) {
6365
return false;
6466
}
@@ -124,22 +126,51 @@ public boolean equals(Message message1, Message message2) {
124126
return true;
125127
}
126128

129+
/** Returns a hash code consistent with {@link #equals(Message, Message)}. */
130+
public int hashCode(Message message) {
131+
if (isAny(message)) {
132+
// Constant hash: unpacked Any equality ignores wire bytes, while fallback ignores type_url.
133+
return ANY_FULL_NAME.hashCode();
134+
}
135+
return Objects.hash(
136+
message.getDescriptorForType(),
137+
message.getUnknownFields(),
138+
Maps.transformEntries(message.getAllFields(), this::hashFieldEntry));
139+
}
140+
141+
private int hashFieldEntry(FieldDescriptor field, Object value) {
142+
if (field.isMapField()) {
143+
ProtoMap protoMap = protoMap((List<?>) value);
144+
return Maps.transformValues(protoMap.map(), v -> hashFieldValue(protoMap.valueField(), v))
145+
.hashCode();
146+
}
147+
if (field.isRepeated()) {
148+
return Lists.transform((List<?>) value, elem -> hashFieldValue(field, elem)).hashCode();
149+
}
150+
return hashFieldValue(field, value);
151+
}
152+
153+
private int hashFieldValue(FieldDescriptor field, Object value) {
154+
switch (field.getJavaType()) {
155+
case MESSAGE:
156+
return hashCode((Message) value);
157+
case DOUBLE:
158+
case FLOAT:
159+
double v = ((Number) value).doubleValue();
160+
// Normalize -0.0 to 0.0 so equal zeros hash alike.
161+
return Double.hashCode(v == 0.0d ? 0.0d : v);
162+
default:
163+
return value.hashCode();
164+
}
165+
}
166+
127167
private boolean equalsFieldValues(FieldDescriptor field, Object value1, Object value2) {
128168
switch (field.getJavaType()) {
129169
case MESSAGE:
130170
return equals((Message) value1, (Message) value2);
131171
case DOUBLE:
132-
// fallthrough
133172
case FLOAT:
134-
Number num1 = (Number) value1;
135-
Number num2 = (Number) value2;
136-
boolean num1IsNaN = Double.isNaN(num1.doubleValue());
137-
boolean num2IsNaN = Double.isNaN(num2.doubleValue());
138-
// handle the NaN case
139-
if (num1IsNaN && num2IsNaN) {
140-
return false;
141-
}
142-
return num1.equals(num2);
173+
return ((Number) value1).doubleValue() == ((Number) value2).doubleValue();
143174
default:
144175
return value1.equals(value2);
145176
}

‎common/src/test/java/dev/cel/common/internal/ProtoEqualityTest.java‎

Lines changed: 106 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,17 +19,20 @@
1919
import com.google.protobuf.Any;
2020
import com.google.protobuf.DynamicMessage;
2121
import com.google.protobuf.InvalidProtocolBufferException;
22+
import com.google.protobuf.Message;
2223
import com.google.protobuf.Struct;
24+
import com.google.protobuf.UnknownFieldSet;
2325
import com.google.protobuf.Value;
26+
import com.google.testing.junit.testparameterinjector.TestParameter;
27+
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
2428
import dev.cel.expr.conformance.proto3.TestAllTypes;
2529
import dev.cel.expr.conformance.proto3.TestAllTypes.NestedEnum;
2630
import dev.cel.expr.conformance.proto3.TestAllTypes.NestedMessage;
2731
import org.junit.Before;
2832
import org.junit.Test;
2933
import org.junit.runner.RunWith;
30-
import org.junit.runners.JUnit4;
3134

32-
@RunWith(JUnit4.class)
35+
@RunWith(TestParameterInjector.class)
3336
public final class ProtoEqualityTest {
3437

3538
private DynamicProto dynamicProto;
@@ -59,13 +62,22 @@ public void notEqualsFloatFields() {
5962
.isFalse();
6063
}
6164

65+
@Test
66+
public void notEqualsNegativeZeroFields_proto3ImplicitPresence() {
67+
// Proto3 implicit presence treats -0.0 as wire-present and +0.0 as unset, matching C++/Go.
68+
TestAllTypes negativeZero = TestAllTypes.newBuilder().setSingleDouble(-0.0).build();
69+
TestAllTypes positiveZero = TestAllTypes.newBuilder().setSingleDouble(0.0).build();
70+
71+
assertThat(protoEquality.equals(negativeZero, positiveZero)).isFalse();
72+
}
73+
6274
@Test
6375
public void notEqualsNaNFields() {
64-
assertThat(
65-
protoEquality.equals(
66-
TestAllTypes.newBuilder().setSingleDouble(Double.NaN).build(),
67-
TestAllTypes.newBuilder().setSingleDouble(Double.NaN).build()))
68-
.isFalse();
76+
TestAllTypes nanDouble = TestAllTypes.newBuilder().setSingleDouble(Double.NaN).build();
77+
TestAllTypes nanFloat = TestAllTypes.newBuilder().setSingleFloat(Float.NaN).build();
78+
79+
assertThat(protoEquality.equals(nanDouble, nanDouble)).isFalse();
80+
assertThat(protoEquality.equals(nanFloat, nanFloat)).isFalse();
6981
}
7082

7183
@Test
@@ -240,6 +252,8 @@ public void equalsMessageAnyFields() {
240252
Any packedStruct = Any.pack(struct);
241253
Any packedStruct2 = Any.pack(struct);
242254
assertThat(protoEquality.equals(packedStruct, packedStruct2)).isTrue();
255+
assertThat(protoEquality.hashCode(packedStruct))
256+
.isEqualTo(protoEquality.hashCode(packedStruct2));
243257
}
244258

245259
@Test
@@ -253,6 +267,8 @@ public void equalsMessageUnknownAnyFields() {
253267
Any packedStruct2 = Any.pack(struct);
254268
packedStruct = packedStruct.toBuilder().setTypeUrl("type.googleapis.com/Unknown").build();
255269
assertThat(protoEquality.equals(packedStruct, packedStruct2)).isTrue();
270+
assertThat(protoEquality.hashCode(packedStruct))
271+
.isEqualTo(protoEquality.hashCode(packedStruct2));
256272
}
257273

258274
@Test
@@ -275,5 +291,88 @@ public void equalsMessageDynamicAnyFields() throws InvalidProtocolBufferExceptio
275291
doublePackedStruct.getValue(),
276292
DefaultDescriptorPool.INSTANCE.getExtensionRegistry());
277293
assertThat(protoEquality.equals(dynAny, dynAny2)).isTrue();
294+
assertThat(protoEquality.hashCode(dynAny)).isEqualTo(protoEquality.hashCode(dynAny2));
295+
}
296+
297+
@SuppressWarnings("ImmutableEnumChecker") // Test only
298+
private enum EqualMessagePairTestCase {
299+
REPEATED_SIGNED_ZERO(
300+
TestAllTypes.newBuilder().addRepeatedDouble(-0.0).addRepeatedFloat(-0.0f).build(),
301+
TestAllTypes.newBuilder().addRepeatedDouble(0.0).addRepeatedFloat(0.0f).build()),
302+
MAP_KEY_ORDER(
303+
TestAllTypes.newBuilder()
304+
.putMapStringString("a", "1")
305+
.putMapStringString("b", "10")
306+
.build(),
307+
TestAllTypes.newBuilder()
308+
.putMapStringString("b", "10")
309+
.putMapStringString("a", "1")
310+
.build()),
311+
SINGLE_VALUE_SIGNED_ZERO(
312+
TestAllTypes.newBuilder().setSingleValue(Value.newBuilder().setNumberValue(-0.0)).build(),
313+
TestAllTypes.newBuilder().setSingleValue(Value.newBuilder().setNumberValue(0.0)).build()),
314+
MAP_DOUBLE_VALUE_SIGNED_ZERO(
315+
TestAllTypes.newBuilder().putMapInt64Double(1L, -0.0).build(),
316+
TestAllTypes.newBuilder().putMapInt64Double(1L, 0.0).build());
317+
318+
private final Message x;
319+
private final Message y;
320+
321+
EqualMessagePairTestCase(Message x, Message y) {
322+
this.x = x;
323+
this.y = y;
324+
}
325+
}
326+
327+
@SuppressWarnings("ImmutableEnumChecker") // Test only
328+
private enum DistinctMessagePairTestCase {
329+
DIFFERENT_UNKNOWN_FIELDS(
330+
TestAllTypes.getDefaultInstance(),
331+
TestAllTypes.newBuilder()
332+
.setUnknownFields(
333+
UnknownFieldSet.newBuilder()
334+
.addField(999, UnknownFieldSet.Field.newBuilder().addVarint(1L).build())
335+
.build())
336+
.build()),
337+
DIFFERENT_SCALAR_DOUBLES(
338+
TestAllTypes.newBuilder().setSingleDouble(1.0).build(),
339+
TestAllTypes.newBuilder().setSingleDouble(2.0).build()),
340+
DIFFERENT_SCALAR_INTS(
341+
TestAllTypes.newBuilder().setSingleInt32(1).build(),
342+
TestAllTypes.newBuilder().setSingleInt32(2).build()),
343+
DIFFERENT_REPEATED_DOUBLES(
344+
TestAllTypes.newBuilder().addRepeatedDouble(1.0).build(),
345+
TestAllTypes.newBuilder().addRepeatedDouble(2.0).build()),
346+
DIFFERENT_MAP_VALUES(
347+
TestAllTypes.newBuilder().putMapStringString("a", "1").build(),
348+
TestAllTypes.newBuilder().putMapStringString("a", "2").build()),
349+
DIFFERENT_NESTED_MESSAGES(
350+
TestAllTypes.newBuilder()
351+
.setSingleNestedMessage(NestedMessage.newBuilder().setBb(1))
352+
.build(),
353+
TestAllTypes.newBuilder()
354+
.setSingleNestedMessage(NestedMessage.newBuilder().setBb(2))
355+
.build());
356+
357+
private final Message x;
358+
private final Message y;
359+
360+
DistinctMessagePairTestCase(Message x, Message y) {
361+
this.x = x;
362+
this.y = y;
363+
}
364+
}
365+
366+
@Test
367+
public void hashCode_equalMessagesHaveEqualHash(
368+
@TestParameter EqualMessagePairTestCase testCase) {
369+
assertThat(protoEquality.equals(testCase.x, testCase.y)).isTrue();
370+
assertThat(protoEquality.hashCode(testCase.x)).isEqualTo(protoEquality.hashCode(testCase.y));
371+
}
372+
373+
@Test
374+
public void hashCode_distinctMessagesHaveDifferentHashes(
375+
@TestParameter DistinctMessagePairTestCase testCase) {
376+
assertThat(protoEquality.hashCode(testCase.x)).isNotEqualTo(protoEquality.hashCode(testCase.y));
278377
}
279378
}

‎runtime/src/main/java/dev/cel/runtime/ProtoMessageRuntimeEquality.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,17 @@ public boolean objectEquals(Object x, Object y) {
6060
return super.objectEquals(x, y);
6161
}
6262

63+
@Override
64+
public int hashCode(Object object) {
65+
if (!celOptions.disableCelStandardEquality() && celOptions.enableProtoDifferencerEquality()) {
66+
object = runtimeHelpers.adaptValue(object);
67+
if (object instanceof Message) {
68+
return protoEquality.hashCode((Message) object);
69+
}
70+
}
71+
return super.hashCode(object);
72+
}
73+
6374
private ProtoMessageRuntimeEquality(DynamicProto dynamicProto, CelOptions celOptions) {
6475
super(ProtoMessageRuntimeHelpers.create(dynamicProto, celOptions), celOptions);
6576
this.protoEquality = new ProtoEquality(dynamicProto);

‎runtime/src/test/java/dev/cel/runtime/ProtoMessageRuntimeEqualityTest.java‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,22 @@ public void indexMap() throws Exception {
172172
() -> RUNTIME_EQUALITY_DEFAULT_OPTIONS.indexMap(mixedKeyMap, 1.1));
173173
}
174174

175+
@Test
176+
public void hashCode_delegatesBasedOnOptions() {
177+
AttributeContext hostAttr =
178+
AttributeContext.newBuilder().setRequest(Request.newBuilder().setHost("localhost")).build();
179+
ProtoMessageRuntimeEquality legacyWithProtoDiff =
180+
ProtoMessageRuntimeEquality.create(
181+
DYNAMIC_PROTO, PROTO_EQUALITY.toBuilder().disableCelStandardEquality(true).build());
182+
183+
assertThat(RUNTIME_EQUALITY_PROTO_EQUALITY.hashCode(hostAttr))
184+
.isNotEqualTo(hostAttr.hashCode());
185+
assertThat(RUNTIME_EQUALITY_EMPTY_OPTIONS.hashCode(hostAttr)).isEqualTo(hostAttr.hashCode());
186+
assertThat(legacyWithProtoDiff.hashCode(hostAttr)).isEqualTo(hostAttr.hashCode());
187+
assertThat(RUNTIME_EQUALITY_LEGACY_OPTIONS.hashCode(1))
188+
.isNotEqualTo(RUNTIME_EQUALITY_LEGACY_OPTIONS.hashCode(1.0));
189+
}
190+
175191
@AutoValue
176192
abstract static class State {
177193
/**
@@ -652,6 +668,19 @@ public static List<Object[]> data() {
652668
.build(),
653669
Result.proto(/* equalsOutcome= */ true, /* diffOutcome= */ false),
654670
},
671+
{
672+
Auth.newBuilder()
673+
.setClaims(
674+
Struct.newBuilder()
675+
.putFields("custom", Value.newBuilder().setNumberValue(-0.0).build()))
676+
.build(),
677+
Auth.newBuilder()
678+
.setClaims(
679+
Struct.newBuilder()
680+
.putFields("custom", Value.newBuilder().setNumberValue(0.0).build()))
681+
.build(),
682+
Result.proto(/* equalsOutcome= */ false, /* diffOutcome= */ true),
683+
},
655684

656685
// Note: this is the motivating use case for converting to heterogeneous equality in
657686
// the future.
@@ -695,6 +724,10 @@ public void objectEquals() throws Exception {
695724
}
696725
assertThat(state.runtimeEquality().objectEquals(lhs, rhs)).isEqualTo(state.outcome());
697726
assertThat(state.runtimeEquality().objectEquals(rhs, lhs)).isEqualTo(state.outcome());
727+
if (state.outcome() && lhs != null && rhs != null) {
728+
assertThat(state.runtimeEquality().hashCode(lhs))
729+
.isEqualTo(state.runtimeEquality().hashCode(rhs));
730+
}
698731
}
699732
}
700733
}

0 commit comments

Comments
 (0)