Skip to content

Commit 70e91d0

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Fix null assignment to fields
PiperOrigin-RevId: 875509705
1 parent 0737691 commit 70e91d0

18 files changed

Lines changed: 204 additions & 113 deletions

File tree

‎common/src/main/java/dev/cel/common/exceptions/CelInvalidArgumentException.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,8 @@ public final class CelInvalidArgumentException extends CelRuntimeException {
2424
public CelInvalidArgumentException(Throwable cause) {
2525
super(cause, CelErrorCode.INVALID_ARGUMENT);
2626
}
27+
28+
public CelInvalidArgumentException(String message) {
29+
super(message, CelErrorCode.INVALID_ARGUMENT);
30+
}
2731
}

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

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,26 @@ public Optional<Object> adaptFieldToValue(FieldDescriptor fieldDescriptor, Objec
204204
@SuppressWarnings({"unchecked", "rawtypes"})
205205
public Optional<Object> adaptValueToFieldType(
206206
FieldDescriptor fieldDescriptor, Object fieldValue) {
207-
if (isWrapperType(fieldDescriptor) && fieldValue.equals(NullValue.NULL_VALUE)) {
208-
return Optional.empty();
207+
if (fieldValue instanceof NullValue) {
208+
// `null` literal can only be assigned to singular messages. Explicitly check
209+
// for non-allowed cases (collections, maps)
210+
if (fieldDescriptor.isMapField()
211+
|| fieldDescriptor.isRepeated()
212+
|| fieldDescriptor.getJavaType() != FieldDescriptor.JavaType.MESSAGE
213+
|| WellKnownProto.JSON_STRUCT_VALUE
214+
.typeName()
215+
.equals(fieldDescriptor.getMessageType().getFullName())
216+
|| WellKnownProto.JSON_LIST_VALUE
217+
.typeName()
218+
.equals(fieldDescriptor.getMessageType().getFullName())) {
219+
throw new IllegalArgumentException("Unsupported field type");
220+
}
221+
222+
String typeFullName = fieldDescriptor.getMessageType().getFullName();
223+
if (!WellKnownProto.ANY_VALUE.typeName().equals(typeFullName)
224+
&& !WellKnownProto.JSON_VALUE.typeName().equals(typeFullName)) {
225+
return Optional.empty();
226+
}
209227
}
210228
if (fieldDescriptor.isMapField()) {
211229
Descriptor entryDescriptor = fieldDescriptor.getMessageType();
@@ -370,14 +388,6 @@ private static String typeName(Descriptor protoType) {
370388
return protoType.getFullName();
371389
}
372390

373-
private static boolean isWrapperType(FieldDescriptor fieldDescriptor) {
374-
if (fieldDescriptor.getJavaType() != FieldDescriptor.JavaType.MESSAGE) {
375-
return false;
376-
}
377-
String fieldTypeName = fieldDescriptor.getMessageType().getFullName();
378-
return WellKnownProto.isWrapperType(fieldTypeName);
379-
}
380-
381391
private static int intCheckedCast(long value) {
382392
try {
383393
return Ints.checkedCast(value);

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

Lines changed: 46 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,24 +41,39 @@ public static CelValueConverter getDefaultInstance() {
4141
return DEFAULT_INSTANCE;
4242
}
4343

44-
/** Adapts a {@link CelValue} to a plain old Java Object. */
45-
public Object unwrap(CelValue celValue) {
46-
Preconditions.checkNotNull(celValue);
44+
/**
45+
* Unwraps the {@code value} into its plain old Java Object representation.
46+
*
47+
* <p>The value may be a {@link CelValue}, a {@link Collection} or a {@link Map}.
48+
*/
49+
public Object maybeUnwrap(Object value) {
50+
if (value instanceof CelValue) {
51+
return unwrap((CelValue) value);
52+
}
4753

48-
if (celValue instanceof OptionalValue) {
49-
OptionalValue<Object, Object> optionalValue = (OptionalValue<Object, Object>) celValue;
50-
if (optionalValue.isZeroValue()) {
51-
return Optional.empty();
54+
if (value instanceof Collection) {
55+
Collection<Object> collection = (Collection<Object>) value;
56+
ImmutableList.Builder<Object> builder =
57+
ImmutableList.builderWithExpectedSize(collection.size());
58+
for (Object element : collection) {
59+
builder.add(maybeUnwrap(element));
5260
}
5361

54-
return Optional.of(optionalValue.value());
62+
return builder.build();
5563
}
5664

57-
if (celValue instanceof ErrorValue) {
58-
return celValue;
65+
if (value instanceof Map) {
66+
Map<Object, Object> map = (Map<Object, Object>) value;
67+
ImmutableMap.Builder<Object, Object> builder =
68+
ImmutableMap.builderWithExpectedSize(map.size());
69+
for (Map.Entry<Object, Object> entry : map.entrySet()) {
70+
builder.put(maybeUnwrap(entry.getKey()), maybeUnwrap(entry.getValue()));
71+
}
72+
73+
return builder.buildOrThrow();
5974
}
6075

61-
return celValue.value();
76+
return value;
6277
}
6378

6479
/**
@@ -101,6 +116,26 @@ protected Object normalizePrimitive(Object value) {
101116
return value;
102117
}
103118

119+
/** Adapts a {@link CelValue} to a plain old Java Object. */
120+
private static Object unwrap(CelValue celValue) {
121+
Preconditions.checkNotNull(celValue);
122+
123+
if (celValue instanceof OptionalValue) {
124+
OptionalValue<Object, Object> optionalValue = (OptionalValue<Object, Object>) celValue;
125+
if (optionalValue.isZeroValue()) {
126+
return Optional.empty();
127+
}
128+
129+
return Optional.of(optionalValue.value());
130+
}
131+
132+
if (celValue instanceof ErrorValue) {
133+
return celValue;
134+
}
135+
136+
return celValue.value();
137+
}
138+
104139
private ImmutableList<Object> toListValue(Collection<Object> iterable) {
105140
Preconditions.checkNotNull(iterable);
106141

‎common/src/main/java/dev/cel/common/values/ProtoCelValueConverter.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ protected Object fromWellKnownProto(MessageLiteOrBuilder msg, WellKnownProto wel
6767
try {
6868
unpackedMessage = dynamicProto.unpack((Any) message);
6969
} catch (InvalidProtocolBufferException e) {
70-
throw new IllegalStateException(
70+
throw new IllegalArgumentException(
7171
"Unpacking failed for message: " + message.getDescriptorForType().getFullName(), e);
7272
}
7373
return toRuntimeValue(unpackedMessage);

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

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,7 @@ public static List<Object[]> data() {
150150
@Test
151151
public void adaptValueToProto_bidirectionalConversion() {
152152
DynamicProto dynamicProto = DynamicProto.create(DefaultMessageFactory.INSTANCE);
153-
ProtoAdapter protoAdapter =
154-
new ProtoAdapter(
155-
dynamicProto,
156-
CelOptions.current().build());
153+
ProtoAdapter protoAdapter = new ProtoAdapter(dynamicProto, CelOptions.current().build());
157154
assertThat(protoAdapter.adaptValueToProto(value, proto.getDescriptorForType().getFullName()))
158155
.isEqualTo(proto);
159156
assertThat(protoAdapter.adaptProtoToValue(proto)).isEqualTo(value);
@@ -181,6 +178,18 @@ public void adaptAnyValue_hermeticTypes_bidirectionalConversion() {
181178

182179
@RunWith(JUnit4.class)
183180
public static class AsymmetricConversionTest {
181+
182+
@Test
183+
public void unpackAny_celNullValue() throws Exception {
184+
ProtoAdapter protoAdapter = new ProtoAdapter(DYNAMIC_PROTO, CelOptions.DEFAULT);
185+
Any any =
186+
(Any)
187+
protoAdapter.adaptValueToProto(
188+
dev.cel.common.values.NullValue.NULL_VALUE, "google.protobuf.Any");
189+
Object unpacked = protoAdapter.adaptProtoToValue(any);
190+
assertThat(unpacked).isEqualTo(dev.cel.common.values.NullValue.NULL_VALUE);
191+
}
192+
184193
@Test
185194
public void adaptValueToProto_asymmetricFloatConversion() {
186195
ProtoAdapter protoAdapter = new ProtoAdapter(DYNAMIC_PROTO, CelOptions.DEFAULT);

‎common/src/test/java/dev/cel/common/values/CelValueConverterTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,15 +37,16 @@ public void toRuntimeValue_optionalValue() {
3737
@Test
3838
@SuppressWarnings("unchecked") // Test only
3939
public void unwrap_optionalValue() {
40-
Optional<Long> result = (Optional<Long>) CEL_VALUE_CONVERTER.unwrap(OptionalValue.create(2L));
40+
Optional<Long> result =
41+
(Optional<Long>) CEL_VALUE_CONVERTER.maybeUnwrap(OptionalValue.create(2L));
4142

4243
assertThat(result).isEqualTo(Optional.of(2L));
4344
}
4445

4546
@Test
4647
@SuppressWarnings("unchecked") // Test only
4748
public void unwrap_emptyOptionalValue() {
48-
Optional<Long> result = (Optional<Long>) CEL_VALUE_CONVERTER.unwrap(OptionalValue.EMPTY);
49+
Optional<Long> result = (Optional<Long>) CEL_VALUE_CONVERTER.maybeUnwrap(OptionalValue.EMPTY);
4950

5051
assertThat(result).isEqualTo(Optional.empty());
5152
}

‎common/src/test/java/dev/cel/common/values/ProtoCelValueConverterTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public class ProtoCelValueConverterTest {
3535

3636
@Test
3737
public void unwrap_nullValue() {
38-
NullValue nullValue = (NullValue) PROTO_CEL_VALUE_CONVERTER.unwrap(NullValue.NULL_VALUE);
38+
NullValue nullValue = (NullValue) PROTO_CEL_VALUE_CONVERTER.maybeUnwrap(NullValue.NULL_VALUE);
3939

4040
// Note: No conversion is attempted. We're using dev.cel.common.values.NullValue.NULL_VALUE as
4141
// the

‎conformance/src/test/java/dev/cel/conformance/BUILD.bazel‎

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -124,14 +124,6 @@ _TESTS_TO_SKIP_LEGACY = [
124124
"string_ext/format",
125125
"string_ext/format_errors",
126126

127-
# TODO: Fix null assignment to a field
128-
"proto2/set_null/single_message",
129-
"proto2/set_null/single_duration",
130-
"proto2/set_null/single_timestamp",
131-
"proto3/set_null/single_message",
132-
"proto3/set_null/single_duration",
133-
"proto3/set_null/single_timestamp",
134-
135127
# Future features for CEL 1.0
136128
# TODO: Strong typing support for enums, specified but not implemented.
137129
"enums/strong_proto2",
@@ -162,7 +154,6 @@ _TESTS_TO_SKIP_PLANNER = [
162154
"string_ext/format_errors",
163155

164156
# TODO: Check behavior for go/cpp
165-
"basic/functions/unbound",
166157
"basic/functions/unbound_is_runtime_error",
167158

168159
# TODO: Ensure overflow occurs on conversions of double values which might not work properly on all platforms.
@@ -175,26 +166,7 @@ _TESTS_TO_SKIP_PLANNER = [
175166
"timestamps/timestamp_range/sub_time_duration_under",
176167

177168
# Skip until fixed.
178-
"fields/qualified_identifier_resolution/map_key_float",
179-
"fields/qualified_identifier_resolution/map_key_null",
180-
"fields/qualified_identifier_resolution/map_value_repeat_key_heterogeneous",
181-
"optionals/optionals/map_null_entry_no_such_key",
182-
"optionals/optionals/map_present_key_invalid_field",
183169
"parse/receiver_function_names",
184-
"proto2/extensions_get/package_scoped_test_all_types_ext",
185-
"proto2/extensions_get/package_scoped_repeated_test_all_types",
186-
"proto2/extensions_get/message_scoped_nested_ext",
187-
"proto2/extensions_get/message_scoped_repeated_test_all_types",
188-
"proto2_ext/get_ext/package_scoped_repeated_test_all_types",
189-
"proto2_ext/get_ext/message_scoped_repeated_test_all_types",
190-
191-
# TODO: Fix null assignment to a field
192-
"proto2/set_null/single_message",
193-
"proto2/set_null/single_duration",
194-
"proto2/set_null/single_timestamp",
195-
"proto3/set_null/single_message",
196-
"proto3/set_null/single_duration",
197-
"proto3/set_null/single_timestamp",
198170

199171
# Type inference edgecases around null(able) assignability.
200172
# These type check, but resolve to a different type.

‎conformance/src/test/java/dev/cel/conformance/ConformanceTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,10 +210,10 @@ public void evaluate() throws Throwable {
210210
}
211211

212212
CelRuntime runtime = getRuntime(test, usePlanner);
213-
Program program = runtime.createProgram(response.getAst());
214213
ExprValue result = null;
215214
CelEvaluationException error = null;
216215
try {
216+
Program program = runtime.createProgram(response.getAst());
217217
result = toExprValue(program.eval(getBindings(test)), response.getAst().getResultType());
218218
} catch (CelEvaluationException e) {
219219
error = e;

‎extensions/src/test/java/dev/cel/extensions/CelProtoExtensionsTest.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
import dev.cel.runtime.CelFunctionBinding;
4747
import dev.cel.runtime.CelRuntime;
4848
import dev.cel.runtime.CelRuntimeFactory;
49+
import dev.cel.runtime.CelRuntimeImpl;
4950
import org.junit.Test;
5051
import org.junit.runner.RunWith;
5152

@@ -60,9 +61,14 @@ public final class CelProtoExtensionsTest {
6061
.addVar("msg", StructTypeReference.create("cel.expr.conformance.proto2.TestAllTypes"))
6162
.setContainer(CelContainer.ofName("cel.expr.conformance.proto2"))
6263
.build();
63-
6464
private static final CelRuntime CEL_RUNTIME =
65-
CelRuntimeFactory.standardCelRuntimeBuilder()
65+
CelRuntimeImpl.newBuilder()
66+
.setOptions(
67+
CelOptions.current()
68+
.enableTimestampEpoch(true)
69+
.enableHeterogeneousNumericComparisons(true)
70+
.build())
71+
// CEL-Internal-2
6672
.addFileTypes(TestAllTypesExtensions.getDescriptor())
6773
.build();
6874

0 commit comments

Comments
 (0)