From 9f81d0194d813b34563b81a6c6decc9480d4104e Mon Sep 17 00:00:00 2001 From: Sean Huh Date: Mon, 26 Oct 2026 14:33:03 -0700 Subject: [PATCH] Prevent flattening nested optionals in optional field selection and fix optional equality PiperOrigin-RevId: 1007561088 --- .../extensions/CelOptionalLibraryTest.java | 28 +++++++++++++++++++ .../runtime/ProtoMessageRuntimeEquality.java | 3 -- .../java/dev/cel/runtime/RuntimeEquality.java | 12 ++++++++ .../planner/EvalOptionalSelectField.java | 12 ++++---- .../ProtoMessageRuntimeEqualityTest.java | 9 ++++++ .../dev/cel/runtime/RuntimeEqualityTest.java | 5 ++++ 6 files changed, 59 insertions(+), 10 deletions(-) diff --git a/extensions/src/test/java/dev/cel/extensions/CelOptionalLibraryTest.java b/extensions/src/test/java/dev/cel/extensions/CelOptionalLibraryTest.java index f594c6dc2..8be97b40d 100644 --- a/extensions/src/test/java/dev/cel/extensions/CelOptionalLibraryTest.java +++ b/extensions/src/test/java/dev/cel/extensions/CelOptionalLibraryTest.java @@ -619,6 +619,34 @@ public void optionalFieldSelection_onMap_chained_returnsSinglyWrappedOptional() assertThat(result).hasValue("baz"); } + @Test + @TestParameters("{source: \"{'a': optional.none()}.?a\"}") + @TestParameters("{source: \"optional.of({'a': optional.none()}).?a\"}") + @TestParameters("{source: \"optional.of({'a': optional.none()}).a\"}") + public void optionalFieldSelection_onMapWithEmptyOptionalValue_returnsNestedOptionalEmpty( + String source) throws Exception { + Cel cel = newCelBuilder().build(); + CelAbstractSyntaxTree ast = compile(cel, source); + + Optional> result = (Optional>) cel.createProgram(ast).eval(); + + assertThat(result).hasValue(Optional.empty()); + } + + @Test + @TestParameters("{source: \"{'a': optional.of(1)}.?a\"}") + @TestParameters("{source: \"optional.of({'a': optional.of(1)}).?a\"}") + @TestParameters("{source: \"optional.of({'a': optional.of(1)}).a\"}") + public void optionalFieldSelection_onMapWithPresentOptionalValue_returnsNestedOptionalValue( + String source) throws Exception { + Cel cel = newCelBuilder().build(); + CelAbstractSyntaxTree ast = compile(cel, source); + + Optional> result = (Optional>) cel.createProgram(ast).eval(); + + assertThat(result).hasValue(Optional.of(1L)); + } + @Test public void optionalFieldSelection_onProtoMessage_returnsOptionalEmpty() throws Exception { Cel cel = diff --git a/runtime/src/main/java/dev/cel/runtime/ProtoMessageRuntimeEquality.java b/runtime/src/main/java/dev/cel/runtime/ProtoMessageRuntimeEquality.java index 25a090081..76d117614 100644 --- a/runtime/src/main/java/dev/cel/runtime/ProtoMessageRuntimeEquality.java +++ b/runtime/src/main/java/dev/cel/runtime/ProtoMessageRuntimeEquality.java @@ -45,9 +45,6 @@ public boolean objectEquals(Object x, Object y) { if (celOptions.disableCelStandardEquality()) { return Objects.equals(x, y); } - if (x == y) { - return true; - } if (celOptions.enableProtoDifferencerEquality()) { x = runtimeHelpers.adaptValue(x); diff --git a/runtime/src/main/java/dev/cel/runtime/RuntimeEquality.java b/runtime/src/main/java/dev/cel/runtime/RuntimeEquality.java index a9b607e2b..c8cffe115 100644 --- a/runtime/src/main/java/dev/cel/runtime/RuntimeEquality.java +++ b/runtime/src/main/java/dev/cel/runtime/RuntimeEquality.java @@ -140,6 +140,15 @@ public boolean objectEquals(Object x, Object y) { if (celOptions.disableCelStandardEquality()) { return Objects.equals(x, y); } + if (x instanceof Optional) { + if (!(y instanceof Optional)) { + return false; + } + Optional xOpt = (Optional) x; + Optional yOpt = (Optional) y; + return xOpt.isPresent() == yOpt.isPresent() + && (!xOpt.isPresent() || objectEquals(xOpt.get(), yOpt.get())); + } if (x == y && !isNan(x)) { return true; } @@ -234,6 +243,9 @@ public int hashCode(Object object) { if (celOptions.disableCelStandardEquality()) { return Objects.hashCode(object); } + if (object instanceof Optional) { + return ((Optional) object).map(this::hashCode).orElse(0); + } object = runtimeHelpers.adaptValue(object); if (object instanceof Number) { diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalOptionalSelectField.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalOptionalSelectField.java index 2b6969559..da90e76d8 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalOptionalSelectField.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalOptionalSelectField.java @@ -37,8 +37,8 @@ final class EvalOptionalSelectField extends PlannedInterpretable { @Override Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) { Object operandValue = EvalHelpers.evalStrictly(operand, resolver, frame); - - if (operandValue instanceof Optional) { + boolean isOptionalOperand = operandValue instanceof Optional; + if (isOptionalOperand) { Optional opt = (Optional) operandValue; if (!opt.isPresent()) { return Optional.empty(); @@ -71,15 +71,13 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) { Object resultValue = EvalHelpers.evalStrictly(selectAttribute, resolver, frame); - if (resultValue instanceof Optional) { - return resultValue; - } - if (resultValue instanceof AccumulatedUnknowns) { return resultValue; } - return Optional.of(resultValue); + // When the operand is Optional, selectAttribute traverses OptionalValue, which already wraps + // the selected field value in Optional. + return isOptionalOperand ? resultValue : Optional.of(resultValue); } static EvalOptionalSelectField create( diff --git a/runtime/src/test/java/dev/cel/runtime/ProtoMessageRuntimeEqualityTest.java b/runtime/src/test/java/dev/cel/runtime/ProtoMessageRuntimeEqualityTest.java index dc4607e5f..8c593686a 100644 --- a/runtime/src/test/java/dev/cel/runtime/ProtoMessageRuntimeEqualityTest.java +++ b/runtime/src/test/java/dev/cel/runtime/ProtoMessageRuntimeEqualityTest.java @@ -52,6 +52,7 @@ import dev.cel.common.values.CelByteString; import java.util.Arrays; import java.util.List; +import java.util.Optional; import org.jspecify.annotations.Nullable; import org.junit.Assert; import org.junit.Test; @@ -288,6 +289,7 @@ public static List data() { // Double tests. {1.0, 1.0, Result.alwaysTrue()}, + {Double.NaN, Double.NaN, Result.alwaysFalse()}, {Double.valueOf(1.0), 1.0, Result.alwaysTrue()}, {DoubleValue.of(42.5), 42.5, Result.alwaysTrue()}, // Floats are unwrapped to double types. @@ -346,6 +348,13 @@ public static List data() { Result.alwaysTrue() }, + // Optional tests. + {Optional.empty(), Optional.empty(), Result.alwaysTrue()}, + {Optional.of(1), 1, Result.alwaysFalse()}, + {Optional.of(Double.NaN), Optional.of(Double.NaN), Result.alwaysFalse()}, + {Optional.of(Int32Value.of(1)), Optional.of(1L), Result.alwaysTrue()}, + {Optional.of(Optional.empty()), Optional.empty(), Result.alwaysFalse()}, + // String tests. {"", "", Result.alwaysTrue()}, {"str", "str", Result.alwaysTrue()}, diff --git a/runtime/src/test/java/dev/cel/runtime/RuntimeEqualityTest.java b/runtime/src/test/java/dev/cel/runtime/RuntimeEqualityTest.java index e67c4f124..78caeca01 100644 --- a/runtime/src/test/java/dev/cel/runtime/RuntimeEqualityTest.java +++ b/runtime/src/test/java/dev/cel/runtime/RuntimeEqualityTest.java @@ -24,6 +24,7 @@ import dev.cel.common.CelOptions; import dev.cel.common.values.ProtoMessageLiteValueProvider; import dev.cel.expr.conformance.proto2.TestAllTypes; +import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; @@ -47,6 +48,8 @@ public void objectEquals_and_hashCode() { runtimeEquality, ImmutableMap.of("a", 1, "b", 2), ImmutableMap.of("a", 1L, "b", UnsignedLong.valueOf(2))); + assertEqualityAndHashCode(runtimeEquality, Optional.empty(), Optional.empty()); + assertEqualityAndHashCode(runtimeEquality, Optional.of(1), Optional.of(1.0)); } private void assertEqualityAndHashCode(RuntimeEquality runtimeEquality, Object obj1, Object obj2) { @@ -105,7 +108,9 @@ public void objectEquals_nanWithIdenticalReference_returnsFalse() { RuntimeEquality runtimeEquality = RuntimeEquality.create(RuntimeHelpers.create(), CelOptions.DEFAULT); Double nan = Double.NaN; + Optional optNan = Optional.of(nan); assertThat(runtimeEquality.objectEquals(nan, nan)).isFalse(); + assertThat(runtimeEquality.objectEquals(optNan, optNan)).isFalse(); } }