Skip to content

Commit 8d51e34

Browse files
mkruskal-googlecopybara-github
authored andcommitted
Fix handling of optional dependencies in java generator.
Java reflection is used to fail smoothly when the extensions aren't found in the classpath. PiperOrigin-RevId: 805477788
1 parent c3f7e8d commit 8d51e34

File tree

4 files changed

+96
-42
lines changed

4 files changed

+96
-42
lines changed
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package com.google.protobuf;
2+
3+
/**
4+
* All generated outer classes extend this class. This class implements shared logic that should
5+
* only be called by generated code.
6+
*
7+
* <p>Users should generally ignore this class and use the generated Outer class directly instead.
8+
*
9+
* <p>This class is intended to only be extended by protoc created gencode. It is not intended or
10+
* supported to extend this class, and any protected methods may be removed without it being
11+
* considered a breaking change as long as all supported gencode does not depend on the changed
12+
* methods.
13+
*/
14+
public abstract class GeneratedFile {
15+
16+
protected GeneratedFile() {}
17+
18+
/** Add an optional extension from a file that may or may not be in the classpath. */
19+
protected static void addOptionalExtension(
20+
ExtensionRegistry registry, final String className, final String fieldName) {
21+
try {
22+
GeneratedMessage.GeneratedExtension<?, ?> ext =
23+
(GeneratedMessage.GeneratedExtension<?, ?>)
24+
Class.forName(className).getField(fieldName).get(null);
25+
registry.add(ext);
26+
} catch (ClassNotFoundException e) {
27+
// ignore missing class if it's not in the classpath.
28+
} catch (NoSuchFieldException e) {
29+
// ignore missing field if it's not in the classpath.
30+
} catch (IllegalAccessException e) {
31+
// ignore malformed field if it's not what we expect.
32+
}
33+
}
34+
}

‎java/core/src/test/java/com/google/protobuf/ImportOptionTest.java‎

Lines changed: 14 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,6 @@ public final class ImportOptionTest {
1818

1919
@Test
2020
public void testImportOption() throws Exception {
21-
// Ensure that UnittestCustomOptions is linked in and referenced.
22-
FileDescriptor unused = UnittestCustomOptions.getDescriptor();
23-
2421
FileDescriptor fileDescriptor = UnittestImportOptionProto.getDescriptor();
2522
Descriptor messageDescriptor = TestMessage.getDescriptor();
2623
FieldDescriptor fieldDescriptor = messageDescriptor.findFieldByName("field1");
@@ -29,26 +26,19 @@ public void testImportOption() throws Exception {
2926
UnknownFieldSet unknownFieldsMessage = messageDescriptor.getOptions().getUnknownFields();
3027
UnknownFieldSet unknownFieldsField = fieldDescriptor.getOptions().getUnknownFields();
3128

32-
// TODO: Currently linked in options also end up in unknown fields.
33-
// TODO: Exclude for open source tests once linked in options are treated
34-
// differently, since `option_deps` are treated as `deps` in Bazel 7.
35-
// assertThat(fileDescriptor.getOptions().getExtension(UnittestCustomOptions.fileOpt1))
36-
// .isEqualTo(1);
37-
// assertThat(messageDescriptor.getOptions().getExtension(UnittestCustomOptions.messageOpt1))
38-
// .isEqualTo(2);
39-
// assertThat(fieldDescriptor.getOptions().getExtension(UnittestCustomOptions.fieldOpt1))
40-
// .isEqualTo(3);
41-
assertThat(unknownFieldsFile.getField(7736974).getVarintList()).containsExactly(1L);
42-
assertThat(unknownFieldsMessage.getField(7739036).getVarintList()).containsExactly(2L);
43-
assertThat(unknownFieldsField.getField(7740936).getFixed64List()).containsExactly(3L);
44-
45-
// Options from import option that are not linked in should be in unknown fields.
46-
assertThat(unknownFieldsFile.getField(7736975).getVarintList()).containsExactly(1L);
47-
assertThat(unknownFieldsMessage.getField(7739037).getVarintList()).containsExactly(2L);
48-
assertThat(unknownFieldsField.getField(7740937).getFixed64List()).containsExactly(3L);
49-
50-
assertThat(unknownFieldsFile.asMap()).hasSize(2);
51-
assertThat(unknownFieldsMessage.asMap()).hasSize(2);
52-
assertThat(unknownFieldsField.asMap()).hasSize(2);
29+
// Using the extension ensures UnittestCustomOptions is linked in and referenced
30+
assertThat(fileDescriptor.getOptions().getExtension(UnittestCustomOptions.fileOpt1))
31+
.isEqualTo(1);
32+
assertThat(messageDescriptor.getOptions().getExtension(UnittestCustomOptions.messageOpt1))
33+
.isEqualTo(2);
34+
assertThat(fieldDescriptor.getOptions().getExtension(UnittestCustomOptions.fieldOpt1))
35+
.isEqualTo(3);
36+
37+
// TODO: Since `option_deps` are treated as `deps` in Bazel 7, the unknown
38+
// fields will be empty. Once we drop Bazel 7 support we can test that these are filled with
39+
// unknown fields.
40+
assertThat(unknownFieldsFile.asMap().size()).isAtMost(1);
41+
assertThat(unknownFieldsMessage.asMap().size()).isAtMost(1);
42+
assertThat(unknownFieldsField.asMap().size()).isAtMost(1);
5343
}
5444
}

‎src/google/protobuf/compiler/java/file.cc‎

Lines changed: 46 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
#include "google/protobuf/compiler/java/internal_helpers.h"
3232
#include "google/protobuf/compiler/java/lite/generator_factory.h"
3333
#include "google/protobuf/compiler/java/name_resolver.h"
34+
#include "google/protobuf/compiler/java/names.h"
3435
#include "google/protobuf/compiler/java/options.h"
3536
#include "google/protobuf/compiler/java/shared_code_generator.h"
3637
#include "google/protobuf/compiler/retention.h"
@@ -116,8 +117,9 @@ void CollectPublicDependencies(
116117

117118
// Finds all extensions for custom options in the given file descriptor with the
118119
// builder pool which resolves Java features instead of the generated pool.
119-
void CollectExtensions(const FileDescriptor& file,
120-
FieldDescriptorSet* extensions) {
120+
void CollectExtensions(const FileDescriptor& file, const Options& options,
121+
FieldDescriptorSet* extensions,
122+
FieldDescriptorSet* optional_extensions) {
121123
FileDescriptorProto file_proto = StripSourceRetentionOptions(file);
122124
std::string file_data;
123125
file_proto.SerializeToString(&file_data);
@@ -139,15 +141,27 @@ void CollectExtensions(const FileDescriptor& file,
139141
// Unknown extensions are ok and expected in the case of option imports.
140142
CollectExtensions(*dynamic_file_proto, extensions);
141143

142-
// TODO Check against dependencies to remove option dependencies
143-
// polluting the pool. These will be handled as optional dependencies.
144+
if (options.strip_nonfunctional_codegen) {
145+
// Skip feature extensions, which are a visible (but non-functional)
146+
// deviation between editions and legacy syntax.
147+
absl::erase_if(*extensions, [](const FieldDescriptor* field) {
148+
return field->containing_type()->full_name() == "google.protobuf.FeatureSet";
149+
});
150+
}
151+
152+
// Check against dependencies to handle option dependencies polluting pool.
144153
absl::flat_hash_set<const FileDescriptor*> dependencies;
145154
dependencies.insert(&file);
146155
for (int i = 0; i < file.dependency_count(); i++) {
147156
CollectPublicDependencies(file.dependency(i), &dependencies);
148157
}
158+
for (auto* extension : *extensions) {
159+
if (!dependencies.contains(extension->file())) {
160+
optional_extensions->insert(extension);
161+
}
162+
}
149163
absl::erase_if(*extensions, [&](const FieldDescriptor* fieldDescriptor) {
150-
return !dependencies.contains(fieldDescriptor->file());
164+
return optional_extensions->contains(fieldDescriptor);
151165
});
152166
}
153167

@@ -325,11 +339,14 @@ void FileGenerator::Generate(io::Printer* printer) {
325339
printer->Print("@com.google.protobuf.Internal.ProtoNonnullApi\n");
326340
}
327341
printer->Print(
328-
"$deprecation$public final class $classname$ {\n"
342+
"$deprecation$public final class $classname$ $extends${\n"
329343
" private $ctor$() {}\n",
330344
"deprecation",
331345
file_->options().deprecated() ? "@java.lang.Deprecated " : "",
332-
"classname", classname_, "ctor", classname_);
346+
"classname", classname_, "ctor", classname_, "extends",
347+
HasDescriptorMethods(file_, context_->EnforceLite())
348+
? "extends com.google.protobuf.GeneratedFile "
349+
: "");
333350
printer->Annotate("classname", file_->name());
334351
printer->Indent();
335352

@@ -507,15 +524,8 @@ void FileGenerator::GenerateDescriptorInitializationCodeForImmutable(
507524
// of the FileDescriptor based on the builder-pool, then we can use
508525
// reflections to find all extension fields
509526
FieldDescriptorSet extensions;
510-
CollectExtensions(*file_, &extensions);
511-
512-
if (options_.strip_nonfunctional_codegen) {
513-
// Skip feature extensions, which are a visible (but non-functional)
514-
// deviation between editions and legacy syntax.
515-
absl::erase_if(extensions, [](const FieldDescriptor* field) {
516-
return field->containing_type()->full_name() == "google.protobuf.FeatureSet";
517-
});
518-
}
527+
FieldDescriptorSet optional_extensions;
528+
CollectExtensions(*file_, options_, &extensions, &optional_extensions);
519529

520530
// Force descriptor initialization of all dependencies.
521531
for (int i = 0; i < file_->dependency_count(); i++) {
@@ -527,7 +537,7 @@ void FileGenerator::GenerateDescriptorInitializationCodeForImmutable(
527537
}
528538
}
529539

530-
if (!extensions.empty()) {
540+
if (!extensions.empty() || !optional_extensions.empty()) {
531541
// Must construct an ExtensionRegistry containing all existing extensions
532542
// and use it to parse the descriptor data again to recognize extensions.
533543
printer->Print(
@@ -544,6 +554,25 @@ void FileGenerator::GenerateDescriptorInitializationCodeForImmutable(
544554
"private static void _clinit_autosplit_dinit_$method_num$(\n"
545555
" com.google.protobuf.ExtensionRegistry registry) {\n");
546556
}
557+
for (const FieldDescriptor* field : optional_extensions) {
558+
std::unique_ptr<ExtensionGenerator> generator(
559+
generator_factory_->NewExtensionGenerator(field));
560+
printer->Emit({{"scope", field->extension_scope() != nullptr
561+
? name_resolver_->GetImmutableClassName(
562+
field->extension_scope())
563+
: name_resolver_->GetImmutableClassName(
564+
field->file())},
565+
{"name", UnderscoresToCamelCaseCheckReserved(field)}},
566+
R"java(
567+
addOptionalExtension(registry, "$scope$", "$name$");
568+
)java");
569+
bytecode_estimate += 8;
570+
MaybeRestartJavaMethod(
571+
printer, &bytecode_estimate, &method_num,
572+
"_clinit_autosplit_dinit_$method_num$(registry);\n",
573+
"private static void _clinit_autosplit_dinit_$method_num$(\n"
574+
" com.google.protobuf.ExtensionRegistry registry) {\n");
575+
}
547576
printer->Print(
548577
"com.google.protobuf.Descriptors.FileDescriptor\n"
549578
" .internalUpdateFileDescriptor(descriptor, registry);\n");

‎src/google/protobuf/compiler/java/file.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,11 @@
1717
#include <vector>
1818

1919
#include "google/protobuf/compiler/java/options.h"
20+
#include "google/protobuf/descriptor.pb.h"
2021
#include "google/protobuf/port.h"
2122

2223
namespace google {
2324
namespace protobuf {
24-
class FileDescriptor; // descriptor.h
2525
namespace io {
2626
class Printer; // printer.h
2727
}
@@ -75,6 +75,7 @@ class FileGenerator {
7575
bool ShouldIncludeDependency(const FileDescriptor* descriptor,
7676
bool immutable_api_);
7777

78+
7879
const FileDescriptor* file_;
7980
std::string java_package_;
8081
std::string classname_;

0 commit comments

Comments
 (0)