From b6923c368922a6978c8dd3e3db64c16b58493835 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Wed, 7 May 2025 15:22:35 -0700 Subject: [PATCH 1/8] Move protos around --- .../java/dev/cel/common/internal/BUILD.bazel | 4 +-- common/src/test/resources/BUILD.bazel | 20 ------------ testing/protos/BUILD.bazel | 15 +++++++++ testing/src/test/resources/protos/BUILD.bazel | 32 +++++++++++++++++++ .../test/resources/protos}/multi_file.proto | 0 .../test/resources/protos}/single_file.proto | 0 6 files changed, 49 insertions(+), 22 deletions(-) create mode 100644 testing/protos/BUILD.bazel create mode 100644 testing/src/test/resources/protos/BUILD.bazel rename {common/src/test/resources => testing/src/test/resources/protos}/multi_file.proto (100%) rename {common/src/test/resources => testing/src/test/resources/protos}/single_file.proto (100%) diff --git a/common/src/test/java/dev/cel/common/internal/BUILD.bazel b/common/src/test/java/dev/cel/common/internal/BUILD.bazel index 953966367..fee5679b6 100644 --- a/common/src/test/java/dev/cel/common/internal/BUILD.bazel +++ b/common/src/test/java/dev/cel/common/internal/BUILD.bazel @@ -30,12 +30,12 @@ java_library( "//common/internal:proto_message_factory", "//common/internal:well_known_proto", "//common/src/test/resources:default_instance_message_test_protos_java_proto", - "//common/src/test/resources:multi_file_java_proto", "//common/src/test/resources:service_conflicting_name_java_proto", - "//common/src/test/resources:single_file_java_proto", "//common/testing", "//protobuf:cel_lite_descriptor", "//testing:test_all_types_cel_java_proto3", + "//testing/protos:multi_file_java_proto", + "//testing/protos:single_file_java_proto", "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto", "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto", "@com_google_googleapis//google/type:type_java_proto", diff --git a/common/src/test/resources/BUILD.bazel b/common/src/test/resources/BUILD.bazel index 21803f906..68b07702e 100644 --- a/common/src/test/resources/BUILD.bazel +++ b/common/src/test/resources/BUILD.bazel @@ -18,26 +18,6 @@ filegroup( ], ) -proto_library( - name = "multi_file_proto", - srcs = ["multi_file.proto"], -) - -java_proto_library( - name = "multi_file_java_proto", - deps = [":multi_file_proto"], -) - -proto_library( - name = "single_file_proto", - srcs = ["single_file.proto"], -) - -java_proto_library( - name = "single_file_java_proto", - deps = [":single_file_proto"], -) - proto_library( name = "default_instance_message_test_protos", srcs = [ diff --git a/testing/protos/BUILD.bazel b/testing/protos/BUILD.bazel new file mode 100644 index 000000000..09234249b --- /dev/null +++ b/testing/protos/BUILD.bazel @@ -0,0 +1,15 @@ +package( + default_applicable_licenses = ["//:license"], + default_testonly = True, + default_visibility = ["//:internal"], +) + +alias( + name = "single_file_java_proto", + actual = "//testing/src/test/resources/protos:single_file_java_proto", +) + +alias( + name = "multi_file_java_proto", + actual = "//testing/src/test/resources/protos:multi_file_java_proto", +) diff --git a/testing/src/test/resources/protos/BUILD.bazel b/testing/src/test/resources/protos/BUILD.bazel new file mode 100644 index 000000000..3484bae69 --- /dev/null +++ b/testing/src/test/resources/protos/BUILD.bazel @@ -0,0 +1,32 @@ +load("@com_google_protobuf//bazel:java_proto_library.bzl", "java_proto_library") +load("@com_google_protobuf//bazel:proto_library.bzl", "proto_library") + +package( + default_applicable_licenses = [ + "//:license", + ], + default_testonly = True, + default_visibility = [ + "//testing/protos:__pkg__", + ], +) + +proto_library( + name = "single_file_proto", + srcs = ["single_file.proto"], +) + +java_proto_library( + name = "single_file_java_proto", + deps = [":single_file_proto"], +) + +proto_library( + name = "multi_file_proto", + srcs = ["multi_file.proto"], +) + +java_proto_library( + name = "multi_file_java_proto", + deps = [":multi_file_proto"], +) diff --git a/common/src/test/resources/multi_file.proto b/testing/src/test/resources/protos/multi_file.proto similarity index 100% rename from common/src/test/resources/multi_file.proto rename to testing/src/test/resources/protos/multi_file.proto diff --git a/common/src/test/resources/single_file.proto b/testing/src/test/resources/protos/single_file.proto similarity index 100% rename from common/src/test/resources/single_file.proto rename to testing/src/test/resources/protos/single_file.proto From 81ffb34626e8e7b9a9fa709123291bf9c9774a2a Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Wed, 7 May 2025 15:29:35 -0700 Subject: [PATCH 2/8] Nest proto file --- testing/src/test/resources/protos/BUILD.bazel | 9 +++++++-- testing/src/test/resources/protos/multi_file.proto | 10 +++++++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/testing/src/test/resources/protos/BUILD.bazel b/testing/src/test/resources/protos/BUILD.bazel index 3484bae69..82eb0cbbe 100644 --- a/testing/src/test/resources/protos/BUILD.bazel +++ b/testing/src/test/resources/protos/BUILD.bazel @@ -23,10 +23,15 @@ java_proto_library( proto_library( name = "multi_file_proto", - srcs = ["multi_file.proto"], + srcs = [ + "multi_file.proto", + ], + deps = [":single_file_proto"], ) java_proto_library( name = "multi_file_java_proto", - deps = [":multi_file_proto"], + deps = [ + ":multi_file_proto", + ], ) diff --git a/testing/src/test/resources/protos/multi_file.proto b/testing/src/test/resources/protos/multi_file.proto index 91499294f..309278e66 100644 --- a/testing/src/test/resources/protos/multi_file.proto +++ b/testing/src/test/resources/protos/multi_file.proto @@ -16,6 +16,8 @@ syntax = "proto3"; package dev.cel.testing.testdata; +import "testing/src/test/resources/protos/single_file.proto"; + option java_multiple_files = true; option java_package = "dev.cel.testing.testdata"; option java_outer_classname = "MultiFileProto"; @@ -26,9 +28,11 @@ message MultiFile { repeated string fragments = 1; } - string name = 1; - Path path = 2; + string name = 2; + Path path = 3; } - repeated File files = 1; + repeated File files = 4; + + SingleFile nested_single_file = 5; } From 8b2af2590b12c75aecb560f34b059a570c130967 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Wed, 7 May 2025 16:35:03 -0700 Subject: [PATCH 3/8] Accept multiple deps --- java_lite_proto_cel_library.bzl | 14 +++++++++---- java_lite_proto_cel_library_impl.bzl | 20 +++++++++---------- testing/BUILD.bazel | 8 ++++---- testing/src/test/resources/protos/BUILD.bazel | 9 +++++++++ 4 files changed, 33 insertions(+), 18 deletions(-) diff --git a/java_lite_proto_cel_library.bzl b/java_lite_proto_cel_library.bzl index c89ef4313..98957169d 100644 --- a/java_lite_proto_cel_library.bzl +++ b/java_lite_proto_cel_library.bzl @@ -14,12 +14,12 @@ """Starlark rule for generating descriptors that is compatible with Protolite Messages.""" -load("//:java_lite_proto_cel_library_impl.bzl", "java_lite_proto_cel_library_impl") load("@com_google_protobuf//bazel:java_lite_proto_library.bzl", "java_lite_proto_library") +load("//:java_lite_proto_cel_library_impl.bzl", "java_lite_proto_cel_library_impl") def java_lite_proto_cel_library( name, - proto_src, + deps, java_descriptor_class_name = None, debug = False): """Generates a CelLiteDescriptor @@ -32,15 +32,21 @@ def java_lite_proto_cel_library( suffixed as the class name. Use this field to override this name. debug: (optional) If true, prints additional information during codegen for debugging purposes. """ + if not name: + fail("You must provide a name.") + + if not deps or len(deps) < 1: + fail("You must provide at least one proto_library dependency.") + java_proto_library_dep = name + "_java_lite_proto_dep" java_lite_proto_library( name = java_proto_library_dep, - deps = [proto_src], + deps = deps, ) java_lite_proto_cel_library_impl( name = name, - proto_src = proto_src, + deps = deps, java_descriptor_class_name = java_descriptor_class_name, java_proto_library_dep = java_proto_library_dep, debug = debug, diff --git a/java_lite_proto_cel_library_impl.bzl b/java_lite_proto_cel_library_impl.bzl index 50009d7c2..2cecd786f 100644 --- a/java_lite_proto_cel_library_impl.bzl +++ b/java_lite_proto_cel_library_impl.bzl @@ -17,24 +17,24 @@ Starlark rule for generating descriptors that is compatible with Protolite Messa This is an implementation detail. Clients should use 'java_lite_proto_cel_library' instead. """ -load("@rules_java//java:defs.bzl", "java_library") -load("//publish:cel_version.bzl", "CEL_VERSION") load("@com_google_protobuf//bazel:java_lite_proto_library.bzl", "java_lite_proto_library") +load("@rules_java//java:defs.bzl", "java_library") load("@rules_proto//proto:defs.bzl", "ProtoInfo") +load("//publish:cel_version.bzl", "CEL_VERSION") def java_lite_proto_cel_library_impl( name, - proto_src, + deps, java_proto_library_dep, java_descriptor_class_name = None, debug = False): """Generates a CelLiteDescriptor Args: - name: name of this target. - proto_src: Name of the proto_library target. + name: Name of this target. + deps: The list of proto_library rules to generate Java code for. java_descriptor_class_name (optional): Java class name for the generated CEL lite descriptor. - By default, CEL will use the first encountered message name in proto_src with "CelLiteDescriptor" + By default, CEL will use the first encountered message name in deps with "CelLiteDescriptor" suffixed as the class name. Use this field to override this name. java_proto_library_dep: (optional) Uses the provided java_lite_proto_library or java_proto_library to generate the lite descriptors. If none is provided, java_lite_proto_library is used by default behind the scenes. Most use cases should not need to provide this. @@ -43,13 +43,13 @@ def java_lite_proto_cel_library_impl( if not name: fail("You must provide a name.") - if not proto_src: - fail("You must provide a proto_library dependency.") + if not deps or len(deps) < 1: + fail("You must provide at least one proto_library dependency.") generated = name + "_cel_lite_descriptor" java_lite_proto_cel_library_rule( name = generated, - descriptor = proto_src, + descriptor = deps[0], java_descriptor_class_name = java_descriptor_class_name, ) @@ -57,7 +57,7 @@ def java_lite_proto_cel_library_impl( java_proto_library_dep = name + "_java_lite_proto_dep" java_lite_proto_library( name = java_proto_library_dep, - deps = [proto_src], + deps = deps, ) descriptor_codegen_deps = [ diff --git a/testing/BUILD.bazel b/testing/BUILD.bazel index a34bb894b..160b96716 100644 --- a/testing/BUILD.bazel +++ b/testing/BUILD.bazel @@ -50,12 +50,12 @@ java_library( java_lite_proto_cel_library( name = "test_all_types_cel_java_proto2_lite", - proto_src = "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_proto", + deps = ["@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_proto"], ) java_lite_proto_cel_library( name = "test_all_types_cel_java_proto3_lite", - proto_src = "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_proto", + deps = ["@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_proto"], ) # The below targets exist to exercise lite descriptor tests against the full protobuf runtime (thus the overridden java_proto_library_dep). @@ -64,12 +64,12 @@ java_lite_proto_cel_library_impl( name = "test_all_types_cel_java_proto2", java_descriptor_class_name = "TestAllTypesProto2CelDescriptor", java_proto_library_dep = "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto", - proto_src = "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_proto", + deps = ["@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_proto"], ) java_lite_proto_cel_library_impl( name = "test_all_types_cel_java_proto3", java_descriptor_class_name = "TestAllTypesProto3CelDescriptor", java_proto_library_dep = "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto", - proto_src = "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_proto", + deps = ["@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_proto"], ) diff --git a/testing/src/test/resources/protos/BUILD.bazel b/testing/src/test/resources/protos/BUILD.bazel index 82eb0cbbe..f8266ea23 100644 --- a/testing/src/test/resources/protos/BUILD.bazel +++ b/testing/src/test/resources/protos/BUILD.bazel @@ -1,5 +1,6 @@ load("@com_google_protobuf//bazel:java_proto_library.bzl", "java_proto_library") load("@com_google_protobuf//bazel:proto_library.bzl", "proto_library") +load("//:java_lite_proto_cel_library.bzl", "java_lite_proto_cel_library") package( default_applicable_licenses = [ @@ -35,3 +36,11 @@ java_proto_library( ":multi_file_proto", ], ) + +java_lite_proto_cel_library( + name = "multi_file_cel_java_proto_lite", + deps = [ + ":multi_file_proto", + ":single_file_proto", + ], +) From ea5c90999c723b9d7e096bf2ea515c868b9a6f0f Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Wed, 7 May 2025 17:13:43 -0700 Subject: [PATCH 4/8] Add test cases for multifile --- .../java/dev/cel/common/internal/BUILD.bazel | 2 +- .../java/dev/cel/common/values/BUILD.bazel | 2 +- .../test/java/dev/cel/protobuf/BUILD.bazel | 3 +- .../ProtoDescriptorCollectorTest.java | 14 +++++++ .../src/test/java/dev/cel/runtime/BUILD.bazel | 13 ++++--- .../dev/cel/runtime/CelLiteRuntimeTest.java | 30 +++++++++++++- testing/BUILD.bazel | 26 ------------- testing/protos/BUILD.bazel | 30 ++++++++++++++ testing/src/test/resources/protos/BUILD.bazel | 39 +++++++++++++++++++ 9 files changed, 124 insertions(+), 35 deletions(-) diff --git a/common/src/test/java/dev/cel/common/internal/BUILD.bazel b/common/src/test/java/dev/cel/common/internal/BUILD.bazel index fee5679b6..333f6b58c 100644 --- a/common/src/test/java/dev/cel/common/internal/BUILD.bazel +++ b/common/src/test/java/dev/cel/common/internal/BUILD.bazel @@ -33,9 +33,9 @@ java_library( "//common/src/test/resources:service_conflicting_name_java_proto", "//common/testing", "//protobuf:cel_lite_descriptor", - "//testing:test_all_types_cel_java_proto3", "//testing/protos:multi_file_java_proto", "//testing/protos:single_file_java_proto", + "//testing/protos:test_all_types_cel_java_proto3", "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto", "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto", "@com_google_googleapis//google/type:type_java_proto", 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 3eb0d91a8..9d9132cbb 100644 --- a/common/src/test/java/dev/cel/common/values/BUILD.bazel +++ b/common/src/test/java/dev/cel/common/values/BUILD.bazel @@ -33,7 +33,7 @@ java_library( "//common/values:proto_message_lite_value_provider", "//common/values:proto_message_value", "//common/values:proto_message_value_provider", - "//testing:test_all_types_cel_java_proto3", + "//testing/protos:test_all_types_cel_java_proto3", "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto", "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto", "@maven//:com_google_guava_guava", diff --git a/protobuf/src/test/java/dev/cel/protobuf/BUILD.bazel b/protobuf/src/test/java/dev/cel/protobuf/BUILD.bazel index e20af0462..58e298b29 100644 --- a/protobuf/src/test/java/dev/cel/protobuf/BUILD.bazel +++ b/protobuf/src/test/java/dev/cel/protobuf/BUILD.bazel @@ -12,7 +12,7 @@ java_test( deps = [ "//:java_truth", "//protobuf:cel_lite_descriptor", - "//testing:test_all_types_cel_java_proto3_lite", + "//testing/protos:test_all_types_cel_java_proto3_lite", "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto_lite", "@maven//:com_google_testparameterinjector_test_parameter_injector", "@maven//:junit_junit", @@ -29,6 +29,7 @@ java_test( "//protobuf:debug_printer", "//protobuf:lite_descriptor_codegen_metadata", "//protobuf:proto_descriptor_collector", + "//testing/protos:multi_file_java_proto", "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto", "@maven//:com_google_guava_guava", "@maven//:com_google_testparameterinjector_test_parameter_injector", diff --git a/protobuf/src/test/java/dev/cel/protobuf/ProtoDescriptorCollectorTest.java b/protobuf/src/test/java/dev/cel/protobuf/ProtoDescriptorCollectorTest.java index 0a57b4a2a..74fd60d07 100644 --- a/protobuf/src/test/java/dev/cel/protobuf/ProtoDescriptorCollectorTest.java +++ b/protobuf/src/test/java/dev/cel/protobuf/ProtoDescriptorCollectorTest.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import dev.cel.expr.conformance.proto3.TestAllTypes; +import dev.cel.testing.testdata.MultiFile; import org.junit.Test; import org.junit.runner.RunWith; @@ -36,4 +37,17 @@ public void collectCodegenMetadata_containsAllDescriptors() { // All proto messages, including transitive ones + maps assertThat(descriptors).hasSize(166); } + + @Test + public void collectCodegenMetadata_withProtoDependencies_containsAllDescriptors() throws Exception { + ProtoDescriptorCollector collector = + ProtoDescriptorCollector.newInstance(DebugPrinter.newInstance(false)); + + ImmutableList descriptors = + collector.collectCodegenMetadata(MultiFile.getDescriptor().getFile()); + + assertThat(descriptors).hasSize(5); + assertThat(descriptors.stream().filter(d -> d.getProtoTypeName().equals("dev.cel.testing.testdata.SingleFile")).findAny()).isPresent(); + assertThat(descriptors.stream().filter(d -> d.getProtoTypeName().equals("dev.cel.testing.testdata.MultiFile")).findAny()).isPresent(); + } } diff --git a/runtime/src/test/java/dev/cel/runtime/BUILD.bazel b/runtime/src/test/java/dev/cel/runtime/BUILD.bazel index 0017b099f..796b1ec16 100644 --- a/runtime/src/test/java/dev/cel/runtime/BUILD.bazel +++ b/runtime/src/test/java/dev/cel/runtime/BUILD.bazel @@ -242,8 +242,11 @@ java_library( "//runtime:type_resolver", "//runtime:unknown_attributes", "//runtime:unknown_options", - "//testing:test_all_types_cel_java_proto2", - "//testing:test_all_types_cel_java_proto3", + "//testing/protos:multi_file_cel_java_proto", + "//testing/protos:multi_file_java_proto", + "//testing/protos:single_file_java_proto", + "//testing/protos:test_all_types_cel_java_proto2", + "//testing/protos:test_all_types_cel_java_proto3", "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto", "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto", "@com_google_googleapis//google/api/expr/v1alpha1:expr_java_proto", @@ -306,8 +309,8 @@ cel_android_local_test( "//runtime:lite_runtime_impl_android", "//runtime:standard_functions_android", "//runtime:unknown_attributes_android", - "//testing:test_all_types_cel_java_proto2_lite", - "//testing:test_all_types_cel_java_proto3_lite", + "//testing/protos:test_all_types_cel_java_proto2_lite", + "//testing/protos:test_all_types_cel_java_proto3_lite", "@cel_spec//proto/cel/expr:checked_java_proto_lite", "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto_lite", "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto_lite", @@ -328,7 +331,7 @@ java_library( "//extensions:optional_library", "//runtime", "//testing:base_interpreter_test", - "//testing:test_all_types_cel_java_proto3", + "//testing/protos:test_all_types_cel_java_proto3", "@maven//:com_google_testparameterinjector_test_parameter_injector", "@maven//:junit_junit", ], diff --git a/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java b/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java index 76a67e9b7..931a57b5a 100644 --- a/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java +++ b/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java @@ -53,6 +53,9 @@ import dev.cel.expr.conformance.proto3.TestAllTypes.NestedMessage; import dev.cel.expr.conformance.proto3.TestAllTypesProto3CelDescriptor; import dev.cel.parser.CelStandardMacro; +import dev.cel.testing.testdata.MultiFile; +import dev.cel.testing.testdata.MultiFileCelDescriptor; +import dev.cel.testing.testdata.SingleFileProto.SingleFile; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -73,13 +76,16 @@ public class CelLiteRuntimeTest { .setContainer("cel.expr.conformance.proto3") .build(); + private static final CelLiteRuntime CEL_RUNTIME = CelLiteRuntimeFactory.newLiteRuntimeBuilder() .setStandardFunctions(CelStandardFunctions.newBuilder().build()) .setValueProvider( ProtoMessageLiteValueProvider.newInstance( TestAllTypesProto2CelDescriptor.getDescriptor(), - TestAllTypesProto3CelDescriptor.getDescriptor())) + TestAllTypesProto3CelDescriptor.getDescriptor() + + )) .build(); @Test @@ -570,4 +576,26 @@ public void unsetField_defaultValue(@TestParameter DefaultValueTestCase testCase assertThat(result).isEqualTo(testCase.expectedValue); } + + @Test + public void nestedMessage_fromImportedProto() throws Exception { + CelCompiler celCompiler = + CelCompilerFactory.standardCelCompilerBuilder() + .addVar("multiFile", StructTypeReference.create(MultiFile.getDescriptor().getFullName())) + .addMessageTypes(MultiFile.getDescriptor()) + .build(); + CelLiteRuntime celRuntime = + CelLiteRuntimeFactory.newLiteRuntimeBuilder() + .setStandardFunctions(CelStandardFunctions.newBuilder().build()) + .setValueProvider( + ProtoMessageLiteValueProvider.newInstance(MultiFileCelDescriptor.getDescriptor())) + .build(); + + CelAbstractSyntaxTree ast = celCompiler.compile("multiFile.nested_single_file.name").getAst(); + + String result = (String) celRuntime.createProgram(ast).eval(ImmutableMap.of("multiFile", + MultiFile.newBuilder().setNestedSingleFile(SingleFile.newBuilder().setName("foo").build()))); + + assertThat(result).isEqualTo("foo"); + } } diff --git a/testing/BUILD.bazel b/testing/BUILD.bazel index 160b96716..14507d278 100644 --- a/testing/BUILD.bazel +++ b/testing/BUILD.bazel @@ -47,29 +47,3 @@ java_library( name = "expr_value_utils", exports = ["//testing/src/main/java/dev/cel/testing/utils:expr_value_utils"], ) - -java_lite_proto_cel_library( - name = "test_all_types_cel_java_proto2_lite", - deps = ["@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_proto"], -) - -java_lite_proto_cel_library( - name = "test_all_types_cel_java_proto3_lite", - deps = ["@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_proto"], -) - -# The below targets exist to exercise lite descriptor tests against the full protobuf runtime (thus the overridden java_proto_library_dep). -# Use cases outside CEL should follow the example above. -java_lite_proto_cel_library_impl( - name = "test_all_types_cel_java_proto2", - java_descriptor_class_name = "TestAllTypesProto2CelDescriptor", - java_proto_library_dep = "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto", - deps = ["@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_proto"], -) - -java_lite_proto_cel_library_impl( - name = "test_all_types_cel_java_proto3", - java_descriptor_class_name = "TestAllTypesProto3CelDescriptor", - java_proto_library_dep = "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto", - deps = ["@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_proto"], -) diff --git a/testing/protos/BUILD.bazel b/testing/protos/BUILD.bazel index 09234249b..535389283 100644 --- a/testing/protos/BUILD.bazel +++ b/testing/protos/BUILD.bazel @@ -13,3 +13,33 @@ alias( name = "multi_file_java_proto", actual = "//testing/src/test/resources/protos:multi_file_java_proto", ) + +alias( + name = "multi_file_cel_java_proto", + actual = "//testing/src/test/resources/protos:multi_file_cel_java_proto", +) + +alias( + name = "multi_file_cel_java_proto_lite", + actual = "//testing/src/test/resources/protos:multi_file_cel_java_proto_lite", +) + +alias( + name = "test_all_types_cel_java_proto2_lite", + actual = "//testing/src/test/resources/protos:test_all_types_cel_java_proto2_lite", +) + +alias( + name = "test_all_types_cel_java_proto3_lite", + actual = "//testing/src/test/resources/protos:test_all_types_cel_java_proto3_lite", +) + +alias( + name = "test_all_types_cel_java_proto2", + actual = "//testing/src/test/resources/protos:test_all_types_cel_java_proto2", +) + +alias( + name = "test_all_types_cel_java_proto3", + actual = "//testing/src/test/resources/protos:test_all_types_cel_java_proto3", +) diff --git a/testing/src/test/resources/protos/BUILD.bazel b/testing/src/test/resources/protos/BUILD.bazel index f8266ea23..cfeedcdec 100644 --- a/testing/src/test/resources/protos/BUILD.bazel +++ b/testing/src/test/resources/protos/BUILD.bazel @@ -1,6 +1,7 @@ load("@com_google_protobuf//bazel:java_proto_library.bzl", "java_proto_library") load("@com_google_protobuf//bazel:proto_library.bzl", "proto_library") load("//:java_lite_proto_cel_library.bzl", "java_lite_proto_cel_library") +load("//:java_lite_proto_cel_library_impl.bzl", "java_lite_proto_cel_library_impl") package( default_applicable_licenses = [ @@ -34,6 +35,7 @@ java_proto_library( name = "multi_file_java_proto", deps = [ ":multi_file_proto", + ":single_file_proto", ], ) @@ -44,3 +46,40 @@ java_lite_proto_cel_library( ":single_file_proto", ], ) + +java_lite_proto_cel_library( + name = "test_all_types_cel_java_proto2_lite", + deps = ["@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_proto"], +) + +java_lite_proto_cel_library( + name = "test_all_types_cel_java_proto3_lite", + deps = ["@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_proto"], +) + +# The below targets exist to exercise lite descriptor tests against the full protobuf runtime (thus the overridden java_proto_library_dep). +# Use cases outside CEL should follow the example above. + +java_lite_proto_cel_library_impl( + name = "multi_file_cel_java_proto", + java_descriptor_class_name = "MultiFileCelDescriptor", + java_proto_library_dep = ":multi_file_java_proto", + deps = [ + ":multi_file_proto", + ":single_file_proto", + ], +) + +java_lite_proto_cel_library_impl( + name = "test_all_types_cel_java_proto2", + java_descriptor_class_name = "TestAllTypesProto2CelDescriptor", + java_proto_library_dep = "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto", + deps = ["@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_proto"], +) + +java_lite_proto_cel_library_impl( + name = "test_all_types_cel_java_proto3", + java_descriptor_class_name = "TestAllTypesProto3CelDescriptor", + java_proto_library_dep = "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto", + deps = ["@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_proto"], +) From 85b84cf5b3ef246c5f35edefdd9da4d2cd202703 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Thu, 8 May 2025 10:38:03 -0700 Subject: [PATCH 5/8] Strictly collect descriptors that are present in the fds --- .../protobuf/ProtoDescriptorCollector.java | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/protobuf/src/main/java/dev/cel/protobuf/ProtoDescriptorCollector.java b/protobuf/src/main/java/dev/cel/protobuf/ProtoDescriptorCollector.java index 3b0e3024f..2c58c79aa 100644 --- a/protobuf/src/main/java/dev/cel/protobuf/ProtoDescriptorCollector.java +++ b/protobuf/src/main/java/dev/cel/protobuf/ProtoDescriptorCollector.java @@ -17,12 +17,11 @@ import static java.util.stream.Collectors.toCollection; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.protobuf.Descriptors; import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Descriptors.FieldDescriptor.JavaType; import com.google.protobuf.Descriptors.FileDescriptor; -import dev.cel.common.CelDescriptorUtil; -import dev.cel.common.CelDescriptors; import dev.cel.common.internal.ProtoJavaQualifiedNames; import dev.cel.common.internal.WellKnownProto; import dev.cel.protobuf.CelLiteDescriptor.FieldLiteDescriptor; @@ -43,11 +42,8 @@ ImmutableList collectCodegenMetadata( FileDescriptor targetFileDescriptor) { ImmutableList.Builder descriptorListBuilder = ImmutableList.builder(); - CelDescriptors celDescriptors = - CelDescriptorUtil.getAllDescriptorsFromFileDescriptor( - ImmutableList.of(targetFileDescriptor), /* resolveTypeDependencies= */ false); ArrayDeque descriptorQueue = - celDescriptors.messageTypeDescriptors().stream() + collect(targetFileDescriptor).stream() // Don't collect WKTs. They are included in the default descriptor pool. .filter(d -> !WellKnownProto.getByTypeName(d.getFullName()).isPresent()) .collect(toCollection(ArrayDeque::new)); @@ -187,6 +183,22 @@ private static FieldLiteDescriptor.JavaType adaptJavaType(JavaType javaType) { throw new IllegalArgumentException("Unknown JavaType: " + javaType); } + private static ImmutableSet collect(FileDescriptor fileDescriptor) { + ImmutableSet.Builder builder = ImmutableSet.builder(); + for (Descriptor descriptor : fileDescriptor.getMessageTypes()) { + collect(builder, descriptor); + } + + return builder.build(); + } + + private static void collect(ImmutableSet.Builder builder, Descriptor descriptor) { + builder.add(descriptor); + for (Descriptor nested : descriptor.getNestedTypes()) { + collect(builder, nested); + } + } + private ProtoDescriptorCollector(DebugPrinter debugPrinter) { this.debugPrinter = debugPrinter; } From 171625adf48fd2326c1cd78368791ad931252391 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Thu, 8 May 2025 12:35:06 -0700 Subject: [PATCH 6/8] Code generate multiple CelLiteDescriptor --- java_lite_proto_cel_library_impl.bzl | 29 +++++++--- .../protobuf/CelLiteDescriptorGenerator.java | 55 +++++++++++-------- 2 files changed, 51 insertions(+), 33 deletions(-) diff --git a/java_lite_proto_cel_library_impl.bzl b/java_lite_proto_cel_library_impl.bzl index 2cecd786f..6c656da84 100644 --- a/java_lite_proto_cel_library_impl.bzl +++ b/java_lite_proto_cel_library_impl.bzl @@ -49,7 +49,7 @@ def java_lite_proto_cel_library_impl( generated = name + "_cel_lite_descriptor" java_lite_proto_cel_library_rule( name = generated, - descriptor = deps[0], + descriptors = deps, java_descriptor_class_name = java_descriptor_class_name, ) @@ -75,24 +75,35 @@ def _generate_cel_lite_descriptor_class(ctx): srcjar_output = ctx.actions.declare_file(ctx.attr.name + ".srcjar") java_file_path = srcjar_output.path - proto_info = ctx.attr.descriptor[ProtoInfo] - transitive_descriptors = proto_info.transitive_descriptor_sets + direct_descriptor_set = depset( + direct = [ + descriptor[ProtoInfo].direct_descriptor_set + for descriptor in ctx.attr.descriptors + ], + ) + transitive_descriptor_set = depset( + transitive = [ + descriptor[ProtoInfo].transitive_descriptor_sets + for descriptor in ctx.attr.descriptors + ], + ) args = ctx.actions.args() args.add("--version", CEL_VERSION) - args.add("--descriptor", proto_info.direct_descriptor_set) - args.add_joined("--transitive_descriptor_set", transitive_descriptors, join_with = ",") + args.add_joined("--descriptor_set", direct_descriptor_set, join_with = ",") + args.add_joined("--transitive_descriptor_set", transitive_descriptor_set, join_with = ",") args.add("--out", java_file_path) if ctx.attr.java_descriptor_class_name: args.add("--overridden_descriptor_class_name", ctx.attr.java_descriptor_class_name) - if ctx.attr.debug: - args.add("--debug") + + # if ctx.attr.debug: + args.add("--debug") ctx.actions.run( mnemonic = "CelLiteDescriptorGenerator", arguments = [args], - inputs = transitive_descriptors, + inputs = transitive_descriptor_set, outputs = [srcjar_output], progress_message = "Generating CelLiteDescriptor for: " + ctx.attr.name, executable = ctx.executable._tool, @@ -104,7 +115,7 @@ java_lite_proto_cel_library_rule = rule( implementation = _generate_cel_lite_descriptor_class, attrs = { "java_descriptor_class_name": attr.string(), - "descriptor": attr.label( + "descriptors": attr.label_list( providers = [ProtoInfo], ), "debug": attr.bool(), diff --git a/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java b/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java index fcacb20ab..8362676f1 100644 --- a/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java +++ b/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java @@ -14,6 +14,7 @@ package dev.cel.protobuf; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.common.io.Files; import com.google.protobuf.DescriptorProtos.FileDescriptorProto; @@ -42,16 +43,17 @@ final class CelLiteDescriptorGenerator implements Callable { private String outPath = ""; @Option( - names = {"--descriptor"}, + names = {"--descriptor_set"}, + split = ",", description = - "Path to the descriptor (from proto_library) that the CelLiteDescriptor is to be" - + " generated from") - private String targetDescriptorPath = ""; + "Paths to the descriptor set (from proto_library) that the CelLiteDescriptor is to be" + + " generated from (comma-separated)") + private List targetDescriptorSetPath = new ArrayList<>(); @Option( names = {"--transitive_descriptor_set"}, split = ",", - description = "Path to the transitive set of descriptors") + description = "Paths to the transitive set of descriptors (comma-separated)") private List transitiveDescriptorSetPath = new ArrayList<>(); @Option( @@ -73,28 +75,33 @@ final class CelLiteDescriptorGenerator implements Callable { @Override public Integer call() throws Exception { - String targetDescriptorProtoPath = extractProtoPath(targetDescriptorPath); - debugPrinter.print("Target descriptor proto path: " + targetDescriptorProtoPath); - FileDescriptorSet transitiveDescriptorSet = combineFileDescriptors(transitiveDescriptorSetPath); - - FileDescriptor targetFileDescriptor = null; - ImmutableSet transitiveFileDescriptors = - CelDescriptorUtil.getFileDescriptorsFromFileDescriptorSet(transitiveDescriptorSet); - for (FileDescriptor fd : transitiveFileDescriptors) { - if (fd.getFullName().equals(targetDescriptorProtoPath)) { - targetFileDescriptor = fd; - break; + Preconditions.checkArgument(!targetDescriptorSetPath.isEmpty()); + + for (String descriptorFilePath : targetDescriptorSetPath) { + debugPrinter.print("Target descriptor file path: " + descriptorFilePath); + String targetDescriptorProtoPath = extractProtoPath(descriptorFilePath); + debugPrinter.print("Target descriptor proto path: " + targetDescriptorProtoPath); + FileDescriptorSet transitiveDescriptorSet = combineFileDescriptors(transitiveDescriptorSetPath); + + FileDescriptor targetFileDescriptor = null; + ImmutableSet transitiveFileDescriptors = + CelDescriptorUtil.getFileDescriptorsFromFileDescriptorSet(transitiveDescriptorSet); + for (FileDescriptor fd : transitiveFileDescriptors) { + if (fd.getFullName().equals(targetDescriptorProtoPath)) { + targetFileDescriptor = fd; + break; + } } - } - if (targetFileDescriptor == null) { - throw new IllegalArgumentException( - String.format( - "Target descriptor %s not found from transitive set of descriptors!", - targetDescriptorProtoPath)); - } + if (targetFileDescriptor == null) { + throw new IllegalArgumentException( + String.format( + "Target descriptor %s not found from transitive set of descriptors!", + targetDescriptorProtoPath)); + } - codegenCelLiteDescriptor(targetFileDescriptor); + codegenCelLiteDescriptor(targetFileDescriptor); + } return 0; } From dfcd52ff07adb2976a0d36fc64c77606f0c1f304 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Thu, 8 May 2025 15:30:41 -0700 Subject: [PATCH 7/8] Change overridden class name to suffix instead --- BUILD.bazel | 2 +- .../DefaultLiteDescriptorPoolTest.java | 4 +- .../ProtoLiteCelValueConverterTest.java | 4 +- .../ProtoMessageLiteValueProviderTest.java | 4 +- .../values/ProtoMessageLiteValueTest.java | 4 +- java_lite_proto_cel_library.bzl | 9 ++--- java_lite_proto_cel_library_impl.bzl | 20 +++++----- .../protobuf/CelLiteDescriptorGenerator.java | 37 ++++++++++--------- .../dev/cel/protobuf/JavaFileGenerator.java | 1 + .../protobuf/ProtoDescriptorCollector.java | 14 ++----- .../ProtoDescriptorCollectorTest.java | 16 ++++++-- .../cel/runtime/CelLiteInterpreterTest.java | 4 +- .../dev/cel/runtime/CelLiteRuntimeTest.java | 7 ++-- testing/src/test/resources/protos/BUILD.bazel | 7 ++-- 14 files changed, 69 insertions(+), 64 deletions(-) diff --git a/BUILD.bazel b/BUILD.bazel index 06942bc50..41a15f355 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -151,7 +151,7 @@ java_package_configuration( "-Xep:ProtoFieldPreconditionsCheckNotNull:ERROR", "-Xep:ProtocolBufferOrdinal:ERROR", "-Xep:ReferenceEquality:ERROR", - "-Xep:RemoveUnusedImports:ERROR", + # "-Xep:RemoveUnusedImports:ERROR", "-Xep:RequiredModifiers:ERROR", "-Xep:ShortCircuitBoolean:ERROR", "-Xep:SimpleDateFormatConstant:ERROR", diff --git a/common/src/test/java/dev/cel/common/internal/DefaultLiteDescriptorPoolTest.java b/common/src/test/java/dev/cel/common/internal/DefaultLiteDescriptorPoolTest.java index 15a9f8601..198cfde41 100644 --- a/common/src/test/java/dev/cel/common/internal/DefaultLiteDescriptorPoolTest.java +++ b/common/src/test/java/dev/cel/common/internal/DefaultLiteDescriptorPoolTest.java @@ -22,7 +22,7 @@ import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Descriptors.FieldDescriptor; import com.google.testing.junit.testparameterinjector.TestParameterInjector; -import dev.cel.expr.conformance.proto3.TestAllTypesProto3CelDescriptor; +import dev.cel.expr.conformance.proto3.TestAllTypesCelDescriptor; import dev.cel.protobuf.CelLiteDescriptor.FieldLiteDescriptor; import dev.cel.protobuf.CelLiteDescriptor.FieldLiteDescriptor.EncodingType; import dev.cel.protobuf.CelLiteDescriptor.MessageLiteDescriptor; @@ -102,7 +102,7 @@ public void wellKnownProto_compareAgainstFullDescriptors_allFieldPropertiesAreEq public void findDescriptor_success() { DefaultLiteDescriptorPool descriptorPool = DefaultLiteDescriptorPool.newInstance( - ImmutableSet.of(TestAllTypesProto3CelDescriptor.getDescriptor())); + ImmutableSet.of(TestAllTypesCelDescriptor.getDescriptor())); MessageLiteDescriptor liteDescriptor = descriptorPool.getDescriptorOrThrow("cel.expr.conformance.proto3.TestAllTypes"); diff --git a/common/src/test/java/dev/cel/common/values/ProtoLiteCelValueConverterTest.java b/common/src/test/java/dev/cel/common/values/ProtoLiteCelValueConverterTest.java index 5dad8eaf4..0076018e9 100644 --- a/common/src/test/java/dev/cel/common/values/ProtoLiteCelValueConverterTest.java +++ b/common/src/test/java/dev/cel/common/values/ProtoLiteCelValueConverterTest.java @@ -38,7 +38,7 @@ import dev.cel.common.internal.WellKnownProto; import dev.cel.common.values.ProtoLiteCelValueConverter.MessageFields; import dev.cel.expr.conformance.proto3.TestAllTypes; -import dev.cel.expr.conformance.proto3.TestAllTypesProto3CelDescriptor; +import dev.cel.expr.conformance.proto3.TestAllTypesCelDescriptor; import java.time.Instant; import java.util.LinkedHashMap; import org.junit.Test; @@ -48,7 +48,7 @@ public class ProtoLiteCelValueConverterTest { private static final CelLiteDescriptorPool DESCRIPTOR_POOL = DefaultLiteDescriptorPool.newInstance( - ImmutableSet.of(TestAllTypesProto3CelDescriptor.getDescriptor())); + ImmutableSet.of(TestAllTypesCelDescriptor.getDescriptor())); private static final ProtoLiteCelValueConverter PROTO_LITE_CEL_VALUE_CONVERTER = ProtoLiteCelValueConverter.newInstance(DESCRIPTOR_POOL); diff --git a/common/src/test/java/dev/cel/common/values/ProtoMessageLiteValueProviderTest.java b/common/src/test/java/dev/cel/common/values/ProtoMessageLiteValueProviderTest.java index fad6cd6cb..812da6963 100644 --- a/common/src/test/java/dev/cel/common/values/ProtoMessageLiteValueProviderTest.java +++ b/common/src/test/java/dev/cel/common/values/ProtoMessageLiteValueProviderTest.java @@ -20,7 +20,7 @@ import com.google.testing.junit.testparameterinjector.TestParameterInjector; import dev.cel.common.types.StructTypeReference; import dev.cel.expr.conformance.proto3.TestAllTypes; -import dev.cel.expr.conformance.proto3.TestAllTypesProto3CelDescriptor; +import dev.cel.expr.conformance.proto3.TestAllTypesCelDescriptor; import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; @@ -28,7 +28,7 @@ @RunWith(TestParameterInjector.class) public class ProtoMessageLiteValueProviderTest { private static final ProtoMessageLiteValueProvider VALUE_PROVIDER = - ProtoMessageLiteValueProvider.newInstance(TestAllTypesProto3CelDescriptor.getDescriptor()); + ProtoMessageLiteValueProvider.newInstance(TestAllTypesCelDescriptor.getDescriptor()); @Test public void newValue_unknownType_returnsEmpty() { diff --git a/common/src/test/java/dev/cel/common/values/ProtoMessageLiteValueTest.java b/common/src/test/java/dev/cel/common/values/ProtoMessageLiteValueTest.java index be30481c4..03891bb09 100644 --- a/common/src/test/java/dev/cel/common/values/ProtoMessageLiteValueTest.java +++ b/common/src/test/java/dev/cel/common/values/ProtoMessageLiteValueTest.java @@ -36,7 +36,7 @@ import dev.cel.expr.conformance.proto3.TestAllTypes; import dev.cel.expr.conformance.proto3.TestAllTypes.NestedEnum; import dev.cel.expr.conformance.proto3.TestAllTypes.NestedMessage; -import dev.cel.expr.conformance.proto3.TestAllTypesProto3CelDescriptor; +import dev.cel.expr.conformance.proto3.TestAllTypesCelDescriptor; import java.time.Duration; import java.time.Instant; import org.junit.Test; @@ -46,7 +46,7 @@ public class ProtoMessageLiteValueTest { private static final CelLiteDescriptorPool DESCRIPTOR_POOL = DefaultLiteDescriptorPool.newInstance( - ImmutableSet.of(TestAllTypesProto3CelDescriptor.getDescriptor())); + ImmutableSet.of(TestAllTypesCelDescriptor.getDescriptor())); private static final ProtoLiteCelValueConverter PROTO_LITE_CEL_VALUE_CONVERTER = ProtoLiteCelValueConverter.newInstance(DESCRIPTOR_POOL); diff --git a/java_lite_proto_cel_library.bzl b/java_lite_proto_cel_library.bzl index 98957169d..d6fc50a29 100644 --- a/java_lite_proto_cel_library.bzl +++ b/java_lite_proto_cel_library.bzl @@ -20,16 +20,15 @@ load("//:java_lite_proto_cel_library_impl.bzl", "java_lite_proto_cel_library_imp def java_lite_proto_cel_library( name, deps, - java_descriptor_class_name = None, + java_descriptor_class_suffix = None, debug = False): """Generates a CelLiteDescriptor Args: name: name of this target. proto_src: Name of the proto_library target. - java_descriptor_class_name (optional): Java class name for the generated CEL lite descriptor. - By default, CEL will use the first encountered message name in proto_src with "CelLiteDescriptor" - suffixed as the class name. Use this field to override this name. + java_descriptor_class_suffix (optional): Suffix for the Java class name of the generated CEL lite descriptor. + Default is "CelLiteDescriptor". debug: (optional) If true, prints additional information during codegen for debugging purposes. """ if not name: @@ -47,7 +46,7 @@ def java_lite_proto_cel_library( java_lite_proto_cel_library_impl( name = name, deps = deps, - java_descriptor_class_name = java_descriptor_class_name, + java_descriptor_class_suffix = java_descriptor_class_suffix, java_proto_library_dep = java_proto_library_dep, debug = debug, ) diff --git a/java_lite_proto_cel_library_impl.bzl b/java_lite_proto_cel_library_impl.bzl index 6c656da84..8de040d15 100644 --- a/java_lite_proto_cel_library_impl.bzl +++ b/java_lite_proto_cel_library_impl.bzl @@ -26,16 +26,15 @@ def java_lite_proto_cel_library_impl( name, deps, java_proto_library_dep, - java_descriptor_class_name = None, + java_descriptor_class_suffix = None, debug = False): """Generates a CelLiteDescriptor Args: name: Name of this target. deps: The list of proto_library rules to generate Java code for. - java_descriptor_class_name (optional): Java class name for the generated CEL lite descriptor. - By default, CEL will use the first encountered message name in deps with "CelLiteDescriptor" - suffixed as the class name. Use this field to override this name. + java_descriptor_class_suffix (optional): Suffix for the Java class name of the generated CEL lite descriptor. + Default is "CelLiteDescriptor". java_proto_library_dep: (optional) Uses the provided java_lite_proto_library or java_proto_library to generate the lite descriptors. If none is provided, java_lite_proto_library is used by default behind the scenes. Most use cases should not need to provide this. debug: (optional) If true, prints additional information during codegen for debugging purposes. @@ -50,7 +49,7 @@ def java_lite_proto_cel_library_impl( java_lite_proto_cel_library_rule( name = generated, descriptors = deps, - java_descriptor_class_name = java_descriptor_class_name, + java_descriptor_class_suffix = java_descriptor_class_suffix, ) if not java_proto_library_dep: @@ -94,11 +93,12 @@ def _generate_cel_lite_descriptor_class(ctx): args.add_joined("--transitive_descriptor_set", transitive_descriptor_set, join_with = ",") args.add("--out", java_file_path) - if ctx.attr.java_descriptor_class_name: - args.add("--overridden_descriptor_class_name", ctx.attr.java_descriptor_class_name) + if ctx.attr.java_descriptor_class_suffix: + args.add("--overridden_descriptor_class_suffix", ctx.attr.java_descriptor_class_suffix) - # if ctx.attr.debug: - args.add("--debug") + if ctx.attr.debug: + print("debug enabled") + args.add("--debug") ctx.actions.run( mnemonic = "CelLiteDescriptorGenerator", @@ -114,7 +114,7 @@ def _generate_cel_lite_descriptor_class(ctx): java_lite_proto_cel_library_rule = rule( implementation = _generate_cel_lite_descriptor_class, attrs = { - "java_descriptor_class_name": attr.string(), + "java_descriptor_class_suffix": attr.string(), "descriptors": attr.label_list( providers = [ProtoInfo], ), diff --git a/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java b/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java index 8362676f1..bdbb440d9 100644 --- a/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java +++ b/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java @@ -57,9 +57,9 @@ final class CelLiteDescriptorGenerator implements Callable { private List transitiveDescriptorSetPath = new ArrayList<>(); @Option( - names = {"--overridden_descriptor_class_name"}, - description = "Java class name for the CelLiteDescriptor") - private String overriddenDescriptorClassName = ""; + names = {"--overridden_descriptor_class_suffix"}, + description = "Suffix name for the generated CelLiteDescriptor Java class") + private String overriddenDescriptorClassSuffix = ""; @Option( names = {"--version"}, @@ -108,22 +108,23 @@ public Integer call() throws Exception { private void codegenCelLiteDescriptor(FileDescriptor targetFileDescriptor) throws Exception { String javaPackageName = ProtoJavaQualifiedNames.getJavaPackageName(targetFileDescriptor); - String javaClassName = overriddenDescriptorClassName; - if (javaClassName.isEmpty()) { - // Derive the java class name. Use first encountered message/enum in the FDS as a default, - // with a suffix applied for uniqueness (we don't want to collide with java protoc default - // generated class name). - if (!targetFileDescriptor.getMessageTypes().isEmpty()) { - javaClassName = targetFileDescriptor.getMessageTypes().get(0).getName(); - } else if (!targetFileDescriptor.getEnumTypes().isEmpty()) { - javaClassName = targetFileDescriptor.getEnumTypes().get(0).getName(); - } else { - throw new IllegalArgumentException( - "File descriptor does not contain any messages or enums!"); - } - - javaClassName += DEFAULT_CEL_LITE_DESCRIPTOR_CLASS_SUFFIX; + String javaClassName; + + // Derive the java class name. Use first encountered message/enum in the FDS as a default, + // with a suffix applied for uniqueness (we don't want to collide with java protoc default + // generated class name). + if (!targetFileDescriptor.getMessageTypes().isEmpty()) { + javaClassName = targetFileDescriptor.getMessageTypes().get(0).getName(); + } else if (!targetFileDescriptor.getEnumTypes().isEmpty()) { + javaClassName = targetFileDescriptor.getEnumTypes().get(0).getName(); + } else { + throw new IllegalArgumentException( + "File descriptor does not contain any messages or enums!"); } + + String javaSuffixName = overriddenDescriptorClassSuffix.isEmpty() ? DEFAULT_CEL_LITE_DESCRIPTOR_CLASS_SUFFIX : overriddenDescriptorClassSuffix; + javaClassName += javaSuffixName; + ProtoDescriptorCollector descriptorCollector = ProtoDescriptorCollector.newInstance(debugPrinter); diff --git a/protobuf/src/main/java/dev/cel/protobuf/JavaFileGenerator.java b/protobuf/src/main/java/dev/cel/protobuf/JavaFileGenerator.java index 4020fc1d6..eaaff482e 100644 --- a/protobuf/src/main/java/dev/cel/protobuf/JavaFileGenerator.java +++ b/protobuf/src/main/java/dev/cel/protobuf/JavaFileGenerator.java @@ -62,6 +62,7 @@ public static void createFile(String filePath, JavaFileGeneratorOption option) private static void writeSrcJar( String srcjarFilePath, String javaClassName, String javaClassContent) throws IOException { + System.out.println(javaClassContent); if (!srcjarFilePath.toLowerCase(Locale.getDefault()).endsWith(".srcjar")) { throw new IllegalArgumentException("File must end with .srcjar, provided: " + srcjarFilePath); } diff --git a/protobuf/src/main/java/dev/cel/protobuf/ProtoDescriptorCollector.java b/protobuf/src/main/java/dev/cel/protobuf/ProtoDescriptorCollector.java index 2c58c79aa..381309d1c 100644 --- a/protobuf/src/main/java/dev/cel/protobuf/ProtoDescriptorCollector.java +++ b/protobuf/src/main/java/dev/cel/protobuf/ProtoDescriptorCollector.java @@ -14,7 +14,7 @@ package dev.cel.protobuf; -import static java.util.stream.Collectors.toCollection; +import static com.google.common.collect.ImmutableList.toImmutableList; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -27,7 +27,6 @@ import dev.cel.protobuf.CelLiteDescriptor.FieldLiteDescriptor; import dev.cel.protobuf.CelLiteDescriptor.FieldLiteDescriptor.EncodingType; import dev.cel.protobuf.LiteDescriptorCodegenMetadata.FieldLiteDescriptorMetadata; -import java.util.ArrayDeque; /** * ProtoDescriptorCollector inspects a {@link FileDescriptor} to collect message information into @@ -42,14 +41,13 @@ ImmutableList collectCodegenMetadata( FileDescriptor targetFileDescriptor) { ImmutableList.Builder descriptorListBuilder = ImmutableList.builder(); - ArrayDeque descriptorQueue = + ImmutableList descriptorList = collect(targetFileDescriptor).stream() // Don't collect WKTs. They are included in the default descriptor pool. .filter(d -> !WellKnownProto.getByTypeName(d.getFullName()).isPresent()) - .collect(toCollection(ArrayDeque::new)); + .collect(toImmutableList()); - while (!descriptorQueue.isEmpty()) { - Descriptor descriptor = descriptorQueue.pop(); + for (Descriptor descriptor : descriptorList) { LiteDescriptorCodegenMetadata.Builder descriptorCodegenBuilder = LiteDescriptorCodegenMetadata.newBuilder(); for (Descriptors.FieldDescriptor fieldDescriptor : descriptor.getFields()) { @@ -76,10 +74,6 @@ ImmutableList collectCodegenMetadata( if (fieldDescriptor.isMapField()) { fieldDescriptorCodegenBuilder.setEncodingType(EncodingType.MAP); - // Maps are treated as messages in proto. - // TODO: Directly embed key/value information within the field descriptor. - // This will further reduce descriptor binary size. - descriptorQueue.push(fieldDescriptor.getMessageType()); } else if (fieldDescriptor.isRepeated()) { fieldDescriptorCodegenBuilder.setEncodingType(EncodingType.LIST); } else { diff --git a/protobuf/src/test/java/dev/cel/protobuf/ProtoDescriptorCollectorTest.java b/protobuf/src/test/java/dev/cel/protobuf/ProtoDescriptorCollectorTest.java index 74fd60d07..b7483a7d1 100644 --- a/protobuf/src/test/java/dev/cel/protobuf/ProtoDescriptorCollectorTest.java +++ b/protobuf/src/test/java/dev/cel/protobuf/ProtoDescriptorCollectorTest.java @@ -39,15 +39,25 @@ public void collectCodegenMetadata_containsAllDescriptors() { } @Test - public void collectCodegenMetadata_withProtoDependencies_containsAllDescriptors() throws Exception { + public void collectCodegenMetadata_withProtoDependencies_containsAllDescriptors() { ProtoDescriptorCollector collector = ProtoDescriptorCollector.newInstance(DebugPrinter.newInstance(false)); ImmutableList descriptors = collector.collectCodegenMetadata(MultiFile.getDescriptor().getFile()); - assertThat(descriptors).hasSize(5); - assertThat(descriptors.stream().filter(d -> d.getProtoTypeName().equals("dev.cel.testing.testdata.SingleFile")).findAny()).isPresent(); + assertThat(descriptors).hasSize(3); assertThat(descriptors.stream().filter(d -> d.getProtoTypeName().equals("dev.cel.testing.testdata.MultiFile")).findAny()).isPresent(); } + + @Test + public void collectCodegenMetadata_withProtoDependencies_doesNotContainImportedProto() { + ProtoDescriptorCollector collector = + ProtoDescriptorCollector.newInstance(DebugPrinter.newInstance(false)); + + ImmutableList descriptors = + collector.collectCodegenMetadata(MultiFile.getDescriptor().getFile()); + + assertThat(descriptors.stream().filter(d -> d.getProtoTypeName().equals("dev.cel.testing.testdata.SingleFile")).findAny()).isEmpty(); + } } diff --git a/runtime/src/test/java/dev/cel/runtime/CelLiteInterpreterTest.java b/runtime/src/test/java/dev/cel/runtime/CelLiteInterpreterTest.java index eca03005e..af802f39a 100644 --- a/runtime/src/test/java/dev/cel/runtime/CelLiteInterpreterTest.java +++ b/runtime/src/test/java/dev/cel/runtime/CelLiteInterpreterTest.java @@ -17,7 +17,7 @@ import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import dev.cel.common.values.ProtoMessageLiteValueProvider; -import dev.cel.expr.conformance.proto3.TestAllTypesProto3CelDescriptor; +import dev.cel.expr.conformance.proto3.TestAllTypesCelDescriptor; import dev.cel.extensions.CelOptionalLibrary; import dev.cel.testing.BaseInterpreterTest; import org.junit.runner.RunWith; @@ -35,7 +35,7 @@ public CelLiteInterpreterTest(@TestParameter InterpreterTestOption testOption) { CelRuntimeFactory.standardCelRuntimeBuilder() .setValueProvider( ProtoMessageLiteValueProvider.newInstance( - TestAllTypesProto3CelDescriptor.getDescriptor())) + TestAllTypesCelDescriptor.getDescriptor())) .addLibraries(CelOptionalLibrary.INSTANCE) .setOptions(testOption.celOptions.toBuilder().enableCelValue(true).build()) .build()); diff --git a/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java b/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java index 931a57b5a..6b91ba7d8 100644 --- a/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java +++ b/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java @@ -47,11 +47,10 @@ import dev.cel.common.values.ProtoMessageLiteValueProvider; import dev.cel.compiler.CelCompiler; import dev.cel.compiler.CelCompilerFactory; -import dev.cel.expr.conformance.proto2.TestAllTypesProto2CelDescriptor; import dev.cel.expr.conformance.proto3.TestAllTypes; import dev.cel.expr.conformance.proto3.TestAllTypes.NestedEnum; import dev.cel.expr.conformance.proto3.TestAllTypes.NestedMessage; -import dev.cel.expr.conformance.proto3.TestAllTypesProto3CelDescriptor; +import dev.cel.expr.conformance.proto3.TestAllTypesCelDescriptor; import dev.cel.parser.CelStandardMacro; import dev.cel.testing.testdata.MultiFile; import dev.cel.testing.testdata.MultiFileCelDescriptor; @@ -82,8 +81,8 @@ public class CelLiteRuntimeTest { .setStandardFunctions(CelStandardFunctions.newBuilder().build()) .setValueProvider( ProtoMessageLiteValueProvider.newInstance( - TestAllTypesProto2CelDescriptor.getDescriptor(), - TestAllTypesProto3CelDescriptor.getDescriptor() + dev.cel.expr.conformance.proto2.TestAllTypesCelDescriptor.getDescriptor(), + TestAllTypesCelDescriptor.getDescriptor() )) .build(); diff --git a/testing/src/test/resources/protos/BUILD.bazel b/testing/src/test/resources/protos/BUILD.bazel index cfeedcdec..f1174b0ac 100644 --- a/testing/src/test/resources/protos/BUILD.bazel +++ b/testing/src/test/resources/protos/BUILD.bazel @@ -62,7 +62,8 @@ java_lite_proto_cel_library( java_lite_proto_cel_library_impl( name = "multi_file_cel_java_proto", - java_descriptor_class_name = "MultiFileCelDescriptor", + debug = 1, + java_descriptor_class_suffix = "CelDescriptor", java_proto_library_dep = ":multi_file_java_proto", deps = [ ":multi_file_proto", @@ -72,14 +73,14 @@ java_lite_proto_cel_library_impl( java_lite_proto_cel_library_impl( name = "test_all_types_cel_java_proto2", - java_descriptor_class_name = "TestAllTypesProto2CelDescriptor", + java_descriptor_class_suffix = "CelDescriptor", java_proto_library_dep = "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto", deps = ["@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_proto"], ) java_lite_proto_cel_library_impl( name = "test_all_types_cel_java_proto3", - java_descriptor_class_name = "TestAllTypesProto3CelDescriptor", + java_descriptor_class_suffix = "CelDescriptor", java_proto_library_dep = "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto", deps = ["@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_proto"], ) From 5d40a28c2995181bda9dd95839325b95ea672140 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Thu, 8 May 2025 15:42:13 -0700 Subject: [PATCH 8/8] Include multiple java classes within the same jar --- BUILD.bazel | 2 +- .../protobuf/CelLiteDescriptorGenerator.java | 15 +++++++--- .../dev/cel/protobuf/JavaFileGenerator.java | 30 +++++++++++++------ .../dev/cel/runtime/CelLiteRuntimeTest.java | 6 +++- 4 files changed, 38 insertions(+), 15 deletions(-) diff --git a/BUILD.bazel b/BUILD.bazel index 41a15f355..06942bc50 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -151,7 +151,7 @@ java_package_configuration( "-Xep:ProtoFieldPreconditionsCheckNotNull:ERROR", "-Xep:ProtocolBufferOrdinal:ERROR", "-Xep:ReferenceEquality:ERROR", - # "-Xep:RemoveUnusedImports:ERROR", + "-Xep:RemoveUnusedImports:ERROR", "-Xep:RequiredModifiers:ERROR", "-Xep:ShortCircuitBoolean:ERROR", "-Xep:SimpleDateFormatConstant:ERROR", diff --git a/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java b/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java index bdbb440d9..f933a41d8 100644 --- a/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java +++ b/protobuf/src/main/java/dev/cel/protobuf/CelLiteDescriptorGenerator.java @@ -15,6 +15,7 @@ package dev.cel.protobuf; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.io.Files; import com.google.protobuf.DescriptorProtos.FileDescriptorProto; @@ -23,6 +24,7 @@ import com.google.protobuf.ExtensionRegistry; import dev.cel.common.CelDescriptorUtil; import dev.cel.common.internal.ProtoJavaQualifiedNames; +import dev.cel.protobuf.JavaFileGenerator.GeneratedClass; import dev.cel.protobuf.JavaFileGenerator.JavaFileGeneratorOption; import java.io.File; import java.io.IOException; @@ -77,6 +79,7 @@ final class CelLiteDescriptorGenerator implements Callable { public Integer call() throws Exception { Preconditions.checkArgument(!targetDescriptorSetPath.isEmpty()); + ImmutableList.Builder generatedClassesBuilder = ImmutableList.builder(); for (String descriptorFilePath : targetDescriptorSetPath) { debugPrinter.print("Target descriptor file path: " + descriptorFilePath); String targetDescriptorProtoPath = extractProtoPath(descriptorFilePath); @@ -100,13 +103,18 @@ public Integer call() throws Exception { targetDescriptorProtoPath)); } - codegenCelLiteDescriptor(targetFileDescriptor); + GeneratedClass generatedClass = codegenCelLiteDescriptor(targetFileDescriptor); + debugPrinter.print("Generated Class:\n" + generatedClass.code()); + + generatedClassesBuilder.add(generatedClass); } + JavaFileGenerator.writeSrcJar(outPath, generatedClassesBuilder.build()); + return 0; } - private void codegenCelLiteDescriptor(FileDescriptor targetFileDescriptor) throws Exception { + private GeneratedClass codegenCelLiteDescriptor(FileDescriptor targetFileDescriptor) throws Exception { String javaPackageName = ProtoJavaQualifiedNames.getJavaPackageName(targetFileDescriptor); String javaClassName; @@ -132,8 +140,7 @@ private void codegenCelLiteDescriptor(FileDescriptor targetFileDescriptor) throw String.format( "Fully qualified descriptor java class name: %s.%s", javaPackageName, javaClassName)); - JavaFileGenerator.createFile( - outPath, + return JavaFileGenerator.generateClass( JavaFileGeneratorOption.newBuilder() .setVersion(version) .setDescriptorClassName(javaClassName) diff --git a/protobuf/src/main/java/dev/cel/protobuf/JavaFileGenerator.java b/protobuf/src/main/java/dev/cel/protobuf/JavaFileGenerator.java index eaaff482e..4fbb3e66f 100644 --- a/protobuf/src/main/java/dev/cel/protobuf/JavaFileGenerator.java +++ b/protobuf/src/main/java/dev/cel/protobuf/JavaFileGenerator.java @@ -32,6 +32,7 @@ import java.io.InputStream; import java.io.StringWriter; import java.io.Writer; +import java.util.Collection; import java.util.Locale; import java.util.zip.ZipEntry; import java.util.zip.ZipOutputStream; @@ -40,7 +41,7 @@ final class JavaFileGenerator { private static final String HELPER_CLASS_TEMPLATE_FILE = "cel_lite_descriptor_template.txt"; - public static void createFile(String filePath, JavaFileGeneratorOption option) + static GeneratedClass generateClass(JavaFileGeneratorOption option) throws IOException, TemplateException { Version version = Configuration.VERSION_2_3_32; Configuration cfg = new Configuration(version); @@ -57,28 +58,39 @@ public static void createFile(String filePath, JavaFileGeneratorOption option) Writer out = new StringWriter(); template.process(option.getTemplateMap(), out); - writeSrcJar(filePath, option.descriptorClassName(), out.toString()); + return GeneratedClass.create(option.descriptorClassName(), out.toString()); } - private static void writeSrcJar( - String srcjarFilePath, String javaClassName, String javaClassContent) throws IOException { - System.out.println(javaClassContent); + static void writeSrcJar( + String srcjarFilePath, Collection generatedClasses) throws IOException { if (!srcjarFilePath.toLowerCase(Locale.getDefault()).endsWith(".srcjar")) { throw new IllegalArgumentException("File must end with .srcjar, provided: " + srcjarFilePath); } try (FileOutputStream fos = new FileOutputStream(srcjarFilePath); ZipOutputStream zos = new ZipOutputStream(fos)) { - ZipEntry entry = new ZipEntry(javaClassName + ".java"); - zos.putNextEntry(entry); + for (GeneratedClass generatedClass : generatedClasses) { + ZipEntry entry = new ZipEntry(generatedClass.className() + ".java"); + zos.putNextEntry(entry); - try (InputStream inputStream = new ByteArrayInputStream(javaClassContent.getBytes(UTF_8))) { - ByteStreams.copy(inputStream, zos); + try (InputStream inputStream = new ByteArrayInputStream(generatedClass.code().getBytes(UTF_8))) { + ByteStreams.copy(inputStream, zos); + } } zos.closeEntry(); } } + @AutoValue + abstract static class GeneratedClass { + abstract String className(); + abstract String code(); + + static GeneratedClass create(String className, String code) { + return new AutoValue_JavaFileGenerator_GeneratedClass(className, code); + } + } + @AutoValue abstract static class JavaFileGeneratorOption { abstract String packageName(); diff --git a/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java b/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java index 6b91ba7d8..9bab34c94 100644 --- a/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java +++ b/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeTest.java @@ -54,6 +54,7 @@ import dev.cel.parser.CelStandardMacro; import dev.cel.testing.testdata.MultiFile; import dev.cel.testing.testdata.MultiFileCelDescriptor; +import dev.cel.testing.testdata.SingleFileCelDescriptor; import dev.cel.testing.testdata.SingleFileProto.SingleFile; import java.util.ArrayList; import java.util.Collections; @@ -587,7 +588,10 @@ public void nestedMessage_fromImportedProto() throws Exception { CelLiteRuntimeFactory.newLiteRuntimeBuilder() .setStandardFunctions(CelStandardFunctions.newBuilder().build()) .setValueProvider( - ProtoMessageLiteValueProvider.newInstance(MultiFileCelDescriptor.getDescriptor())) + ProtoMessageLiteValueProvider.newInstance( + SingleFileCelDescriptor.getDescriptor(), + MultiFileCelDescriptor.getDescriptor()) + ) .build(); CelAbstractSyntaxTree ast = celCompiler.compile("multiFile.nested_single_file.name").getAst();