diff --git a/core/src/jmh/java/org/projectnessie/cel/interpreter/AttributesBench.java b/core/src/jmh/java/org/projectnessie/cel/interpreter/AttributesBench.java index 6e112ed2a..a4c965e37 100644 --- a/core/src/jmh/java/org/projectnessie/cel/interpreter/AttributesBench.java +++ b/core/src/jmh/java/org/projectnessie/cel/interpreter/AttributesBench.java @@ -42,6 +42,7 @@ import org.openjdk.jmh.annotations.Threads; import org.openjdk.jmh.annotations.Warmup; import org.projectnessie.cel.common.containers.Container; +import org.projectnessie.cel.common.types.ref.FieldType; import org.projectnessie.cel.common.types.ref.TypeRegistry; import org.projectnessie.cel.interpreter.AttributeFactory.Attribute; import org.projectnessie.cel.interpreter.AttributeFactory.NamespacedAttribute; @@ -56,6 +57,24 @@ @OutputTimeUnit(TimeUnit.MICROSECONDS) public class AttributesBench { + @State(Scope.Benchmark) + public static class GeneratedScalarFieldGetterState { + + final NestedMessage message; + final FieldType fieldType; + + public GeneratedScalarFieldGetterState() { + message = NestedMessage.newBuilder().setBb(123).build(); + TypeRegistry reg = newRegistry(message); + fieldType = reg.findFieldType("cel.expr.conformance.proto3.TestAllTypes.NestedMessage", "bb"); + } + } + + @Benchmark + public Object generatedScalarFieldGetter(GeneratedScalarFieldGetterState state) { + return state.fieldType.getFrom.getFrom(state.message); + } + @Benchmark public void attributesConditionalAttr_TrueBranch() { TypeRegistry reg = newRegistry(); diff --git a/core/src/main/java/org/projectnessie/cel/common/types/pb/GeneratedFieldAccessor.java b/core/src/main/java/org/projectnessie/cel/common/types/pb/GeneratedFieldAccessor.java new file mode 100644 index 000000000..f65cde96f --- /dev/null +++ b/core/src/main/java/org/projectnessie/cel/common/types/pb/GeneratedFieldAccessor.java @@ -0,0 +1,130 @@ +/* + * Copyright (C) 2021 The Authors of CEL-Java + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.cel.common.types.pb; + +import com.google.protobuf.ByteString; +import com.google.protobuf.Descriptors.FieldDescriptor; +import com.google.protobuf.DynamicMessage; +import com.google.protobuf.Message; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; +import org.projectnessie.cel.common.types.ref.FieldGetter; + +final class GeneratedFieldAccessor { + + private static final MethodType OBJECT_GETTER = MethodType.methodType(Object.class, Object.class); + + private GeneratedFieldAccessor() {} + + static FieldGetter create(PbTypeDescription type, FieldDescription field) { + FieldDescriptor descriptor = field.descriptor(); + Class returnType = generatedReturnType(descriptor); + Message zero = type.zero(); + Class messageClass = type.reflectType(); + if (returnType == null + || descriptor.isExtension() + || descriptor.isRepeated() + || zero instanceof DynamicMessage + || !messageClass.isInstance(zero)) { + return null; + } + + String suffix = accessorSuffix(descriptor.getName()); + for (String methodName : new String[] {"get" + suffix, "get" + suffix + '_'}) { + try { + MethodHandle getter = + MethodHandles.publicLookup() + .findVirtual(messageClass, methodName, MethodType.methodType(returnType)); + if (!matchesField(getter, zero, descriptor)) { + continue; + } + MethodHandle objectGetter = getter.asType(OBJECT_GETTER); + return target -> invoke(objectGetter, target); + } catch (IllegalAccessException | NoSuchMethodException | RuntimeException e) { + // Generated-name collisions and inaccessible generated classes use descriptor access. + } + } + return null; + } + + private static Class generatedReturnType(FieldDescriptor descriptor) { + return switch (descriptor.getType()) { + case BOOL -> boolean.class; + case STRING -> String.class; + case BYTES -> ByteString.class; + case INT32, SINT32, SFIXED32 -> int.class; + case INT64, SINT64, SFIXED64 -> long.class; + case FLOAT -> float.class; + case DOUBLE -> double.class; + default -> null; + }; + } + + private static boolean matchesField( + MethodHandle getter, Message zero, FieldDescriptor descriptor) { + Object expected = validationValue(descriptor); + Message probe = zero.newBuilderForType().setField(descriptor, expected).buildPartial(); + try { + return expected.equals(getter.invoke(probe)); + } catch (Throwable e) { + return false; + } + } + + private static Object validationValue(FieldDescriptor descriptor) { + return switch (descriptor.getType()) { + case BOOL -> true; + case STRING -> "cel_generated_field_accessor"; + case BYTES -> ByteString.copyFromUtf8("cel_generated_field_accessor"); + case INT32, SINT32, SFIXED32 -> 0x51a7; + case INT64, SINT64, SFIXED64 -> 0x51a7_19b3_42c5L; + case FLOAT -> 123.25f; + case DOUBLE -> 123.25d; + default -> + throw new IllegalArgumentException("Unsupported field type " + descriptor.getType()); + }; + } + + private static String accessorSuffix(String fieldName) { + StringBuilder result = new StringBuilder(fieldName.length()); + boolean capitalizeNext = true; + for (int i = 0; i < fieldName.length(); i++) { + char c = fieldName.charAt(i); + if (c >= 'a' && c <= 'z') { + result.append(capitalizeNext ? Character.toUpperCase(c) : c); + capitalizeNext = false; + } else if ((c >= 'A' && c <= 'Z')) { + result.append(c); + capitalizeNext = false; + } else if (c >= '0' && c <= '9') { + result.append(c); + capitalizeNext = true; + } else { + capitalizeNext = true; + } + } + return result.toString(); + } + + private static Object invoke(MethodHandle getter, Object target) { + try { + return (Object) getter.invokeExact(target); + } catch (Throwable e) { + throw new RuntimeException(e); + } + } +} diff --git a/core/src/main/java/org/projectnessie/cel/common/types/pb/ProtoTypeRegistry.java b/core/src/main/java/org/projectnessie/cel/common/types/pb/ProtoTypeRegistry.java index 964142599..1f007f902 100644 --- a/core/src/main/java/org/projectnessie/cel/common/types/pb/ProtoTypeRegistry.java +++ b/core/src/main/java/org/projectnessie/cel/common/types/pb/ProtoTypeRegistry.java @@ -16,6 +16,7 @@ package org.projectnessie.cel.common.types.pb; import static org.projectnessie.cel.common.types.BoolT.BoolType; +import static org.projectnessie.cel.common.types.BoolT.True; import static org.projectnessie.cel.common.types.BytesT.BytesType; import static org.projectnessie.cel.common.types.DoubleT.DoubleType; import static org.projectnessie.cel.common.types.DurationT.DurationType; @@ -45,6 +46,7 @@ import com.google.api.expr.v1alpha1.Type; import com.google.protobuf.Any; import com.google.protobuf.BoolValue; +import com.google.protobuf.ByteString; import com.google.protobuf.BytesValue; import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Descriptors.EnumDescriptor; @@ -81,8 +83,12 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import org.projectnessie.cel.common.ULong; +import org.projectnessie.cel.common.types.IteratorT; +import org.projectnessie.cel.common.types.MapT; import org.projectnessie.cel.common.types.NullT; import org.projectnessie.cel.common.types.TypeT; +import org.projectnessie.cel.common.types.ref.FieldGetter; import org.projectnessie.cel.common.types.ref.FieldType; import org.projectnessie.cel.common.types.ref.TypeRegistry; import org.projectnessie.cel.common.types.ref.Val; @@ -214,11 +220,18 @@ private FieldType loadFieldType(String messageType, String fieldName) { if (field == null) { return null; } - FieldDescription resolvedField = field; - return new FieldType( - resolvedField.checkedType(), - resolvedField::hasField, - target -> resolvedField.getField(target, this)); + FieldGetter getter = target -> field.getField(target, this); + PbTypeDescription type = pbdb.describeType(messageType); + FieldGetter generatedGetter = type != null ? GeneratedFieldAccessor.create(type, field) : null; + if (generatedGetter != null) { + Class generatedType = type.reflectType(); + getter = + target -> + generatedType.isInstance(target) + ? generatedGetter.getFrom(target) + : field.getField(target, this); + } + return new FieldType(field.checkedType(), field::hasField, getter); } FieldDescription findFieldDescription(String messageType, String fieldName) { @@ -230,9 +243,6 @@ FieldDescription findFieldDescription(String messageType, String fieldName) { if (field == null) { field = pbdb.describeExtension(messageType, fieldName); } - if (field == null) { - return null; - } return field; } @@ -287,9 +297,6 @@ private Val newValueSetFields(Map fields, PbTypeDescription td, Bui return noSuchField(name); } - // TODO resolve inefficiency for maps: first converted from a MapT to a native Java map and - // then to a protobuf struct. The intermediate step (the Java map) could be omitted. - FieldDescriptor pbDesc = field.descriptor(); if (nv.getValue() == org.projectnessie.cel.common.types.NullT.NullValue && isNullClearedField(pbDesc)) { @@ -297,17 +304,22 @@ && isNullClearedField(pbDesc)) { } try { - Object value = toNativeFieldValue(nv.getValue(), field); - if (value.getClass().isArray()) { - value = Arrays.asList((Object[]) value); - } + Object value; + if (pbDesc.isMapField() && nv.getValue() instanceof MapT map) { + value = toProtoMapStructure(field, map); + } else { + value = toNativeFieldValue(nv.getValue(), field); + if (value.getClass().isArray()) { + value = Arrays.asList((Object[]) value); + } - if (pbDesc.getJavaType() == JavaType.ENUM) { - value = intToProtoEnumValues(field, value); - } + if (pbDesc.getJavaType() == JavaType.ENUM) { + value = intToProtoEnumValues(field, value); + } - if (pbDesc.isMapField()) { - value = toProtoMapStructure(pbDesc, value); + if (pbDesc.isMapField()) { + value = toProtoMapStructure(pbDesc, value); + } } builder.setField(pbDesc, value); @@ -385,6 +397,58 @@ private static Class messageNativeType(FieldDescriptor field) { }; } + /** Converts a CEL map directly to protobuf map entries without an intermediate Java map. */ + private Object toProtoMapStructure(FieldDescription field, MapT value) { + FieldDescriptor fieldDesc = field.descriptor(); + Descriptor entryType = fieldDesc.getMessageType(); + FieldDescriptor keyType = field.keyType.descriptor(); + FieldDescriptor valueType = field.valueType.descriptor(); + WireFormat.FieldType keyFieldType = WireFormat.FieldType.valueOf(keyType.getType().name()); + WireFormat.FieldType valueFieldType = WireFormat.FieldType.valueOf(valueType.getType().name()); + List> entries = new ArrayList<>((int) value.size().intValue()); + + IteratorT iterator = value.iterator(); + while (iterator.hasNext() == True) { + Val key = iterator.next(); + Val mapValue = value.find(key); + if (mapValue == NullT.NullValue && isNullPrunedMessageField(valueType)) { + continue; + } + + Object nativeKey = toNativeMapEntryValue(key, keyType); + Object nativeValue = toNativeMapEntryValue(mapValue, valueType); + entries.add( + MapEntry.newDefaultInstance( + entryType, keyFieldType, nativeKey, valueFieldType, nativeValue)); + } + return entries; + } + + private static Object toNativeMapEntryValue(Val value, FieldDescriptor field) { + return switch (field.getType()) { + case DOUBLE -> value.convertToNative(Double.class); + case FLOAT -> value.convertToNative(Float.class); + case INT64, SINT64, SFIXED64 -> value.convertToNative(Long.class); + case UINT64, FIXED64 -> value.convertToNative(ULong.class).longValue(); + case INT32, SINT32, SFIXED32 -> value.convertToNative(Integer.class); + case UINT32, FIXED32 -> value.convertToNative(ULong.class).intValue(); + case BOOL -> value.convertToNative(Boolean.class); + case STRING -> value.convertToNative(String.class); + case BYTES -> value.convertToNative(ByteString.class); + case ENUM -> { + if (value == NullT.NullValue) { + if (field.getEnumType().getFullName().equals("google.protobuf.NullValue")) { + yield 0; + } + throw new IllegalArgumentException("null is only valid for google.protobuf.NullValue"); + } + yield value.convertToNative(Integer.class); + } + case MESSAGE -> value.convertToNative(messageNativeType(field)); + case GROUP -> throw new IllegalArgumentException("protobuf maps cannot contain group values"); + }; + } + /** * Converts {@code value}, of the map-field {@code fieldDesc} from its Java {@link Map} * representation to the protobuf-y {@code {@link List}<{@link MapEntry}>} representation. diff --git a/core/src/test/java/org/projectnessie/cel/common/types/ProviderTest.java b/core/src/test/java/org/projectnessie/cel/common/types/ProviderTest.java index ad59033f5..b969f316b 100644 --- a/core/src/test/java/org/projectnessie/cel/common/types/ProviderTest.java +++ b/core/src/test/java/org/projectnessie/cel/common/types/ProviderTest.java @@ -60,6 +60,7 @@ import java.time.Instant; import java.util.Calendar; import java.util.Date; +import java.util.Map; import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; @@ -164,7 +165,7 @@ void typeRegistryNewValue() { assertThat(srcInfo) .extracting( SourceInfo::getLocation, SourceInfo::getLineOffsetsList, SourceInfo::getPositionsMap) - .containsExactly("TestTypeRegistryNewValue", asList(0, 2), mapOf(1L, 2L, 3L, 4L)); + .containsExactly("TestTypeRegistryNewValue", asList(0, 2), mapOf(1L, 2, 3L, 4)); } @Test @@ -297,6 +298,47 @@ reg, mapOf(true, NullValue, false, durationOf(Duration.ofSeconds(1)))), .containsExactlyEntriesOf(mapOf(false, Int32Value.of(1))); } + @Test + void typeRegistryNewValue_ConvertsMapEntriesDirectly() { + TypeRegistry reg = newRegistry(TestAllTypes.getDefaultInstance()); + Val nestedMessage = + reg.newValue( + "cel.expr.conformance.proto3.TestAllTypes.NestedMessage", mapOf("bb", intOf(42))); + + Val exp = + reg.newValue( + "cel.expr.conformance.proto3.TestAllTypes", + mapOf( + "map_bool_bytes", + new DirectConversionMap(reg, mapOf(true, bytesOf("bytes"))), + "map_bool_int32", + new DirectConversionMap(reg, mapOf(true, intOf(Integer.MIN_VALUE))), + "map_bool_uint32", + new DirectConversionMap(reg, mapOf(true, uintOf(0xffff_ffffL))), + "map_bool_uint64", + new DirectConversionMap(reg, mapOf(true, uintOf(-1L))), + "map_bool_float", + new DirectConversionMap(reg, mapOf(true, doubleOf(1.25))), + "map_bool_enum", + new DirectConversionMap(reg, mapOf(true, intOf(TestAllTypes.NestedEnum.BAR_VALUE))), + "map_bool_null_value", + new DirectConversionMap(reg, mapOf(true, NullValue)), + "map_bool_message", + new DirectConversionMap(reg, mapOf(true, nestedMessage)))); + + assertThat(exp).matches(v -> !Err.isError(v)); + TestAllTypes value = exp.convertToNative(TestAllTypes.class); + assertThat(value.getMapBoolBytesMap()).containsEntry(true, ByteString.copyFromUtf8("bytes")); + assertThat(value.getMapBoolInt32Map()).containsEntry(true, Integer.MIN_VALUE); + assertThat(value.getMapBoolUint32Map()).containsEntry(true, -1); + assertThat(value.getMapBoolUint64Map()).containsEntry(true, -1L); + assertThat(value.getMapBoolFloatMap()).containsEntry(true, 1.25F); + assertThat(value.getMapBoolEnumMap()).containsEntry(true, TestAllTypes.NestedEnum.BAR); + assertThat(value.getMapBoolNullValueMap()).containsEntry(true, NULL_VALUE); + assertThat(value.getMapBoolMessageMap()) + .containsEntry(true, TestAllTypes.NestedMessage.newBuilder().setBb(42).build()); + } + @Test void typeRegistryNewValue_InvalidNullFieldAssignmentsReturnErrors() { TypeRegistry reg = newRegistry(TestAllTypes.getDefaultInstance()); @@ -305,10 +347,67 @@ void typeRegistryNewValue_InvalidNullFieldAssignmentsReturnErrors() { assertThat(reg.newValue(typeName, mapOf("single_bool", NullValue))).matches(Err::isError); assertThat(reg.newValue(typeName, mapOf("repeated_int32", NullValue))).matches(Err::isError); assertThat(reg.newValue(typeName, mapOf("map_string_string", NullValue))).matches(Err::isError); + assertThat( + reg.newValue( + typeName, mapOf("map_bool_enum", newMaybeWrappedMap(reg, mapOf(true, NullValue))))) + .matches(Err::isError); assertThat(reg.newValue(typeName, mapOf("list_value", NullValue))).matches(Err::isError); assertThat(reg.newValue(typeName, mapOf("single_struct", NullValue))).matches(Err::isError); } + private static final class DirectConversionMap extends MapT { + private final MapT delegate; + + private DirectConversionMap(TypeRegistry registry, Map value) { + this.delegate = (MapT) newMaybeWrappedMap(registry, value); + } + + @Override + public T convertToNative(Class typeDesc) { + throw new AssertionError("protobuf map conversion must not materialize a native Java map"); + } + + @Override + public Val convertToType(org.projectnessie.cel.common.types.ref.Type typeValue) { + return delegate.convertToType(typeValue); + } + + @Override + public IteratorT iterator() { + return delegate.iterator(); + } + + @Override + public Val equal(Val other) { + return delegate.equal(other); + } + + @Override + public Object value() { + return delegate.value(); + } + + @Override + public Val contains(Val value) { + return delegate.contains(value); + } + + @Override + public Val get(Val index) { + return delegate.get(index); + } + + @Override + public Val size() { + return delegate.size(); + } + + @Override + public Val find(Val key) { + return delegate.find(key); + } + } + @Test void typeRegistryNewValue_ProtobufStructFieldRequiresStringKeys() { TypeRegistry reg = newRegistry(TestAllTypes.getDefaultInstance()); diff --git a/core/src/test/java/org/projectnessie/cel/common/types/pb/GeneratedFieldAccessorTest.java b/core/src/test/java/org/projectnessie/cel/common/types/pb/GeneratedFieldAccessorTest.java new file mode 100644 index 000000000..3cf535df6 --- /dev/null +++ b/core/src/test/java/org/projectnessie/cel/common/types/pb/GeneratedFieldAccessorTest.java @@ -0,0 +1,140 @@ +/* + * Copyright (C) 2021 The Authors of CEL-Java + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.cel.common.types.pb; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.projectnessie.cel.common.types.pb.Db.newDb; + +import com.google.protobuf.ByteString; +import com.google.protobuf.Descriptors.FieldDescriptor; +import com.google.protobuf.DynamicMessage; +import dev.cel.expr.conformance.proto3.TestAllTypes; +import dev.cel.expr.conformance.proto3.TestAllTypes.NestedEnum; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.projectnessie.cel.common.types.ref.FieldGetter; +import org.projectnessie.cel.common.types.ref.FieldType; + +class GeneratedFieldAccessorTest { + + @Test + void bindsSupportedGeneratedScalarGetters() { + TestAllTypes message = + TestAllTypes.newBuilder() + .setSingleInt32(12) + .setSingleInt64(13L) + .setSingleSint32(14) + .setSingleSint64(15L) + .setSingleSfixed32(16) + .setSingleSfixed64(17L) + .setSingleFloat(18.25f) + .setSingleDouble(19.25d) + .setSingleBool(true) + .setSingleString("twenty") + .setSingleBytes(ByteString.copyFromUtf8("twenty-one")) + .build(); + Db db = newDb(); + db.registerMessage(TestAllTypes.getDefaultInstance()); + PbTypeDescription type = db.describeType(message.getDescriptorForType().getFullName()); + + for (String fieldName : + List.of( + "single_int32", + "single_int64", + "single_sint32", + "single_sint64", + "single_sfixed32", + "single_sfixed64", + "single_float", + "single_double", + "single_bool", + "single_string", + "single_bytes")) { + FieldDescription field = type.fieldByName(fieldName); + FieldGetter getter = GeneratedFieldAccessor.create(type, field); + assertThat(getter).as(fieldName).isNotNull(); + assertThat(getter.getFrom(message)) + .as(fieldName) + .isEqualTo(message.getField(field.descriptor())); + } + } + + @Test + void excludesFieldsThatNeedSpecializedNormalization() { + Db db = newDb(); + db.registerMessage(TestAllTypes.getDefaultInstance()); + PbTypeDescription type = db.describeType(TestAllTypes.getDescriptor().getFullName()); + + for (String fieldName : + List.of( + "single_uint32", + "standalone_enum", + "single_nested_message", + "single_int32_wrapper", + "repeated_int32", + "map_string_string")) { + assertThat(GeneratedFieldAccessor.create(type, type.fieldByName(fieldName))) + .as(fieldName) + .isNull(); + } + } + + @Test + void generatedRegistryGetterFallsBackForDynamicMessages() { + TestAllTypes generated = + TestAllTypes.newBuilder().setSingleInt32(42).setStandaloneEnum(NestedEnum.BAR).build(); + DynamicMessage dynamic = + DynamicMessage.newBuilder(generated.getDescriptorForType()).mergeFrom(generated).build(); + ProtoTypeRegistry registry = ProtoTypeRegistry.newRegistry(TestAllTypes.getDefaultInstance()); + + FieldType scalar = + registry.findFieldType(generated.getDescriptorForType().getFullName(), "single_int32"); + FieldType enumField = + registry.findFieldType(generated.getDescriptorForType().getFullName(), "standalone_enum"); + + assertThat(scalar.getFrom.getFrom(generated)).isEqualTo(42); + assertThat(scalar.getFrom.getFrom(dynamic)).isEqualTo(42); + FieldDescriptor enumDescriptor = + generated.getDescriptorForType().findFieldByName("standalone_enum"); + assertThat(enumField.getFrom.getFrom(dynamic)).isEqualTo(dynamic.getField(enumDescriptor)); + } + + @Test + void proto2DefaultsAndPresenceRemainDescriptorDriven() { + dev.cel.expr.conformance.proto2.TestAllTypes absent = + dev.cel.expr.conformance.proto2.TestAllTypes.getDefaultInstance(); + dev.cel.expr.conformance.proto2.TestAllTypes present = + dev.cel.expr.conformance.proto2.TestAllTypes.newBuilder().setSingleInt32(42).build(); + ProtoTypeRegistry registry = ProtoTypeRegistry.newRegistry(absent); + FieldType field = + registry.findFieldType(absent.getDescriptorForType().getFullName(), "single_int32"); + + assertThat(field.getFrom.getFrom(absent)).isEqualTo(-32); + assertThat(field.isSet.isSet(absent)).isFalse(); + assertThat(field.getFrom.getFrom(present)).isEqualTo(42); + assertThat(field.isSet.isSet(present)).isTrue(); + } + + @Test + void dynamicTypeDescriptionDoesNotBindGeneratedGetter() { + DynamicMessage zero = DynamicMessage.getDefaultInstance(TestAllTypes.getDescriptor()); + Db db = newDb(); + db.registerMessage(zero); + PbTypeDescription type = db.describeType(zero.getDescriptorForType().getFullName()); + + assertThat(GeneratedFieldAccessor.create(type, type.fieldByName("single_int32"))).isNull(); + } +} diff --git a/jackson/src/jmh/java/org/projectnessie/cel/types/jackson/JacksonRegistryBench.java b/jackson/src/jmh/java/org/projectnessie/cel/types/jackson/JacksonRegistryBench.java index ae88be30c..f35fd0455 100644 --- a/jackson/src/jmh/java/org/projectnessie/cel/types/jackson/JacksonRegistryBench.java +++ b/jackson/src/jmh/java/org/projectnessie/cel/types/jackson/JacksonRegistryBench.java @@ -31,6 +31,7 @@ import org.openjdk.jmh.annotations.Threads; import org.openjdk.jmh.annotations.Warmup; import org.openjdk.jmh.infra.Blackhole; +import org.projectnessie.cel.common.types.ref.FieldType; @Warmup(iterations = 1, time = 1500, timeUnit = TimeUnit.MILLISECONDS) @Measurement(iterations = 5, time = 300, timeUnit = TimeUnit.MILLISECONDS) @@ -43,18 +44,18 @@ public class JacksonRegistryBench { @State(Scope.Benchmark) public static class ReadState { JacksonRegistry registry; + Policy policy; JacksonObjectT value; + FieldType ownerField; @Setup public void init() { registry = (JacksonRegistry) JacksonRegistry.newRegistry(); registry.typeDescription(Policy.class); registry.enumDescription(Status.class); - value = - JacksonObjectT.newObject( - registry, - new Policy("policy-1", 7, new Principal("alice@example.com"), Status.ACTIVE), - registry.typeDescription(Policy.class)); + policy = new Policy("policy-1", 7, new Principal("alice@example.com"), Status.ACTIVE); + value = JacksonObjectT.newObject(registry, policy, registry.typeDescription(Policy.class)); + ownerField = registry.findFieldType(Policy.class.getName(), "owner"); } } @@ -91,6 +92,11 @@ public void propertyRead(ReadState state, Blackhole blackhole) { blackhole.consume(state.value.get(stringOf("owner"))); } + @Benchmark + public void preResolvedPropertyRead(ReadState state, Blackhole blackhole) { + blackhole.consume(state.ownerField.getFrom.getFrom(state.policy)); + } + @Benchmark public void enumConversion(ReadState state, Blackhole blackhole) { blackhole.consume(state.registry.nativeToValue(Status.ACTIVE)); diff --git a/jackson/src/main/java/org/projectnessie/cel/types/jackson/JacksonObjectT.java b/jackson/src/main/java/org/projectnessie/cel/types/jackson/JacksonObjectT.java index 5108c085b..2b1e07a0b 100644 --- a/jackson/src/main/java/org/projectnessie/cel/types/jackson/JacksonObjectT.java +++ b/jackson/src/main/java/org/projectnessie/cel/types/jackson/JacksonObjectT.java @@ -50,11 +50,12 @@ public Val isSet(Val field) { } String fieldName = (String) field.value(); - if (!typeDesc().hasProperty(fieldName)) { + JacksonFieldType fieldType = typeDesc().fieldType(fieldName); + if (fieldType == null) { return noSuchField(fieldName); } - Object value = typeDesc().fromObject(value(), fieldName); + Object value = fieldType.getFrom.getFrom(value()); return boolOf(value != null); } @@ -66,11 +67,12 @@ public Val get(Val index) { } String fieldName = (String) index.value(); - if (!typeDesc().hasProperty(fieldName)) { + JacksonFieldType fieldType = typeDesc().fieldType(fieldName); + if (fieldType == null) { return noSuchField(fieldName); } - Object v = typeDesc().fromObject(value(), fieldName); + Object v = fieldType.getFrom.getFrom(value()); return registry().nativeToValue(v); } diff --git a/jackson/src/main/java/org/projectnessie/cel/types/jackson/JacksonTypeDescription.java b/jackson/src/main/java/org/projectnessie/cel/types/jackson/JacksonTypeDescription.java index 5174e2c9a..1c40a6ee9 100644 --- a/jackson/src/main/java/org/projectnessie/cel/types/jackson/JacksonTypeDescription.java +++ b/jackson/src/main/java/org/projectnessie/cel/types/jackson/JacksonTypeDescription.java @@ -17,11 +17,18 @@ import com.fasterxml.jackson.databind.JavaType; import com.fasterxml.jackson.databind.JsonSerializer; +import com.fasterxml.jackson.databind.introspect.AnnotatedMember; import com.fasterxml.jackson.databind.ser.BeanPropertyWriter; import com.fasterxml.jackson.databind.ser.PropertyWriter; import com.google.protobuf.ByteString; import com.google.protobuf.Duration; import com.google.protobuf.Timestamp; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; +import java.lang.reflect.Field; +import java.lang.reflect.Member; +import java.lang.reflect.Method; import java.time.Instant; import java.time.ZonedDateTime; import java.util.Collection; @@ -33,7 +40,7 @@ import org.projectnessie.cel.common.ULong; import org.projectnessie.cel.common.types.TypeT; import org.projectnessie.cel.common.types.pb.Checked; -import org.projectnessie.cel.common.types.ref.FieldType; +import org.projectnessie.cel.common.types.ref.FieldGetter; import org.projectnessie.cel.common.types.ref.Type; import org.projectnessie.cel.common.types.ref.TypeDescription; @@ -58,12 +65,13 @@ final class JacksonTypeDescription implements TypeDescription { while (propIter.hasNext()) { PropertyWriter pw = propIter.next(); String n = pw.getName(); + FieldGetter getter = newFieldGetter(pw, n); JacksonFieldType ft = new JacksonFieldType( findTypeForJacksonType(pw.getType(), typeQuery), - target -> fromObject(target, n) != null, - target -> fromObject(target, n), + target -> getter.getFrom(target) != null, + getter, pw); fieldTypes.put(n, ft); } @@ -139,29 +147,56 @@ private JavaType elementType(JavaType type) { return elementType; } - boolean hasProperty(String property) { - return fieldTypes.containsKey(property); - } + private static FieldGetter newFieldGetter(PropertyWriter propertyWriter, String property) { + if (!(propertyWriter instanceof BeanPropertyWriter beanPropertyWriter)) { + return target -> { + throw new UnsupportedOperationException( + String.format( + "Unknown property-writer '%s' for property '%s'", + propertyWriter.getClass().getName(), property)); + }; + } - Object fromObject(Object value, String property) { - JacksonFieldType ft = fieldTypes.get(property); - if (ft == null) { - throw new IllegalArgumentException(String.format("No property named '%s'", property)); + if (propertyWriter.getClass() == BeanPropertyWriter.class) { + FieldGetter directGetter = newDirectGetter(beanPropertyWriter); + if (directGetter != null) { + return directGetter; + } } - PropertyWriter pw = ft.propertyWriter(); - if (pw instanceof BeanPropertyWriter) { + return target -> { try { - return ((BeanPropertyWriter) pw).get(value); + return beanPropertyWriter.get(target); } catch (Exception e) { throw new RuntimeException(e); } - } else if (pw == null) { + }; + } + + private static FieldGetter newDirectGetter(BeanPropertyWriter propertyWriter) { + AnnotatedMember annotatedMember = propertyWriter.getMember(); + Member member = annotatedMember != null ? annotatedMember.getMember() : null; + try { + MethodHandle getter; + if (member instanceof Method method) { + getter = MethodHandles.lookup().unreflect(method); + } else if (member instanceof Field field) { + getter = MethodHandles.lookup().unreflectGetter(field); + } else { + return null; + } + MethodHandle objectGetter = getter.asType(MethodType.methodType(Object.class, Object.class)); + return target -> invokeGetter(objectGetter, target); + } catch (IllegalAccessException | RuntimeException e) { return null; - } else { - throw new UnsupportedOperationException( - String.format( - "Unknown property-writer '%s' for property '%s'", pw.getClass().getName(), property)); + } + } + + private static Object invokeGetter(MethodHandle getter, Object target) { + try { + return (Object) getter.invokeExact(target); + } catch (Throwable e) { + throw new RuntimeException(e); } } @@ -173,7 +208,7 @@ com.google.api.expr.v1alpha1.Type pbType() { return pbType; } - FieldType fieldType(String fieldName) { + JacksonFieldType fieldType(String fieldName) { return fieldTypes.get(fieldName); } diff --git a/jackson/src/test/java/org/projectnessie/cel/types/jackson/Jackson2TypeDescriptionTest.java b/jackson/src/test/java/org/projectnessie/cel/types/jackson/Jackson2TypeDescriptionTest.java index 3cf8a92cf..a383ef539 100644 --- a/jackson/src/test/java/org/projectnessie/cel/types/jackson/Jackson2TypeDescriptionTest.java +++ b/jackson/src/test/java/org/projectnessie/cel/types/jackson/Jackson2TypeDescriptionTest.java @@ -26,6 +26,7 @@ import static org.projectnessie.cel.common.types.UintT.uintOf; import static org.projectnessie.cel.types.jackson.JacksonRegistry.newRegistry; +import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.JavaType; import com.google.api.expr.v1alpha1.Type.ListType; import com.google.api.expr.v1alpha1.Type.MapType; @@ -51,6 +52,7 @@ import org.projectnessie.cel.common.types.ObjectT; import org.projectnessie.cel.common.types.TypeT; import org.projectnessie.cel.common.types.pb.Checked; +import org.projectnessie.cel.common.types.ref.FieldType; import org.projectnessie.cel.common.types.ref.Val; import org.projectnessie.cel.types.jackson.types.AnEnum; import org.projectnessie.cel.types.jackson.types.CollectionsObject; @@ -60,6 +62,27 @@ class Jackson2TypeDescriptionTest { + static final class AccessorObject { + public final String field; + + @JsonProperty private final String hidden; + + private final String original; + + AccessorObject(String field, String hidden, String original) { + this.field = field; + this.hidden = hidden; + this.original = original; + } + + @JsonProperty("renamed") + public String getOriginal() { + return original; + } + } + + record AccessorRecord(@JsonProperty("record_name") String name) {} + @Test void basics() { JacksonRegistry reg = (JacksonRegistry) newRegistry(); @@ -121,6 +144,32 @@ void basics() { .isInstanceOf(IllegalArgumentException.class); } + @Test + void cachedPropertyAccessorsPreserveDiscoveredMembers() { + JacksonRegistry reg = (JacksonRegistry) newRegistry(); + AccessorObject value = new AccessorObject("field-value", "hidden-value", "method-value"); + reg.register(AccessorObject.class); + + FieldType field = reg.findFieldType(AccessorObject.class.getName(), "field"); + FieldType hidden = reg.findFieldType(AccessorObject.class.getName(), "hidden"); + FieldType renamed = reg.findFieldType(AccessorObject.class.getName(), "renamed"); + + assertThat(field.getFrom.getFrom(value)).isEqualTo("field-value"); + assertThat(hidden.getFrom.getFrom(value)).isEqualTo("hidden-value"); + assertThat(renamed.getFrom.getFrom(value)).isEqualTo("method-value"); + assertThat(renamed.isSet.isSet(value)).isTrue(); + + ObjectT object = (ObjectT) reg.nativeToValue(value); + assertThat(object.get(stringOf("field"))).isEqualTo(stringOf("field-value")); + assertThat(object.get(stringOf("hidden"))).isEqualTo(stringOf("hidden-value")); + assertThat(object.get(stringOf("renamed"))).isEqualTo(stringOf("method-value")); + + AccessorRecord record = new AccessorRecord("record-value"); + reg.register(AccessorRecord.class); + FieldType recordName = reg.findFieldType(AccessorRecord.class.getName(), "record_name"); + assertThat(recordName.getFrom.getFrom(record)).isEqualTo("record-value"); + } + @Test void enumConstantSpecificClassBodyUsesDeclaringClassName() { JacksonRegistry reg = (JacksonRegistry) newRegistry(); diff --git a/jackson3/src/jmh/java/org/projectnessie/cel/types/jackson3/Jackson3RegistryBench.java b/jackson3/src/jmh/java/org/projectnessie/cel/types/jackson3/Jackson3RegistryBench.java index 9e739a791..c15b3a4f5 100644 --- a/jackson3/src/jmh/java/org/projectnessie/cel/types/jackson3/Jackson3RegistryBench.java +++ b/jackson3/src/jmh/java/org/projectnessie/cel/types/jackson3/Jackson3RegistryBench.java @@ -31,6 +31,7 @@ import org.openjdk.jmh.annotations.Threads; import org.openjdk.jmh.annotations.Warmup; import org.openjdk.jmh.infra.Blackhole; +import org.projectnessie.cel.common.types.ref.FieldType; @Warmup(iterations = 1, time = 1500, timeUnit = TimeUnit.MILLISECONDS) @Measurement(iterations = 5, time = 300, timeUnit = TimeUnit.MILLISECONDS) @@ -43,18 +44,18 @@ public class Jackson3RegistryBench { @State(Scope.Benchmark) public static class ReadState { Jackson3Registry registry; + Policy policy; JacksonObjectT value; + FieldType ownerField; @Setup public void init() { registry = (Jackson3Registry) Jackson3Registry.newRegistry(); registry.typeDescription(Policy.class); registry.enumDescription(Status.class); - value = - JacksonObjectT.newObject( - registry, - new Policy("policy-1", 7, new Principal("alice@example.com"), Status.ACTIVE), - registry.typeDescription(Policy.class)); + policy = new Policy("policy-1", 7, new Principal("alice@example.com"), Status.ACTIVE); + value = JacksonObjectT.newObject(registry, policy, registry.typeDescription(Policy.class)); + ownerField = registry.findFieldType(Policy.class.getName(), "owner"); } } @@ -91,6 +92,11 @@ public void propertyRead(ReadState state, Blackhole blackhole) { blackhole.consume(state.value.get(stringOf("owner"))); } + @Benchmark + public void preResolvedPropertyRead(ReadState state, Blackhole blackhole) { + blackhole.consume(state.ownerField.getFrom.getFrom(state.policy)); + } + @Benchmark public void enumConversion(ReadState state, Blackhole blackhole) { blackhole.consume(state.registry.nativeToValue(Status.ACTIVE)); diff --git a/jackson3/src/main/java/org/projectnessie/cel/types/jackson3/JacksonObjectT.java b/jackson3/src/main/java/org/projectnessie/cel/types/jackson3/JacksonObjectT.java index 79c74cf34..4ab3ab362 100644 --- a/jackson3/src/main/java/org/projectnessie/cel/types/jackson3/JacksonObjectT.java +++ b/jackson3/src/main/java/org/projectnessie/cel/types/jackson3/JacksonObjectT.java @@ -50,11 +50,12 @@ public Val isSet(Val field) { } String fieldName = (String) field.value(); - if (!typeDesc().hasProperty(fieldName)) { + JacksonFieldType fieldType = typeDesc().fieldType(fieldName); + if (fieldType == null) { return noSuchField(fieldName); } - Object value = typeDesc().fromObject(value(), fieldName); + Object value = fieldType.getFrom.getFrom(value()); return boolOf(value != null); } @@ -66,11 +67,12 @@ public Val get(Val index) { } String fieldName = (String) index.value(); - if (!typeDesc().hasProperty(fieldName)) { + JacksonFieldType fieldType = typeDesc().fieldType(fieldName); + if (fieldType == null) { return noSuchField(fieldName); } - Object v = typeDesc().fromObject(value(), fieldName); + Object v = fieldType.getFrom.getFrom(value()); return registry().nativeToValue(v); } diff --git a/jackson3/src/main/java/org/projectnessie/cel/types/jackson3/JacksonTypeDescription.java b/jackson3/src/main/java/org/projectnessie/cel/types/jackson3/JacksonTypeDescription.java index a11026766..64a869f76 100644 --- a/jackson3/src/main/java/org/projectnessie/cel/types/jackson3/JacksonTypeDescription.java +++ b/jackson3/src/main/java/org/projectnessie/cel/types/jackson3/JacksonTypeDescription.java @@ -29,7 +29,7 @@ import org.projectnessie.cel.common.ULong; import org.projectnessie.cel.common.types.TypeT; import org.projectnessie.cel.common.types.pb.Checked; -import org.projectnessie.cel.common.types.ref.FieldType; +import org.projectnessie.cel.common.types.ref.FieldGetter; import org.projectnessie.cel.common.types.ref.Type; import org.projectnessie.cel.common.types.ref.TypeDescription; import tools.jackson.databind.JavaType; @@ -58,12 +58,13 @@ final class JacksonTypeDescription implements TypeDescription { while (propIter.hasNext()) { PropertyWriter pw = propIter.next(); String n = pw.getName(); + FieldGetter getter = newFieldGetter(pw, n); JacksonFieldType ft = new JacksonFieldType( findTypeForJacksonType(pw.getType(), typeQuery), - target -> fromObject(target, n) != null, - target -> fromObject(target, n), + target -> getter.getFrom(target) != null, + getter, pw); fieldTypes.put(n, ft); } @@ -139,30 +140,22 @@ private JavaType elementType(JavaType type) { return elementType; } - boolean hasProperty(String property) { - return fieldTypes.containsKey(property); - } - - Object fromObject(Object value, String property) { - JacksonFieldType ft = fieldTypes.get(property); - if (ft == null) { - throw new IllegalArgumentException(String.format("No property named '%s'", property)); + private static FieldGetter newFieldGetter(PropertyWriter propertyWriter, String property) { + if (propertyWriter instanceof BeanPropertyWriter beanPropertyWriter) { + return target -> { + try { + return beanPropertyWriter.get(target); + } catch (Exception e) { + throw new RuntimeException(e); + } + }; } - PropertyWriter pw = ft.propertyWriter(); - - if (pw instanceof BeanPropertyWriter) { - try { - return ((BeanPropertyWriter) pw).get(value); - } catch (Exception e) { - throw new RuntimeException(e); - } - } else if (pw == null) { - return null; - } else { + return target -> { throw new UnsupportedOperationException( String.format( - "Unknown property-writer '%s' for property '%s'", pw.getClass().getName(), property)); - } + "Unknown property-writer '%s' for property '%s'", + propertyWriter.getClass().getName(), property)); + }; } Type type() { @@ -173,7 +166,7 @@ com.google.api.expr.v1alpha1.Type pbType() { return pbType; } - FieldType fieldType(String fieldName) { + JacksonFieldType fieldType(String fieldName) { return fieldTypes.get(fieldName); } diff --git a/jackson3/src/test/java/org/projectnessie/cel/types/jackson3/Jackson3TypeDescriptionTest.java b/jackson3/src/test/java/org/projectnessie/cel/types/jackson3/Jackson3TypeDescriptionTest.java index 230573961..53aa96496 100644 --- a/jackson3/src/test/java/org/projectnessie/cel/types/jackson3/Jackson3TypeDescriptionTest.java +++ b/jackson3/src/test/java/org/projectnessie/cel/types/jackson3/Jackson3TypeDescriptionTest.java @@ -26,6 +26,7 @@ import static org.projectnessie.cel.common.types.UintT.uintOf; import static org.projectnessie.cel.types.jackson3.Jackson3Registry.newRegistry; +import com.fasterxml.jackson.annotation.JsonProperty; import com.google.api.expr.v1alpha1.Type.ListType; import com.google.api.expr.v1alpha1.Type.MapType; import com.google.api.expr.v1alpha1.Type.TypeKindCase; @@ -50,6 +51,7 @@ import org.projectnessie.cel.common.types.ObjectT; import org.projectnessie.cel.common.types.TypeT; import org.projectnessie.cel.common.types.pb.Checked; +import org.projectnessie.cel.common.types.ref.FieldType; import org.projectnessie.cel.common.types.ref.Val; import org.projectnessie.cel.types.jackson3.types.AnEnum; import org.projectnessie.cel.types.jackson3.types.CollectionsObject; @@ -59,6 +61,27 @@ class Jackson3TypeDescriptionTest { + static final class AccessorObject { + public final String field; + + @JsonProperty private final String hidden; + + private final String original; + + AccessorObject(String field, String hidden, String original) { + this.field = field; + this.hidden = hidden; + this.original = original; + } + + @JsonProperty("renamed") + public String getOriginal() { + return original; + } + } + + record AccessorRecord(@JsonProperty("record_name") String name) {} + @Test void basics() { Jackson3Registry reg = (Jackson3Registry) newRegistry(); @@ -120,6 +143,32 @@ void basics() { .isInstanceOf(IllegalArgumentException.class); } + @Test + void cachedPropertyAccessorsPreserveDiscoveredMembers() { + Jackson3Registry reg = (Jackson3Registry) newRegistry(); + AccessorObject value = new AccessorObject("field-value", "hidden-value", "method-value"); + reg.register(AccessorObject.class); + + FieldType field = reg.findFieldType(AccessorObject.class.getName(), "field"); + FieldType hidden = reg.findFieldType(AccessorObject.class.getName(), "hidden"); + FieldType renamed = reg.findFieldType(AccessorObject.class.getName(), "renamed"); + + assertThat(field.getFrom.getFrom(value)).isEqualTo("field-value"); + assertThat(hidden.getFrom.getFrom(value)).isEqualTo("hidden-value"); + assertThat(renamed.getFrom.getFrom(value)).isEqualTo("method-value"); + assertThat(renamed.isSet.isSet(value)).isTrue(); + + ObjectT object = (ObjectT) reg.nativeToValue(value); + assertThat(object.get(stringOf("field"))).isEqualTo(stringOf("field-value")); + assertThat(object.get(stringOf("hidden"))).isEqualTo(stringOf("hidden-value")); + assertThat(object.get(stringOf("renamed"))).isEqualTo(stringOf("method-value")); + + AccessorRecord record = new AccessorRecord("record-value"); + reg.register(AccessorRecord.class); + FieldType recordName = reg.findFieldType(AccessorRecord.class.getName(), "record_name"); + assertThat(recordName.getFrom.getFrom(record)).isEqualTo("record-value"); + } + @Test void enumConstantSpecificClassBodyUsesDeclaringClassName() { Jackson3Registry reg = (Jackson3Registry) newRegistry();