Skip to content

Commit 3fa45d3

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Fix FileDescriptorSetConverter to always reference WellKnownTypes descriptors from generated ones
PiperOrigin-RevId: 815892539
1 parent 1fc8922 commit 3fa45d3

21 files changed

Lines changed: 305 additions & 40 deletions

File tree

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ java_library(
2525
"//checker:checker_legacy_environment",
2626
"//checker:proto_type_mask",
2727
"//common:cel_ast",
28+
"//common:cel_descriptor_util",
2829
"//common:cel_source",
2930
"//common:compiler_common",
3031
"//common:container",

‎bundle/src/test/java/dev/cel/bundle/CelImplTest.java‎

Lines changed: 61 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import com.google.protobuf.ByteString;
4444
import com.google.protobuf.DescriptorProtos.FileDescriptorProto;
4545
import com.google.protobuf.DescriptorProtos.FileDescriptorSet;
46+
import com.google.protobuf.Descriptors.Descriptor;
4647
import com.google.protobuf.Descriptors.FileDescriptor;
4748
import com.google.protobuf.Duration;
4849
import com.google.protobuf.Empty;
@@ -51,6 +52,7 @@
5152
import com.google.protobuf.Struct;
5253
import com.google.protobuf.TextFormat;
5354
import com.google.protobuf.Timestamp;
55+
import com.google.protobuf.TypeRegistry;
5456
import com.google.protobuf.WrappersProto;
5557
import com.google.rpc.context.AttributeContext;
5658
import com.google.testing.junit.testparameterinjector.TestParameter;
@@ -62,6 +64,7 @@
6264
import dev.cel.checker.TypeProvider;
6365
import dev.cel.common.CelAbstractSyntaxTree;
6466
import dev.cel.common.CelContainer;
67+
import dev.cel.common.CelDescriptorUtil;
6568
import dev.cel.common.CelErrorCode;
6669
import dev.cel.common.CelIssue;
6770
import dev.cel.common.CelOptions;
@@ -91,6 +94,7 @@
9194
import dev.cel.compiler.CelCompilerFactory;
9295
import dev.cel.compiler.CelCompilerImpl;
9396
import dev.cel.expr.conformance.proto2.Proto2ExtensionScopedMessage;
97+
import dev.cel.expr.conformance.proto2.TestAllTypesExtensions;
9498
import dev.cel.expr.conformance.proto3.TestAllTypes;
9599
import dev.cel.parser.CelParserImpl;
96100
import dev.cel.parser.CelStandardMacro;
@@ -196,13 +200,11 @@ public void build_badFileDescriptorSet() {
196200
IllegalArgumentException.class,
197201
() ->
198202
standardCelBuilderWithMacros()
199-
.setContainer(CelContainer.ofName("google.rpc.context.AttributeContext"))
203+
.setContainer(CelContainer.ofName("cel.expr.conformance.proto2"))
200204
.addFileTypes(
201205
FileDescriptorSet.newBuilder()
202-
.addFile(AttributeContext.getDescriptor().getFile().toProto())
206+
.addFile(TestAllTypesExtensions.getDescriptor().getFile().toProto())
203207
.build())
204-
.setProtoResultType(
205-
CelProtoTypes.createMessage("google.rpc.context.AttributeContext.Resource"))
206208
.build());
207209
assertThat(e).hasMessageThat().contains("file descriptor set with unresolved proto file");
208210
}
@@ -2124,6 +2126,61 @@ public void program_evaluateCanonicalTypesToNativeTypesDisabled_producesBytesPro
21242126
assertThat(result).isEqualTo(ByteString.copyFromUtf8("abc"));
21252127
}
21262128

2129+
@Test
2130+
public void textProto_containsWktDependency_descriptorInstancesMatch() throws Exception {
2131+
Cel cel =
2132+
standardCelBuilderWithMacros()
2133+
.addMessageTypes(TestAllTypes.getDescriptor())
2134+
.setContainer(CelContainer.ofName("cel.expr.conformance.proto3"))
2135+
.build();
2136+
CelAbstractSyntaxTree ast =
2137+
cel.compile("TestAllTypes{single_bytes: bytes('abc')}.single_bytes").getAst();
2138+
2139+
ByteString result = (ByteString) cel.createProgram(ast).eval();
2140+
2141+
assertThat(result).isEqualTo(ByteString.copyFromUtf8("abc"));
2142+
}
2143+
2144+
@Test
2145+
public void program_fdsContainsWktDependency_descriptorInstancesMatch() throws Exception {
2146+
// Force serialization of the descriptor to get a unique instance
2147+
FileDescriptorProto proto = TestAllTypes.getDescriptor().getFile().toProto();
2148+
FileDescriptorSet fds = FileDescriptorSet.newBuilder().addFile(proto).build();
2149+
ImmutableSet<FileDescriptor> fileDescriptors =
2150+
CelDescriptorUtil.getFileDescriptorsFromFileDescriptorSet(fds);
2151+
ImmutableSet<Descriptor> descriptors =
2152+
CelDescriptorUtil.getAllDescriptorsFromFileDescriptor(fileDescriptors)
2153+
.messageTypeDescriptors();
2154+
// Parse text proto using this fds
2155+
String textProto =
2156+
"single_timestamp {\n" //
2157+
+ " seconds: 100\n" //
2158+
+ "}";
2159+
TypeRegistry typeRegistry = TypeRegistry.newBuilder().add(descriptors).build();
2160+
TestAllTypes.Builder testAllTypesBuilder = TestAllTypes.newBuilder();
2161+
TextFormat.Parser textFormatParser =
2162+
TextFormat.Parser.newBuilder().setTypeRegistry(typeRegistry).build();
2163+
textFormatParser.merge(textProto, testAllTypesBuilder);
2164+
TestAllTypes testAllTypesFromTextProto = testAllTypesBuilder.build();
2165+
// Evaluate a timestamp message using the same descriptor
2166+
Cel cel =
2167+
standardCelBuilderWithMacros()
2168+
.addMessageTypes(descriptors)
2169+
.setOptions(
2170+
CelOptions.current()
2171+
.evaluateCanonicalTypesToNativeValues(true)
2172+
.enableTimestampEpoch(true)
2173+
.build())
2174+
.setContainer(CelContainer.ofName("cel.expr.conformance.proto3"))
2175+
.build();
2176+
CelAbstractSyntaxTree ast =
2177+
cel.compile("TestAllTypes{single_timestamp: timestamp(100)}").getAst();
2178+
2179+
TestAllTypes evalResult = (TestAllTypes) cel.createProgram(ast).eval();
2180+
2181+
assertThat(evalResult).isEqualTo(testAllTypesFromTextProto);
2182+
}
2183+
21272184
@Test
21282185
public void toBuilder_isImmutable() {
21292186
CelBuilder celBuilder = CelFactory.standardCelBuilder();

‎checker/src/main/java/dev/cel/checker/BUILD.bazel‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ java_library(
7373
":type_provider_legacy_impl",
7474
"//:auto_value",
7575
"//common:cel_ast",
76-
"//common:cel_descriptors",
76+
"//common:cel_descriptor_util",
7777
"//common:cel_source",
7878
"//common:compiler_common",
7979
"//common:container",

‎common/BUILD.bazel‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,5 +106,18 @@ java_library(
106106

107107
java_library(
108108
name = "cel_descriptors",
109+
visibility = ["//:internal"],
109110
exports = ["//common/src/main/java/dev/cel/common:cel_descriptors"],
110111
)
112+
113+
java_library(
114+
name = "cel_descriptor_util",
115+
visibility = [
116+
"//:internal",
117+
# TODO: Remove references to the following clients
118+
"//java/com/google/abuse/admin/notebook/compiler/checkedtypes:__pkg__",
119+
"//java/com/google/paymentfraud/v2/util/featurereplay/common/risklogrecordio:__pkg__",
120+
"//java/com/google/payments/consumer/growth/treatmentconfig/management/backend/service/config/utils:__pkg__",
121+
],
122+
exports = ["//common/src/main/java/dev/cel/common:cel_descriptor_util"],
123+
)

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

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,18 +36,31 @@ PROTO_V1ALPHA1_AST_SOURCE = [
3636
]
3737

3838
java_library(
39-
name = "cel_descriptors",
39+
name = "cel_descriptor_util",
4040
srcs = [
4141
"CelDescriptorUtil.java",
42-
"CelDescriptors.java",
4342
],
4443
tags = [
4544
],
4645
deps = [
47-
"//:auto_value",
46+
":cel_descriptors",
4847
"//common/annotations",
4948
"//common/internal:file_descriptor_converter",
5049
"//common/types:cel_types",
50+
"@maven//:com_google_guava_guava",
51+
"@maven//:com_google_protobuf_protobuf_java",
52+
],
53+
)
54+
55+
java_library(
56+
name = "cel_descriptors",
57+
srcs = [
58+
"CelDescriptors.java",
59+
],
60+
tags = [
61+
],
62+
deps = [
63+
"//:auto_value",
5164
"@maven//:com_google_errorprone_error_prone_annotations",
5265
"@maven//:com_google_guava_guava",
5366
"@maven//:com_google_protobuf_protobuf_java",

‎common/src/main/java/dev/cel/common/CelDescriptorUtil.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,10 +166,12 @@ private static void collectMessageTypeDescriptors(
166166
if (visited.contains(messageName)) {
167167
return;
168168
}
169+
169170
if (!descriptor.getOptions().getMapEntry()) {
170171
visited.add(messageName);
171172
celDescriptors.addMessageTypeDescriptors(descriptor);
172173
}
174+
173175
if (CelTypes.getWellKnownCelType(messageName).isPresent()) {
174176
return;
175177
}
@@ -234,6 +236,7 @@ private static void copyToFileDescriptorSet(
234236
if (visited.contains(fd.getFullName())) {
235237
return;
236238
}
239+
237240
visited.add(fd.getFullName());
238241
for (FileDescriptor dep : fd.getDependencies()) {
239242
copyToFileDescriptorSet(visited, dep, files);

‎common/src/main/java/dev/cel/common/internal/BUILD.bazel‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,9 @@ java_library(
232232
tags = [
233233
],
234234
deps = [
235+
":cel_descriptor_pools",
235236
"//:auto_value",
237+
"//common/internal:well_known_proto",
236238
"@maven//:com_google_errorprone_error_prone_annotations",
237239
"@maven//:com_google_guava_guava",
238240
"@maven//:com_google_protobuf_protobuf_java",

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,10 @@ public static DefaultDescriptorPool create(
119119
extensionRegistry);
120120
}
121121

122+
public static Descriptor getWellKnownProtoDescriptor(WellKnownProto wellKnownProto) {
123+
return WELL_KNOWN_PROTO_TO_DESCRIPTORS.get(wellKnownProto);
124+
}
125+
122126
@Override
123127
public Optional<Descriptor> findDescriptor(String name) {
124128
return Optional.ofNullable(descriptorMap.get(name));

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package dev.cel.common.internal;
1616

1717
import com.google.common.base.VerifyException;
18+
import com.google.common.collect.ImmutableCollection;
1819
import com.google.common.collect.ImmutableSet;
1920
import com.google.errorprone.annotations.CanIgnoreReturnValue;
2021
import com.google.errorprone.annotations.CheckReturnValue;
@@ -76,7 +77,15 @@ private static FileDescriptor readDescriptor(
7677
// Read dependencies first, they are needed to create the logical descriptor from the proto.
7778
List<FileDescriptor> deps = new ArrayList<>();
7879
for (String dep : fileProto.getDependencyList()) {
79-
deps.add(readDescriptor(dep, descriptorProtos, descriptors));
80+
ImmutableCollection<WellKnownProto> wktProtos = WellKnownProto.getByPathName(dep);
81+
if (wktProtos.isEmpty()) {
82+
deps.add(readDescriptor(dep, descriptorProtos, descriptors));
83+
} else {
84+
// Ensure the generated message's descriptor is used as a dependency for WKTs to avoid
85+
// issues with descriptor instance mismatch.
86+
WellKnownProto wellKnownProto = wktProtos.iterator().next();
87+
deps.add(DefaultDescriptorPool.getWellKnownProtoDescriptor(wellKnownProto).getFile());
88+
}
8089
}
8190
// Create the file descriptor, cache, and return.
8291
try {

0 commit comments

Comments
 (0)