Skip to content

Commit 91248e0

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 91248e0

21 files changed

Lines changed: 290 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: 46 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,46 @@ public void program_evaluateCanonicalTypesToNativeTypesDisabled_producesBytesPro
21242126
assertThat(result).isEqualTo(ByteString.copyFromUtf8("abc"));
21252127
}
21262128

2129+
@Test
2130+
public void program_fdsContainsWktDependency_descriptorInstancesMatch() throws Exception {
2131+
// Force serialization of the descriptor to get a unique instance
2132+
FileDescriptorProto proto = TestAllTypes.getDescriptor().getFile().toProto();
2133+
FileDescriptorSet fds = FileDescriptorSet.newBuilder().addFile(proto).build();
2134+
ImmutableSet<FileDescriptor> fileDescriptors =
2135+
CelDescriptorUtil.getFileDescriptorsFromFileDescriptorSet(fds);
2136+
ImmutableSet<Descriptor> descriptors =
2137+
CelDescriptorUtil.getAllDescriptorsFromFileDescriptor(fileDescriptors)
2138+
.messageTypeDescriptors();
2139+
// Parse text proto using this fds
2140+
String textProto =
2141+
"single_timestamp {\n" //
2142+
+ " seconds: 100\n" //
2143+
+ "}";
2144+
TypeRegistry typeRegistry = TypeRegistry.newBuilder().add(descriptors).build();
2145+
TestAllTypes.Builder testAllTypesBuilder = TestAllTypes.newBuilder();
2146+
TextFormat.Parser textFormatParser =
2147+
TextFormat.Parser.newBuilder().setTypeRegistry(typeRegistry).build();
2148+
textFormatParser.merge(textProto, testAllTypesBuilder);
2149+
TestAllTypes testAllTypesFromTextProto = testAllTypesBuilder.build();
2150+
// Evaluate a timestamp message using the same descriptor
2151+
Cel cel =
2152+
standardCelBuilderWithMacros()
2153+
.addMessageTypes(descriptors)
2154+
.setOptions(
2155+
CelOptions.current()
2156+
.evaluateCanonicalTypesToNativeValues(true)
2157+
.enableTimestampEpoch(true)
2158+
.build())
2159+
.setContainer(CelContainer.ofName("cel.expr.conformance.proto3"))
2160+
.build();
2161+
CelAbstractSyntaxTree ast =
2162+
cel.compile("TestAllTypes{single_timestamp: timestamp(100)}").getAst();
2163+
2164+
TestAllTypes evalResult = (TestAllTypes) cel.createProgram(ast).eval();
2165+
2166+
assertThat(evalResult).isEqualTo(testAllTypesFromTextProto);
2167+
}
2168+
21272169
@Test
21282170
public void toBuilder_isImmutable() {
21292171
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 {

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

Lines changed: 93 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@
1717
import static com.google.common.collect.ImmutableMap.toImmutableMap;
1818
import static java.util.Arrays.stream;
1919

20+
import com.google.common.collect.ImmutableCollection;
2021
import com.google.common.collect.ImmutableMap;
22+
import com.google.common.collect.ImmutableMultimap;
2123
import com.google.protobuf.Any;
2224
import com.google.protobuf.BoolValue;
2325
import com.google.protobuf.BytesValue;
@@ -46,28 +48,71 @@
4648
*/
4749
@Internal
4850
public enum WellKnownProto {
49-
ANY_VALUE("google.protobuf.Any", Any.class),
50-
DURATION("google.protobuf.Duration", Duration.class),
51-
JSON_LIST_VALUE("google.protobuf.ListValue", ListValue.class),
52-
JSON_STRUCT_VALUE("google.protobuf.Struct", Struct.class),
53-
JSON_VALUE("google.protobuf.Value", Value.class),
54-
TIMESTAMP("google.protobuf.Timestamp", Timestamp.class),
51+
ANY_VALUE("google.protobuf.Any", "google/protobuf/any.proto", Any.class),
52+
DURATION("google.protobuf.Duration", "google/protobuf/duration.proto", Duration.class),
53+
JSON_LIST_VALUE("google.protobuf.ListValue", "google/protobuf/struct.proto", ListValue.class),
54+
JSON_STRUCT_VALUE("google.protobuf.Struct", "google/protobuf/struct.proto", Struct.class),
55+
JSON_VALUE("google.protobuf.Value", "google/protobuf/struct.proto", Value.class),
56+
TIMESTAMP("google.protobuf.Timestamp", "google/protobuf/timestamp.proto", Timestamp.class),
5557
// Wrapper types
56-
FLOAT_VALUE("google.protobuf.FloatValue", FloatValue.class, /* isWrapperType= */ true),
57-
INT32_VALUE("google.protobuf.Int32Value", Int32Value.class, /* isWrapperType= */ true),
58-
INT64_VALUE("google.protobuf.Int64Value", Int64Value.class, /* isWrapperType= */ true),
59-
STRING_VALUE("google.protobuf.StringValue", StringValue.class, /* isWrapperType= */ true),
60-
BOOL_VALUE("google.protobuf.BoolValue", BoolValue.class, /* isWrapperType= */ true),
61-
BYTES_VALUE("google.protobuf.BytesValue", BytesValue.class, /* isWrapperType= */ true),
62-
DOUBLE_VALUE("google.protobuf.DoubleValue", DoubleValue.class, /* isWrapperType= */ true),
63-
UINT32_VALUE("google.protobuf.UInt32Value", UInt32Value.class, /* isWrapperType= */ true),
64-
UINT64_VALUE("google.protobuf.UInt64Value", UInt64Value.class, /* isWrapperType= */ true),
58+
FLOAT_VALUE(
59+
"google.protobuf.FloatValue",
60+
"google/protobuf/wrappers.proto",
61+
FloatValue.class,
62+
/* isWrapperType= */ true),
63+
INT32_VALUE(
64+
"google.protobuf.Int32Value",
65+
"google/protobuf/wrappers.proto",
66+
Int32Value.class,
67+
/* isWrapperType= */ true),
68+
INT64_VALUE(
69+
"google.protobuf.Int64Value",
70+
"google/protobuf/wrappers.proto",
71+
Int64Value.class,
72+
/* isWrapperType= */ true),
73+
STRING_VALUE(
74+
"google.protobuf.StringValue",
75+
"google/protobuf/wrappers.proto",
76+
StringValue.class,
77+
/* isWrapperType= */ true),
78+
BOOL_VALUE(
79+
"google.protobuf.BoolValue",
80+
"google/protobuf/wrappers.proto",
81+
BoolValue.class,
82+
/* isWrapperType= */ true),
83+
BYTES_VALUE(
84+
"google.protobuf.BytesValue",
85+
"google/protobuf/wrappers.proto",
86+
BytesValue.class,
87+
/* isWrapperType= */ true),
88+
DOUBLE_VALUE(
89+
"google.protobuf.DoubleValue",
90+
"google/protobuf/wrappers.proto",
91+
DoubleValue.class,
92+
/* isWrapperType= */ true),
93+
UINT32_VALUE(
94+
"google.protobuf.UInt32Value",
95+
"google/protobuf/wrappers.proto",
96+
UInt32Value.class,
97+
/* isWrapperType= */ true),
98+
UINT64_VALUE(
99+
"google.protobuf.UInt64Value",
100+
"google/protobuf/wrappers.proto",
101+
UInt64Value.class,
102+
/* isWrapperType= */ true),
65103
// These aren't explicitly called out as wrapper types in the spec, but behave like one, because
66104
// they are still converted into an equivalent primitive type.
67105

68-
EMPTY("google.protobuf.Empty", Empty.class, /* isWrapperType= */ true),
69-
FIELD_MASK("google.protobuf.FieldMask", FieldMask.class, /* isWrapperType= */ true),
70-
;
106+
EMPTY(
107+
"google.protobuf.Empty",
108+
"google/protobuf/empty.proto",
109+
Empty.class,
110+
/* isWrapperType= */ true),
111+
FIELD_MASK(
112+
"google.protobuf.FieldMask",
113+
"google/protobuf/field_mask.proto",
114+
FieldMask.class,
115+
/* isWrapperType= */ true);
71116

72117
private static final ImmutableMap<String, WellKnownProto> TYPE_NAME_TO_WELL_KNOWN_PROTO_MAP =
73118
stream(WellKnownProto.values())
@@ -78,10 +123,27 @@ public enum WellKnownProto {
78123
stream(WellKnownProto.values())
79124
.collect(toImmutableMap(WellKnownProto::messageClass, Function.identity()));
80125

126+
private static final ImmutableMultimap<String, WellKnownProto> PATH_NAME_TO_WELL_KNOWN_PROTO_MAP =
127+
initPathNameMap();
128+
129+
private static ImmutableMultimap<String, WellKnownProto> initPathNameMap() {
130+
ImmutableMultimap.Builder<String, WellKnownProto> builder = ImmutableMultimap.builder();
131+
for (WellKnownProto proto : values()) {
132+
builder.put(proto.pathName(), proto);
133+
}
134+
return builder.build();
135+
}
136+
81137
private final String wellKnownProtoTypeName;
138+
private final String pathName;
82139
private final Class<?> clazz;
83140
private final boolean isWrapperType;
84141

142+
/** Gets the full proto path name (ex: google/protobuf/any.proto) */
143+
public String pathName() {
144+
return pathName;
145+
}
146+
85147
/** Gets the fully qualified prototype name (ex: google.protobuf.FloatValue) */
86148
public String typeName() {
87149
return wellKnownProtoTypeName;
@@ -92,6 +154,14 @@ public Class<?> messageClass() {
92154
return clazz;
93155
}
94156

157+
/**
158+
* Returns the well known proto given the full proto path (example:
159+
* google/protobuf/timestamp.proto)
160+
*/
161+
public static ImmutableCollection<WellKnownProto> getByPathName(String typeName) {
162+
return PATH_NAME_TO_WELL_KNOWN_PROTO_MAP.get(typeName);
163+
}
164+
95165
public static Optional<WellKnownProto> getByTypeName(String typeName) {
96166
return Optional.ofNullable(TYPE_NAME_TO_WELL_KNOWN_PROTO_MAP.get(typeName));
97167
}
@@ -112,12 +182,14 @@ public boolean isWrapperType() {
112182
return isWrapperType;
113183
}
114184

115-
WellKnownProto(String wellKnownProtoTypeName, Class<?> clazz) {
116-
this(wellKnownProtoTypeName, clazz, /* isWrapperType= */ false);
185+
WellKnownProto(String wellKnownProtoTypeName, String pathName, Class<?> clazz) {
186+
this(wellKnownProtoTypeName, pathName, clazz, /* isWrapperType= */ false);
117187
}
118188

119-
WellKnownProto(String wellKnownProtoFullName, Class<?> clazz, boolean isWrapperType) {
189+
WellKnownProto(
190+
String wellKnownProtoFullName, String pathName, Class<?> clazz, boolean isWrapperType) {
120191
this.wellKnownProtoTypeName = wellKnownProtoFullName;
192+
this.pathName = pathName;
121193
this.clazz = clazz;
122194
this.isWrapperType = isWrapperType;
123195
}

0 commit comments

Comments
 (0)