diff --git a/common/src/main/java/dev/cel/common/values/BUILD.bazel b/common/src/main/java/dev/cel/common/values/BUILD.bazel index 51fa12e98..895a3410b 100644 --- a/common/src/main/java/dev/cel/common/values/BUILD.bazel +++ b/common/src/main/java/dev/cel/common/values/BUILD.bazel @@ -167,6 +167,7 @@ java_library( ":preadapted_list", "//:auto_value", "//common/annotations", + "//common/exceptions:invalid_argument", "//common/types", "//common/types:type_providers", "@maven//:com_google_errorprone_error_prone_annotations", @@ -218,6 +219,7 @@ cel_android_library( ":preadapted_list_android", "//:auto_value", "//common/annotations", + "//common/exceptions:invalid_argument", "//common/types:type_providers_android", "//common/types:types_android", "@maven//:com_google_errorprone_error_prone_annotations", diff --git a/common/src/main/java/dev/cel/common/values/CelValueConverter.java b/common/src/main/java/dev/cel/common/values/CelValueConverter.java index 20deef1d3..3e00be2e6 100644 --- a/common/src/main/java/dev/cel/common/values/CelValueConverter.java +++ b/common/src/main/java/dev/cel/common/values/CelValueConverter.java @@ -17,8 +17,10 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.Immutable; import dev.cel.common.annotations.Internal; +import dev.cel.common.exceptions.CelInvalidArgumentException; import java.util.Collection; import java.util.Iterator; import java.util.List; @@ -26,6 +28,7 @@ import java.util.Optional; import java.util.RandomAccess; import java.util.function.Function; +import org.jspecify.annotations.Nullable; /** * {@code CelValueConverter} handles bidirectional conversion between native Java objects to {@link @@ -74,7 +77,7 @@ protected Object mapContainer(Object value, Function mapper) { if (value instanceof List && value instanceof RandomAccess) { List list = (List) value; for (int i = 0; i < list.size(); i++) { - Object element = list.get(i); + Object element = checkListElement(list.get(i), i); Object mapped = mapper.apply(element); if (mapped != element) { @@ -85,7 +88,7 @@ protected Object mapContainer(Object value, Function mapper) { } builder.add(mapped); for (int j = i + 1; j < list.size(); j++) { - builder.add(mapper.apply(list.get(j))); + builder.add(mapper.apply(checkListElement(list.get(j), j))); } return builder.build(); } @@ -100,8 +103,9 @@ protected Object mapContainer(Object value, Function mapper) { Collection collection = (Collection) value; ImmutableList.Builder builder = ImmutableList.builderWithExpectedSize(collection.size()); + int index = 0; for (Object element : collection) { - builder.add(mapper.apply(element)); + builder.add(mapper.apply(checkListElement(element, index++))); } return builder.build(); } @@ -112,6 +116,7 @@ protected Object mapContainer(Object value, Function mapper) { while (iterator.hasNext()) { Map.Entry entry = iterator.next(); + checkMapEntry(entry); Object mappedKey = mapper.apply(entry.getKey()); Object mappedValue = mapper.apply(entry.getValue()); @@ -128,6 +133,7 @@ protected Object mapContainer(Object value, Function mapper) { builder.put(mappedKey, mappedValue); while (iterator.hasNext()) { Map.Entry nextEntry = iterator.next(); + checkMapEntry(nextEntry); builder.put(mapper.apply(nextEntry.getKey()), mapper.apply(nextEntry.getValue())); } return builder.buildOrThrow(); @@ -162,6 +168,59 @@ public Object toRuntimeValue(Object value) { return normalizePrimitive(value); } + /** + * Adapts {@code value} for an intermediate field selection hop. + * + *

{@link Map} instances are returned as-is to avoid O(N) whole-map normalization per hop; the + * accessed entry is validated on lookup via {@link #findMapValue} or {@link #containsMapKey}. + * Callers materializing a final evaluation result must use {@link #toRuntimeValue} instead. + */ + public final Object toTraversalTarget(Object value) { + if (value instanceof Map) { + return value; + } + + return toRuntimeValue(value); + } + + /** + * Returns the unadapted value bound to {@code key} in {@code map}, or {@link Optional#empty()} if + * absent. + * + * @throws CelInvalidArgumentException if {@code key} is bound to {@code null}. + */ + public static Optional findMapValue(Map map, Object key) { + Object value = map.get(key); + if (value != null) { + return Optional.of(value); + } + + if (map.containsKey(key)) { + throw new CelInvalidArgumentException( + String.format("Map value cannot be null for key: %s", key)); + } + + return Optional.empty(); + } + + /** + * Returns whether {@code key} is present in {@code map}. + * + * @throws CelInvalidArgumentException if {@code key} is bound to {@code null}. + */ + public static boolean containsMapKey(Map map, Object key) { + if (map.get(key) != null) { + return true; + } + + if (map.containsKey(key)) { + throw new CelInvalidArgumentException( + String.format("Map value cannot be null for key: %s", key)); + } + + return false; + } + protected Object normalizePrimitive(Object value) { Preconditions.checkNotNull(value); @@ -196,6 +255,28 @@ private Object unwrap(CelValue celValue) { return celValue.value(); } + private static void checkMapEntry(Map.Entry entry) { + Object key = entry.getKey(); + if (key == null) { + throw new CelInvalidArgumentException("Map key cannot be null."); + } + + if (entry.getValue() == null) { + throw new CelInvalidArgumentException( + String.format("Map value cannot be null for key: %s", key)); + } + } + + @CanIgnoreReturnValue + private static Object checkListElement(@Nullable Object element, int index) { + if (element == null) { + throw new CelInvalidArgumentException( + String.format("List element cannot be null at index: %d", index)); + } + + return element; + } + protected CelValueConverter() { this.maybeUnwrapFunction = this::maybeUnwrap; this.toRuntimeValueFunction = this::toRuntimeValue; diff --git a/common/src/main/java/dev/cel/common/values/MutableMapValue.java b/common/src/main/java/dev/cel/common/values/MutableMapValue.java index 706436b2e..4f6cfa882 100644 --- a/common/src/main/java/dev/cel/common/values/MutableMapValue.java +++ b/common/src/main/java/dev/cel/common/values/MutableMapValue.java @@ -105,23 +105,13 @@ public Set> entrySet() { @Override public Object select(Object field) { - Object val = internalMap.get(field); - if (val != null) { - return val; - } - if (!internalMap.containsKey(field)) { - throw CelAttributeNotFoundException.forMissingMapKey(field.toString()); - } - throw CelAttributeNotFoundException.of( - String.format("Map value cannot be null for key: %s", field)); + return CelValueConverter.findMapValue(internalMap, field) + .orElseThrow(() -> CelAttributeNotFoundException.forMissingMapKey(field.toString())); } @Override public Optional find(Object field) { - if (internalMap.containsKey(field)) { - return Optional.ofNullable(internalMap.get(field)); - } - return Optional.empty(); + return CelValueConverter.findMapValue(internalMap, field); } @Override diff --git a/common/src/test/java/dev/cel/common/values/BUILD.bazel b/common/src/test/java/dev/cel/common/values/BUILD.bazel index 1732c6667..a6947b979 100644 --- a/common/src/test/java/dev/cel/common/values/BUILD.bazel +++ b/common/src/test/java/dev/cel/common/values/BUILD.bazel @@ -14,8 +14,10 @@ java_library( "//bundle:cel", "//common:cel_ast", "//common:cel_descriptor_util", + "//common:error_codes", "//common:options", "//common/exceptions:attribute_not_found", + "//common/exceptions:invalid_argument", "//common/internal:cel_descriptor_pools", "//common/internal:cel_lite_descriptor_pool", "//common/internal:default_lite_descriptor_pool", diff --git a/common/src/test/java/dev/cel/common/values/CelValueConverterTest.java b/common/src/test/java/dev/cel/common/values/CelValueConverterTest.java index ccb8e605f..75d9182b3 100644 --- a/common/src/test/java/dev/cel/common/values/CelValueConverterTest.java +++ b/common/src/test/java/dev/cel/common/values/CelValueConverterTest.java @@ -15,7 +15,16 @@ package dev.cel.common.values; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; +import dev.cel.common.CelErrorCode; +import dev.cel.common.exceptions.CelInvalidArgumentException; +import java.util.Arrays; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; @@ -50,4 +59,180 @@ public void unwrap_emptyOptionalValue() { assertThat(result).isEqualTo(Optional.empty()); } + + @Test + public void toRuntimeValue_mapWithNullValue_throws() { + Map map = new HashMap<>(); + map.put("key", null); + + CelInvalidArgumentException e = + assertThrows( + CelInvalidArgumentException.class, () -> CEL_VALUE_CONVERTER.toRuntimeValue(map)); + + assertThat(e).hasMessageThat().isEqualTo("Map value cannot be null for key: key"); + assertThat(e.getErrorCode()).isEqualTo(CelErrorCode.INVALID_ARGUMENT); + } + + @Test + public void toRuntimeValue_mapWithNullKey_throws() { + Map map = new HashMap<>(); + map.put(null, "value"); + + CelInvalidArgumentException e = + assertThrows( + CelInvalidArgumentException.class, () -> CEL_VALUE_CONVERTER.toRuntimeValue(map)); + + assertThat(e).hasMessageThat().isEqualTo("Map key cannot be null."); + } + + @Test + public void toRuntimeValue_mapWithNullValueAfterAdaptedEntry_throws() { + // The first entry normalizes (Integer -> Long), which diverts mapContainer onto its rebuild + // path. The illegal entry is only reached by the tail loop. + Map map = new LinkedHashMap<>(); + map.put("adapted", 1); + map.put("illegal", null); + + CelInvalidArgumentException e = + assertThrows( + CelInvalidArgumentException.class, () -> CEL_VALUE_CONVERTER.toRuntimeValue(map)); + + assertThat(e).hasMessageThat().isEqualTo("Map value cannot be null for key: illegal"); + } + + @Test + public void maybeUnwrap_mapWithNullValue_throws() { + Map map = new HashMap<>(); + map.put("key", null); + + CelInvalidArgumentException e = + assertThrows(CelInvalidArgumentException.class, () -> CEL_VALUE_CONVERTER.maybeUnwrap(map)); + + assertThat(e).hasMessageThat().isEqualTo("Map value cannot be null for key: key"); + } + + @Test + public void toTraversalTarget_map_returnsSameInstanceWithoutInspectingEntries() { + Map map = new HashMap<>(); + map.put("illegal", null); + + Object result = CEL_VALUE_CONVERTER.toTraversalTarget(map); + + assertThat(result).isSameInstanceAs(map); + } + + @Test + public void toTraversalTarget_nonMap_normalizes() { + Object result = CEL_VALUE_CONVERTER.toTraversalTarget(1); + + assertThat(result).isEqualTo(1L); + } + + @Test + public void findMapValue_boundKey_returnsValueAsStored() { + Map map = new HashMap<>(); + map.put("key", 1); + + Optional result = CelValueConverter.findMapValue(map, "key"); + + // Unadapted: the caller decides whether this hop materializes or merely traverses. + assertThat(result).hasValue(1); + } + + @Test + public void findMapValue_absentKey_returnsEmpty() { + Optional result = CelValueConverter.findMapValue(new HashMap<>(), "key"); + + assertThat(result).isEmpty(); + } + + @Test + public void findMapValue_nullBoundKey_throws() { + Map map = new HashMap<>(); + map.put("key", null); + + CelInvalidArgumentException e = + assertThrows( + CelInvalidArgumentException.class, () -> CelValueConverter.findMapValue(map, "key")); + + assertThat(e).hasMessageThat().isEqualTo("Map value cannot be null for key: key"); + assertThat(e.getErrorCode()).isEqualTo(CelErrorCode.INVALID_ARGUMENT); + } + + @Test + public void containsMapKey_boundKey_returnsTrue() { + Map map = new HashMap<>(); + map.put("key", "value"); + + assertThat(CelValueConverter.containsMapKey(map, "key")).isTrue(); + } + + @Test + public void containsMapKey_absentKey_returnsFalse() { + Map map = new HashMap<>(); + map.put("key", "value"); + + assertThat(CelValueConverter.containsMapKey(map, "absent")).isFalse(); + } + + @Test + public void containsMapKey_nullBoundKey_throws() { + Map map = new HashMap<>(); + map.put("key", null); + + CelInvalidArgumentException e = + assertThrows( + CelInvalidArgumentException.class, () -> CelValueConverter.containsMapKey(map, "key")); + + assertThat(e).hasMessageThat().isEqualTo("Map value cannot be null for key: key"); + } + + @Test + public void toRuntimeValue_listWithNullElement_throws() { + List list = Arrays.asList("a", null); + + CelInvalidArgumentException e = + assertThrows( + CelInvalidArgumentException.class, () -> CEL_VALUE_CONVERTER.toRuntimeValue(list)); + + assertThat(e).hasMessageThat().isEqualTo("List element cannot be null at index: 1"); + } + + @Test + public void toRuntimeValue_listWithNullElementAfterAdaptedElement_throws() { + // The first element normalizes (Integer -> Long), which diverts mapContainer onto its rebuild + // path. The illegal element is only reached by the tail loop. + List list = Arrays.asList(1, null); + + CelInvalidArgumentException e = + assertThrows( + CelInvalidArgumentException.class, () -> CEL_VALUE_CONVERTER.toRuntimeValue(list)); + + assertThat(e).hasMessageThat().isEqualTo("List element cannot be null at index: 1"); + } + + @Test + public void toRuntimeValue_nonRandomAccessCollectionWithNullElement_throws() { + List collection = new LinkedList<>(Arrays.asList("a", null)); + + CelInvalidArgumentException e = + assertThrows( + CelInvalidArgumentException.class, + () -> CEL_VALUE_CONVERTER.toRuntimeValue(collection)); + + assertThat(e).hasMessageThat().isEqualTo("List element cannot be null at index: 1"); + } + + @Test + public void maybeUnwrap_listWithNullElement_throws() { + // Previously returned the list with the illegal element intact, because the element mapped to + // itself and the zero-allocation path never rebuilt. + List list = Arrays.asList("a", null); + + CelInvalidArgumentException e = + assertThrows( + CelInvalidArgumentException.class, () -> CEL_VALUE_CONVERTER.maybeUnwrap(list)); + + assertThat(e).hasMessageThat().isEqualTo("List element cannot be null at index: 1"); + } }