Skip to content

Commit e91a59c

Browse files
l46kokcopybara-github
authored andcommitted
Reject Java nulls uniformly in map and list adaptation
PiperOrigin-RevId: 976836703
1 parent b4977d9 commit e91a59c

17 files changed

Lines changed: 2554 additions & 215 deletions

common/src/main/java/dev/cel/common/values/BUILD.bazel

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ java_library(
167167
":preadapted_list",
168168
"//:auto_value",
169169
"//common/annotations",
170+
"//common/exceptions:invalid_argument",
170171
"//common/types",
171172
"//common/types:type_providers",
172173
"@maven//:com_google_errorprone_error_prone_annotations",
@@ -218,6 +219,7 @@ cel_android_library(
218219
":preadapted_list_android",
219220
"//:auto_value",
220221
"//common/annotations",
222+
"//common/exceptions:invalid_argument",
221223
"//common/types:type_providers_android",
222224
"//common/types:types_android",
223225
"@maven//:com_google_errorprone_error_prone_annotations",
@@ -323,6 +325,8 @@ java_library(
323325
],
324326
deps = [
325327
":base_proto_cel_value_converter",
328+
":optimized_selectable",
329+
":select_field",
326330
":values",
327331
"//:auto_value",
328332
"//common/annotations",
@@ -351,6 +355,8 @@ cel_android_library(
351355
],
352356
deps = [
353357
":base_proto_cel_value_converter_android",
358+
":optimized_selectable_android",
359+
":select_field_android",
354360
":values_android",
355361
"//:auto_value",
356362
"//common/annotations",
@@ -434,3 +440,85 @@ cel_android_library(
434440
"@maven//:com_google_errorprone_error_prone_annotations",
435441
],
436442
)
443+
444+
java_library(
445+
name = "select_field",
446+
srcs = ["SelectField.java"],
447+
tags = [
448+
],
449+
deps = [
450+
"//:auto_value",
451+
"//common/annotations",
452+
"@maven//:com_google_errorprone_error_prone_annotations",
453+
"@maven//:com_google_guava_guava",
454+
"@maven//:org_jspecify_jspecify",
455+
],
456+
)
457+
458+
cel_android_library(
459+
name = "select_field_android",
460+
srcs = ["SelectField.java"],
461+
tags = [
462+
],
463+
deps = [
464+
"//:auto_value",
465+
"//common/annotations",
466+
"@maven//:com_google_errorprone_error_prone_annotations",
467+
"@maven//:org_jspecify_jspecify",
468+
"@maven_android//:com_google_guava_guava",
469+
],
470+
)
471+
472+
java_library(
473+
name = "optimized_selectable",
474+
srcs = ["OptimizedSelectable.java"],
475+
tags = [
476+
],
477+
deps = [
478+
":select_field",
479+
"//common/annotations",
480+
"@maven//:com_google_errorprone_error_prone_annotations",
481+
],
482+
)
483+
484+
cel_android_library(
485+
name = "optimized_selectable_android",
486+
srcs = ["OptimizedSelectable.java"],
487+
tags = [
488+
],
489+
deps = [
490+
":select_field_android",
491+
"//common/annotations",
492+
"@maven//:com_google_errorprone_error_prone_annotations",
493+
],
494+
)
495+
496+
java_library(
497+
name = "optimized_select_traversal",
498+
srcs = ["OptimizedSelectTraversal.java"],
499+
tags = [
500+
],
501+
deps = [
502+
":optimized_selectable",
503+
":select_field",
504+
":values",
505+
"//common/annotations",
506+
"//common/exceptions:attribute_not_found",
507+
"@maven//:com_google_guava_guava",
508+
],
509+
)
510+
511+
cel_android_library(
512+
name = "optimized_select_traversal_android",
513+
srcs = ["OptimizedSelectTraversal.java"],
514+
tags = [
515+
],
516+
deps = [
517+
":optimized_selectable_android",
518+
":select_field_android",
519+
":values_android",
520+
"//common/annotations",
521+
"//common/exceptions:attribute_not_found",
522+
"@maven_android//:com_google_guava_guava",
523+
],
524+
)

common/src/main/java/dev/cel/common/values/CelValueConverter.java

Lines changed: 90 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,18 @@
1717
import com.google.common.base.Preconditions;
1818
import com.google.common.collect.ImmutableList;
1919
import com.google.common.collect.ImmutableMap;
20+
import com.google.errorprone.annotations.CanIgnoreReturnValue;
2021
import com.google.errorprone.annotations.Immutable;
2122
import dev.cel.common.annotations.Internal;
23+
import dev.cel.common.exceptions.CelInvalidArgumentException;
2224
import java.util.Collection;
2325
import java.util.Iterator;
2426
import java.util.List;
2527
import java.util.Map;
2628
import java.util.Optional;
2729
import java.util.RandomAccess;
2830
import java.util.function.Function;
31+
import org.jspecify.annotations.Nullable;
2932

3033
/**
3134
* {@code CelValueConverter} handles bidirectional conversion between native Java objects to {@link
@@ -74,7 +77,7 @@ protected Object mapContainer(Object value, Function<Object, Object> mapper) {
7477
if (value instanceof List && value instanceof RandomAccess) {
7578
List<Object> list = (List<Object>) value;
7679
for (int i = 0; i < list.size(); i++) {
77-
Object element = list.get(i);
80+
Object element = checkListElement(list.get(i), i);
7881
Object mapped = mapper.apply(element);
7982

8083
if (mapped != element) {
@@ -85,7 +88,7 @@ protected Object mapContainer(Object value, Function<Object, Object> mapper) {
8588
}
8689
builder.add(mapped);
8790
for (int j = i + 1; j < list.size(); j++) {
88-
builder.add(mapper.apply(list.get(j)));
91+
builder.add(mapper.apply(checkListElement(list.get(j), j)));
8992
}
9093
return builder.build();
9194
}
@@ -100,8 +103,9 @@ protected Object mapContainer(Object value, Function<Object, Object> mapper) {
100103
Collection<Object> collection = (Collection<Object>) value;
101104
ImmutableList.Builder<Object> builder =
102105
ImmutableList.builderWithExpectedSize(collection.size());
106+
int index = 0;
103107
for (Object element : collection) {
104-
builder.add(mapper.apply(element));
108+
builder.add(mapper.apply(checkListElement(element, index++)));
105109
}
106110
return builder.build();
107111
}
@@ -112,6 +116,7 @@ protected Object mapContainer(Object value, Function<Object, Object> mapper) {
112116

113117
while (iterator.hasNext()) {
114118
Map.Entry<Object, Object> entry = iterator.next();
119+
checkMapEntry(entry);
115120
Object mappedKey = mapper.apply(entry.getKey());
116121
Object mappedValue = mapper.apply(entry.getValue());
117122

@@ -128,6 +133,7 @@ protected Object mapContainer(Object value, Function<Object, Object> mapper) {
128133
builder.put(mappedKey, mappedValue);
129134
while (iterator.hasNext()) {
130135
Map.Entry<Object, Object> nextEntry = iterator.next();
136+
checkMapEntry(nextEntry);
131137
builder.put(mapper.apply(nextEntry.getKey()), mapper.apply(nextEntry.getValue()));
132138
}
133139
return builder.buildOrThrow();
@@ -162,6 +168,57 @@ public Object toRuntimeValue(Object value) {
162168
return normalizePrimitive(value);
163169
}
164170

171+
/**
172+
* Adapts {@code value} for an intermediate field selection hop.
173+
*
174+
* <p>{@link Map} instances are returned as-is to avoid O(N) whole-map normalization per hop; the
175+
* accessed entry is validated on lookup via {@link #findMapValue} or {@link #containsMapKey}.
176+
* Callers materializing a final evaluation result must use {@link #toRuntimeValue} instead.
177+
*/
178+
public final Object toTraversalTarget(Object value) {
179+
if (value instanceof Map) {
180+
return value;
181+
}
182+
183+
return toRuntimeValue(value);
184+
}
185+
186+
/**
187+
* Returns the unadapted value bound to {@code key} in {@code map}, or {@link Optional#empty()} if
188+
* absent.
189+
*
190+
* @throws CelInvalidArgumentException if {@code key} is bound to {@code null}.
191+
*/
192+
public static Optional<Object> findMapValue(Map<?, ?> map, Object key) {
193+
Object value = map.get(key);
194+
if (value != null) {
195+
return Optional.of(value);
196+
}
197+
198+
if (map.containsKey(key)) {
199+
throw nullMapValue(key);
200+
}
201+
202+
return Optional.empty();
203+
}
204+
205+
/**
206+
* Returns whether {@code key} is present in {@code map}.
207+
*
208+
* @throws CelInvalidArgumentException if {@code key} is bound to {@code null}.
209+
*/
210+
public static boolean containsMapKey(Map<?, ?> map, Object key) {
211+
if (map.get(key) != null) {
212+
return true;
213+
}
214+
215+
if (map.containsKey(key)) {
216+
throw nullMapValue(key);
217+
}
218+
219+
return false;
220+
}
221+
165222
protected Object normalizePrimitive(Object value) {
166223
Preconditions.checkNotNull(value);
167224

@@ -196,6 +253,36 @@ private Object unwrap(CelValue celValue) {
196253
return celValue.value();
197254
}
198255

256+
private static void checkMapEntry(Map.Entry<?, ?> entry) {
257+
Object key = entry.getKey();
258+
if (key == null) {
259+
throw new CelInvalidArgumentException("Map key cannot be null.");
260+
}
261+
262+
if (entry.getValue() == null) {
263+
throw nullMapValue(key);
264+
}
265+
}
266+
267+
@CanIgnoreReturnValue
268+
private static Object checkListElement(@Nullable Object element, int index) {
269+
if (element == null) {
270+
throw new CelInvalidArgumentException(
271+
String.format("List element cannot be null at index: %d", index));
272+
}
273+
274+
return element;
275+
}
276+
277+
/**
278+
* Uses {@code INVALID_ARGUMENT} rather than {@code ATTRIBUTE_NOT_FOUND} so presence tests and
279+
* optional selects do not treat a null-bound key as absent.
280+
*/
281+
private static CelInvalidArgumentException nullMapValue(Object key) {
282+
return new CelInvalidArgumentException(
283+
String.format("Map value cannot be null for key: %s", key));
284+
}
285+
199286
protected CelValueConverter() {
200287
this.maybeUnwrapFunction = this::maybeUnwrap;
201288
this.toRuntimeValueFunction = this::toRuntimeValue;

common/src/main/java/dev/cel/common/values/MutableMapValue.java

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -105,23 +105,13 @@ public Set<Entry<Object, Object>> entrySet() {
105105

106106
@Override
107107
public Object select(Object field) {
108-
Object val = internalMap.get(field);
109-
if (val != null) {
110-
return val;
111-
}
112-
if (!internalMap.containsKey(field)) {
113-
throw CelAttributeNotFoundException.forMissingMapKey(field.toString());
114-
}
115-
throw CelAttributeNotFoundException.of(
116-
String.format("Map value cannot be null for key: %s", field));
108+
return CelValueConverter.findMapValue(internalMap, field)
109+
.orElseThrow(() -> CelAttributeNotFoundException.forMissingMapKey(field.toString()));
117110
}
118111

119112
@Override
120113
public Optional<?> find(Object field) {
121-
if (internalMap.containsKey(field)) {
122-
return Optional.ofNullable(internalMap.get(field));
123-
}
124-
return Optional.empty();
114+
return CelValueConverter.findMapValue(internalMap, field);
125115
}
126116

127117
@Override

0 commit comments

Comments
 (0)