diff --git a/common/src/main/java/dev/cel/common/internal/ProtoLiteAdapter.java b/common/src/main/java/dev/cel/common/internal/ProtoLiteAdapter.java index 3b13ecea1..8131d0eec 100644 --- a/common/src/main/java/dev/cel/common/internal/ProtoLiteAdapter.java +++ b/common/src/main/java/dev/cel/common/internal/ProtoLiteAdapter.java @@ -279,7 +279,8 @@ private Message adaptValueToUint32(Object value) { if (value instanceof Integer) { return UInt32Value.of((Integer) value); } - if (value instanceof Long) { + // Per the spec, an int isn't a uint, unless unsigned longs are disabled. + if (!celOptions.enableUnsignedLongs() && value instanceof Long) { try { return UInt32Value.of(unsignedIntCheckedCast((Long) value)); } catch (IllegalArgumentException e) { @@ -301,7 +302,7 @@ private Message adaptValueToUint64(Object value) { if (value instanceof Integer) { return UInt64Value.of(UnsignedInts.toLong((Integer) value)); } - if (value instanceof Long) { + if (!celOptions.enableUnsignedLongs() && value instanceof Long) { return UInt64Value.of((Long) value); } if (value instanceof UnsignedLong) { diff --git a/common/src/main/java/dev/cel/common/values/BaseProtoCelValueConverter.java b/common/src/main/java/dev/cel/common/values/BaseProtoCelValueConverter.java index 9fc218abe..7f7bcda1d 100644 --- a/common/src/main/java/dev/cel/common/values/BaseProtoCelValueConverter.java +++ b/common/src/main/java/dev/cel/common/values/BaseProtoCelValueConverter.java @@ -95,7 +95,8 @@ protected Object fromWellKnownProto(MessageLiteOrBuilder message, WellKnownProto case STRING_VALUE: return normalizePrimitive(((StringValue) message).getValue()); case UINT32_VALUE: - return UnsignedLong.valueOf(((UInt32Value) message).getValue()); + return UnsignedLong.fromLongBits( + Integer.toUnsignedLong(((UInt32Value) message).getValue())); case UINT64_VALUE: return UnsignedLong.fromLongBits(((UInt64Value) message).getValue()); case EMPTY: diff --git a/common/src/main/java/dev/cel/common/values/ProtoCelValueConverter.java b/common/src/main/java/dev/cel/common/values/ProtoCelValueConverter.java index a4280e1ea..67fae979f 100644 --- a/common/src/main/java/dev/cel/common/values/ProtoCelValueConverter.java +++ b/common/src/main/java/dev/cel/common/values/ProtoCelValueConverter.java @@ -15,6 +15,7 @@ package dev.cel.common.values; import com.google.common.base.Preconditions; +import com.google.common.collect.Lists; import com.google.common.primitives.UnsignedLong; import com.google.errorprone.annotations.Immutable; import com.google.protobuf.Any; @@ -130,6 +131,10 @@ public Object fromProtoMessageFieldToCelValue(Message message, FieldDescriptor f } if (fieldDescriptor.isMapField()) { + FieldDescriptor keyFieldDescriptor = + fieldDescriptor.getMessageType().findFieldByNumber(1); + FieldDescriptor valueFieldDescriptor = + fieldDescriptor.getMessageType().findFieldByNumber(2); Map map = new HashMap<>(); Object mapKey; Object mapValue; @@ -140,17 +145,14 @@ public Object fromProtoMessageFieldToCelValue(Message message, FieldDescriptor f mapValue = mapEntry.getValue(); } else if (entry instanceof DynamicMessage) { DynamicMessage dynamicMessage = (DynamicMessage) entry; - FieldDescriptor keyFieldDescriptor = - fieldDescriptor.getMessageType().findFieldByNumber(1); - FieldDescriptor valueFieldDescriptor = - fieldDescriptor.getMessageType().findFieldByNumber(2); mapKey = dynamicMessage.getField(keyFieldDescriptor); mapValue = dynamicMessage.getField(valueFieldDescriptor); } else { throw new IllegalStateException("Unexpected map field type: " + entry); } - map.put(mapKey, mapValue); + map.put( + toUnsigned(keyFieldDescriptor, mapKey), toUnsigned(valueFieldDescriptor, mapValue)); } return toRuntimeValue(map); } @@ -158,16 +160,13 @@ public Object fromProtoMessageFieldToCelValue(Message message, FieldDescriptor f return toRuntimeValue(result); case UINT32: case FIXED32: - if (!fieldDescriptor.isRepeated()) { - return UnsignedLong.valueOf((int) result); - } - break; case UINT64: case FIXED64: - if (!fieldDescriptor.isRepeated()) { - return UnsignedLong.fromLongBits((long) result); - } - break; + // Converts elements as they're read, so selecting one doesn't copy the whole list. + return fieldDescriptor.isRepeated() + ? CelPreAdaptedList.wrap( + Lists.transform((List) result, element -> toUnsigned(fieldDescriptor, element))) + : toUnsigned(fieldDescriptor, result); default: break; } @@ -187,6 +186,20 @@ public Object fromProtoMessageFieldToCelValue(Message message, FieldDescriptor f return toRuntimeValue(result); } + // Protobuf returns uint32 and uint64 values as signed ints and longs. + private static Object toUnsigned(FieldDescriptor fieldDescriptor, Object value) { + switch (fieldDescriptor.getType()) { + case UINT32: + case FIXED32: + return UnsignedLong.fromLongBits(Integer.toUnsignedLong((Integer) value)); + case UINT64: + case FIXED64: + return UnsignedLong.fromLongBits((Long) value); + default: + return value; + } + } + private ProtoCelValueConverter( CelDescriptorPool celDescriptorPool, DynamicProto dynamicProto, CelOptions celOptions) { Preconditions.checkNotNull(celDescriptorPool); diff --git a/common/src/test/java/dev/cel/common/internal/ProtoAdapterTest.java b/common/src/test/java/dev/cel/common/internal/ProtoAdapterTest.java index 91e0e22db..be7e4f042 100644 --- a/common/src/test/java/dev/cel/common/internal/ProtoAdapterTest.java +++ b/common/src/test/java/dev/cel/common/internal/ProtoAdapterTest.java @@ -36,6 +36,7 @@ import com.google.protobuf.StringValue; import com.google.protobuf.Struct; import com.google.protobuf.Timestamp; +import com.google.protobuf.UInt32Value; import com.google.protobuf.UInt64Value; import com.google.protobuf.Value; import com.google.type.Expr; @@ -213,6 +214,31 @@ public void adaptValueToProto_asymmetricFloatDoubleConversion() { .isEqualTo(DoubleValue.of(1.5D)); } + @Test + public void adaptValueToProto_signedUintWrappers_throws() { + ProtoAdapter protoAdapter = new ProtoAdapter(DYNAMIC_PROTO, CelOptions.DEFAULT); + String uint32TypeName = UInt32Value.getDescriptor().getFullName(); + String uint64TypeName = UInt64Value.getDescriptor().getFullName(); + + assertThrows( + IllegalArgumentException.class, () -> protoAdapter.adaptValueToProto(1L, uint32TypeName)); + assertThrows( + IllegalArgumentException.class, + () -> protoAdapter.adaptValueToProto(-1L, uint64TypeName)); + } + + @Test + @SuppressWarnings("deprecation") // Signed uints are only enabled through a deprecated option. + public void adaptValueToProto_unsignedLongsDisabled_acceptsSignedUintWrappers() { + ProtoAdapter protoAdapter = + new ProtoAdapter(DYNAMIC_PROTO, CelOptions.current().enableUnsignedLongs(false).build()); + + assertThat(protoAdapter.adaptValueToProto(1L, UInt32Value.getDescriptor().getFullName())) + .isEqualTo(UInt32Value.of(1)); + assertThat(protoAdapter.adaptValueToProto(1L, UInt64Value.getDescriptor().getFullName())) + .isEqualTo(UInt64Value.of(1)); + } + @Test public void adaptValueToProto_asymmetricJsonConversion() { ProtoAdapter protoAdapter = new ProtoAdapter(DYNAMIC_PROTO, CelOptions.DEFAULT); diff --git a/common/src/test/java/dev/cel/common/values/ProtoMessageValueTest.java b/common/src/test/java/dev/cel/common/values/ProtoMessageValueTest.java index 03dec6045..9f6cd5e84 100644 --- a/common/src/test/java/dev/cel/common/values/ProtoMessageValueTest.java +++ b/common/src/test/java/dev/cel/common/values/ProtoMessageValueTest.java @@ -191,7 +191,7 @@ private enum SelectFieldTestCase { BOOL("single_bool", true), INT32("single_int32", 4L), INT64("single_int64", 5L), - UINT32("single_uint32", UnsignedLong.valueOf(1L)), + UINT32("single_uint32", UnsignedLong.valueOf(0xFFFFFFFFL)), UINT64("single_uint64", UnsignedLong.MAX_VALUE), FLOAT("single_float", 1.5d), DOUBLE("single_double", 2.5d), @@ -203,14 +203,18 @@ private enum SelectFieldTestCase { TIMESTAMP("single_timestamp", Instant.ofEpochSecond(100)), INT32_WRAPPER("single_int32_wrapper", 5L), INT64_WRAPPER("single_int64_wrapper", 10L), - UINT32_WRAPPER("single_uint32_wrapper", UnsignedLong.valueOf(1L)), + UINT32_WRAPPER("single_uint32_wrapper", UnsignedLong.valueOf(0xFFFFFFFFL)), UINT64_WRAPPER("single_uint64_wrapper", UnsignedLong.MAX_VALUE), FLOAT_WRAPPER("single_float_wrapper", 7.5d), DOUBLE_WRAPPER("single_double_wrapper", 8.5d), STRING_WRAPPER("single_string_wrapper", "hello"), BYTES_WRAPPER("single_bytes_wrapper", CelByteString.of(new byte[] {0x02})), REPEATED_INT64("repeated_int64", ImmutableList.of(5L)), + REPEATED_UINT64("repeated_uint64", ImmutableList.of(UnsignedLong.MAX_VALUE)), MAP_STRING_STRING("map_string_string", ImmutableMap.of("a", "b")), + MAP_UINT32_UINT64( + "map_uint32_uint64", + ImmutableMap.of(UnsignedLong.valueOf(0xFFFFFFFFL), UnsignedLong.MAX_VALUE)), NESTED_MESSAGE( "standalone_message", ProtoMessageValue.create( @@ -653,7 +657,7 @@ private static TestAllTypes createPopulatedTestAllTypes() { .setSingleBool(true) .setSingleInt32(4) .setSingleInt64(5L) - .setSingleUint32(1) + .setSingleUint32(0xFFFFFFFF) .setSingleUint64(UnsignedLong.MAX_VALUE.longValue()) .setSingleFloat(1.5f) .setSingleDouble(2.5d) @@ -664,14 +668,16 @@ private static TestAllTypes createPopulatedTestAllTypes() { .setSingleTimestamp(Timestamp.newBuilder().setSeconds(100)) .setSingleInt32Wrapper(Int32Value.of(5)) .setSingleInt64Wrapper(Int64Value.of(10L)) - .setSingleUint32Wrapper(UInt32Value.of(1)) + .setSingleUint32Wrapper(UInt32Value.of(0xFFFFFFFF)) .setSingleUint64Wrapper(UInt64Value.of(UnsignedLong.MAX_VALUE.longValue())) .setSingleStringWrapper(StringValue.of("hello")) .setSingleFloatWrapper(FloatValue.of(7.5f)) .setSingleDoubleWrapper(DoubleValue.of(8.5d)) .setSingleBytesWrapper(BytesValue.of(ByteString.copyFrom(new byte[] {0x02}))) .addRepeatedInt64(5L) + .addRepeatedUint64(UnsignedLong.MAX_VALUE.longValue()) .putMapStringString("a", "b") + .putMapUint32Uint64(0xFFFFFFFF, UnsignedLong.MAX_VALUE.longValue()) .setStandaloneMessage(NestedMessage.getDefaultInstance()) .setStandaloneEnum(NestedEnum.BAR) .build(); diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateStruct.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateStruct.java index a2e8a9da6..e566edba0 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateStruct.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateStruct.java @@ -46,7 +46,8 @@ final class EvalCreateStruct extends PlannedInterpretable { @Override Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) { - Map fieldValues = Maps.newHashMapWithExpectedSize(keys.length); + // In source order, so that the last of several fields set in one oneof wins, as in protobuf. + Map fieldValues = Maps.newLinkedHashMapWithExpectedSize(keys.length); AccumulatedUnknowns unknowns = null; for (int i = 0; i < keys.length; i++) { Object value = EvalHelpers.evalStrictly(values[i], resolver, frame); diff --git a/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java b/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java index dd225229b..24160aef1 100644 --- a/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java +++ b/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java @@ -692,6 +692,23 @@ public void plan_createStruct_withContainer() throws Exception { assertThat(result).isEqualTo(TestAllTypes.getDefaultInstance()); } + // With fewer fields, the oneof fields would share a HashMap bucket and keep their order. + @Test + @TestParameters({ + "{expression: 'TestAllTypes{oneof_bool: true, single_int32: 1, single_int64: 2," + + " oneof_msg: TestAllTypes.NestedMessage{}}', expectedCase: ONEOF_MSG}", + "{expression: 'TestAllTypes{oneof_msg: TestAllTypes.NestedMessage{}, single_int32: 1," + + " single_int64: 2, oneof_bool: true}', expectedCase: ONEOF_BOOL}" + }) + public void plan_createStruct_oneofSetTwice_lastFieldWins( + String expression, TestAllTypes.KindCase expectedCase) throws Exception { + CelAbstractSyntaxTree ast = compile(expression); + + TestAllTypes result = (TestAllTypes) PLANNER.plan(ast).eval(); + + assertThat(result.getKindCase()).isEqualTo(expectedCase); + } + @Test public void plan_call_zeroArgs() throws Exception { CelAbstractSyntaxTree ast = compile("zero()");