Skip to content

Commit 694eedd

Browse files
committed
Remove CollectExtensions check banning unknown custom options since unknown custom options are now expected when using import option and option_deps which exclude the options from the "builder" pool (aka "import" pool).
Note this check is not actually hit in most cases using `import option` unless there is also a separate dependency on `descriptor.proto`. PiperOrigin-RevId: 794717886
1 parent 7359f75 commit 694eedd

File tree

2 files changed

+24
-18
lines changed

2 files changed

+24
-18
lines changed

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

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,11 @@ using FieldDescriptorSet =
7070
// Recursively searches the given message to collect extensions.
7171
// Returns true if all the extensions can be recognized. The extensions will be
7272
// appended in to the extensions parameter.
73-
// Returns false when there are unknown fields, in which case the data in the
74-
// extensions output parameter is not reliable and should be discarded.
75-
bool CollectExtensions(const Message& message, FieldDescriptorSet* extensions) {
73+
// Unknown extensions may be present in the case of option imports and will be
74+
// ignored.
75+
void CollectExtensions(const Message& message, FieldDescriptorSet* extensions) {
7676
const Reflection* reflection = message.GetReflection();
7777

78-
// There are unknown fields that could be extensions, thus this call fails.
79-
if (reflection->GetUnknownFields(message).field_count() > 0) return false;
80-
8178
std::vector<const FieldDescriptor*> fields;
8279
reflection->ListFields(message, &fields);
8380

@@ -92,16 +89,14 @@ bool CollectExtensions(const Message& message, FieldDescriptorSet* extensions) {
9289
for (int j = 0; j < size; j++) {
9390
const Message& sub_message =
9491
reflection->GetRepeatedMessage(message, fields[i], j);
95-
if (!CollectExtensions(sub_message, extensions)) return false;
92+
CollectExtensions(sub_message, extensions);
9693
}
9794
} else {
9895
const Message& sub_message = reflection->GetMessage(message, fields[i]);
99-
if (!CollectExtensions(sub_message, extensions)) return false;
96+
CollectExtensions(sub_message, extensions);
10097
}
10198
}
10299
}
103-
104-
return true;
105100
}
106101

107102
// Finds all extensions for custom options in the given file descriptor with the
@@ -115,7 +110,7 @@ void CollectExtensions(const FileDescriptor& file,
115110
file_proto.GetDescriptor()->full_name());
116111

117112
// descriptor.proto is not found in the builder pool, meaning there are no
118-
// custom options.
113+
// custom options or they are option imported and not reachable.
119114
if (file_proto_desc == nullptr) return;
120115

121116
DynamicMessageFactory factory;
@@ -124,14 +119,10 @@ void CollectExtensions(const FileDescriptor& file,
124119
ABSL_CHECK(dynamic_file_proto.get() != nullptr);
125120
ABSL_CHECK(dynamic_file_proto->ParseFromString(file_data));
126121

127-
// Collect the extensions again from the dynamic message.
122+
// Collect the extensions from the dynamic message.
128123
extensions->clear();
129-
ABSL_CHECK(CollectExtensions(*dynamic_file_proto, extensions))
130-
<< "Found unknown fields in FileDescriptorProto when building "
131-
<< file_proto.name()
132-
<< ". It's likely that those fields are custom options, however, "
133-
"those options cannot be recognized in the builder pool. "
134-
"This normally should not happen. Please report a bug.";
124+
// Unknown extensions are ok and expected in the case of option imports.
125+
CollectExtensions(*dynamic_file_proto, extensions);
135126
}
136127

137128
// Our static initialization methods can become very, very large.

src/google/protobuf/compiler/java/generator_unittest.cc

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <memory>
1111
#include <string>
1212

13+
#include "google/protobuf/testing/file.h"
1314
#include "google/protobuf/testing/file.h"
1415
#include "google/protobuf/testing/file.h"
1516
#include "google/protobuf/descriptor.pb.h"
@@ -19,13 +20,15 @@
1920
#include "google/protobuf/compiler/java/java_features.pb.h"
2021
#include "google/protobuf/compiler/command_line_interface_tester.h"
2122

23+
2224
namespace google {
2325
namespace protobuf {
2426
namespace compiler {
2527
namespace java {
2628
namespace {
2729

2830
#define PACKAGE_PREFIX ""
31+
#define PACKAGE_IMPORT_PREFIX ""
2932

3033
class JavaGeneratorTest : public CommandLineInterfaceTester {
3134
protected:
@@ -45,6 +48,16 @@ class JavaGeneratorTest : public CommandLineInterfaceTester {
4548
std::string path = absl::StrCat(temp_directory(), "/", filename);
4649
return File::Exists(path);
4750
}
51+
52+
bool FileContainsSubstring(absl::string_view filename,
53+
absl::string_view substring) {
54+
std::string path = absl::StrCat(temp_directory(), "/", filename);
55+
std::string contents;
56+
if (!File::GetContents(path, &contents, true).ok()) {
57+
return false;
58+
}
59+
return contents.find(substring) != std::string::npos;
60+
}
4861
};
4962

5063
TEST_F(JavaGeneratorTest, Basic) {
@@ -365,6 +378,8 @@ TEST_F(JavaGeneratorTest,
365378
"class name, \"TestFileNameProto\", matches the name "
366379
"of one of the types declared inside it");
367380
}
381+
382+
368383
} // namespace
369384
} // namespace java
370385
} // namespace compiler

0 commit comments

Comments
 (0)