Skip to content

Commit

Permalink
pandaproxy/sr: fix normalization order for repeated custom options
Browse files Browse the repository at this point in the history 
- grouped all the duplicated calls to maybe_sort custom options into
a single helper function
- changed projection function from just the name of the custom option
to the full serialization of it. This way, both the name and the
value contributes to the ordering.
  • Loading branch information
@IoannisRP
IoannisRP committed Jan 21, 2025
1 parent f15e7e4 commit 0ec5585
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 49 deletions.
74 changes: 25 additions & 49 deletions src/v/pandaproxy/schema_registry/protobuf.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,14 @@ struct protobuf_schema_definition::impl {
return range_proxy<Range>{std::move(copy)};
}

auto maybe_sorted_uninterpreted_options(
const pb::RepeatedPtrField<pb::UninterpretedOption>& options) const {
return maybe_sorted(
options, std::less{}, [](const pb::UninterpretedOption& o) {
return fmt::format("{}", o);
});
}

void render_field(
std::ostream& os,
pb::Edition edition,
Expand Down Expand Up @@ -867,12 +875,8 @@ struct protobuf_schema_definition::impl {
},
field_options());

auto uninterpreted_options = maybe_sorted(
options.uninterpreted_option(),
std::less{},
[](const auto& option) {
return fmt::format("{}", fmt::join(option.name(), "."));
});
auto uninterpreted_options = maybe_sorted_uninterpreted_options(
options.uninterpreted_option());
for (const auto& option : uninterpreted_options) {
maybe_print_seperator();
fmt::print(os, "{}", option);
Expand Down Expand Up @@ -975,12 +979,8 @@ struct protobuf_schema_definition::impl {
message.options().no_standard_descriptor_accessor());
}
}
auto uninterepreted_options = maybe_sorted(
message.options().uninterpreted_option(),
std::less{},
[](const auto& option) {
return fmt::format("{}", fmt::join(option.name(), "."));
});
auto uninterepreted_options = maybe_sorted_uninterpreted_options(
message.options().uninterpreted_option());
for (const auto& option : uninterepreted_options) {
fmt::print(os, "{:{}}option {};\n", "", indent + 2, option);
}
Expand Down Expand Up @@ -1045,12 +1045,8 @@ struct protobuf_schema_definition::impl {
for (const int i : oneofs) {
const auto& decl = message.oneof_decl(i);
fmt::print(os, "{:{}}oneof {} {{\n", "", indent + 2, decl.name());
auto uninterpreted_options = maybe_sorted(
decl.options().uninterpreted_option(),
std::less{},
[](const pb::UninterpretedOption& o) {
return fmt::format("{}", fmt::join(o.name(), ""));
});
auto uninterpreted_options = maybe_sorted_uninterpreted_options(
decl.options().uninterpreted_option());
for (const auto& option : uninterpreted_options) {
fmt::print(os, "{:{}}option {};\n", "", indent + 4, option);
}
Expand Down Expand Up @@ -1152,12 +1148,8 @@ struct protobuf_schema_definition::impl {
indent + 2,
enum_proto.options().deprecated());
}
auto uninterpreted_options = maybe_sorted(
enum_proto.options().uninterpreted_option(),
std::less{},
[](const pb::UninterpretedOption& o) {
return fmt::format("{}", fmt::join(o.name(), ""));
});
auto uninterpreted_options = maybe_sorted_uninterpreted_options(
enum_proto.options().uninterpreted_option());
for (const auto& option : uninterpreted_options) {
fmt::print(os, "{:{}}option {};\n", "", indent + 2, option);
}
Expand Down Expand Up @@ -1191,13 +1183,9 @@ struct protobuf_schema_definition::impl {
}
if (!value.options().uninterpreted_option().empty()) {
maybe_print_comma();
auto uninterpreted_options = maybe_sorted(
value.options().uninterpreted_option(),
std::less{},
[](const auto& option) {
return fmt::format(
"{}", fmt::join(option.name(), "."));
});
auto uninterpreted_options
= maybe_sorted_uninterpreted_options(
value.options().uninterpreted_option());
fmt::print(
os, "{}", fmt::join(uninterpreted_options, ", "));
}
Expand All @@ -1223,12 +1211,8 @@ struct protobuf_schema_definition::impl {
indent + 2,
service.options().deprecated());
}
auto uninterpreted_options = maybe_sorted(
service.options().uninterpreted_option(),
std::less{},
[](const pb::UninterpretedOption& o) {
return fmt::format("{}", fmt::join(o.name(), "."));
});
auto uninterpreted_options = maybe_sorted_uninterpreted_options(
service.options().uninterpreted_option());
for (const auto& option : uninterpreted_options) {
fmt::print(os, "{:{}}option {};\n", "", indent + 2, option);
}
Expand Down Expand Up @@ -1263,12 +1247,8 @@ struct protobuf_schema_definition::impl {
pb::MethodOptions_IdempotencyLevel_Name(
method.options().idempotency_level()));
}
auto uninterpreted_options = maybe_sorted(
method.options().uninterpreted_option(),
std::less{},
[](const pb::UninterpretedOption& o) {
return fmt::format("{}", fmt::join(o.name(), "."));
});
auto uninterpreted_options = maybe_sorted_uninterpreted_options(
method.options().uninterpreted_option());
for (const auto& option : uninterpreted_options) {
fmt::print(os, "{:{}}option {};\n", "", indent + 4, option);
}
Expand Down Expand Up @@ -1348,12 +1328,8 @@ struct protobuf_schema_definition::impl {
if (options.has_py_generic_services()) {
printv("py_generic_services", options.py_generic_services());
}
auto uninterpreted_options = maybe_sorted(
options.uninterpreted_option(),
std::less{},
[](const pb::UninterpretedOption& o) {
return fmt::format("{}", fmt::join(o.name(), "."));
});
auto uninterpreted_options = maybe_sorted_uninterpreted_options(
options.uninterpreted_option());
for (const auto& option : uninterpreted_options) {
first_option = false;
fmt::print(os, "option {};\n", option);
Expand Down
12 changes: 12 additions & 0 deletions src/v/pandaproxy/schema_registry/test/compatibility_protobuf.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,7 @@ SEASTAR_THREAD_TEST_CASE(test_protobuf_normalize_custom_options) {
extend google.protobuf.FileOptions {
optional string my_file_option_b = 50008;
optional string my_file_option_a = 50000;
repeated uint32 my_repeated_file_option = 60000;
}
extend google.protobuf.MessageOptions {
optional int32 my_message_option_b = 50009;
Expand Down Expand Up @@ -638,7 +639,10 @@ extend google.protobuf.MethodOptions {
optional MyMessage my_method_option_a = 50007;
}
option (my_repeated_file_option) = 2;
option (my_file_option_b) = "Some other string";
option (my_repeated_file_option) = 1;
option (my_repeated_file_option) = 3;
option (my_file_option_a) = "Hello world!";
message MyMessage {
Expand Down Expand Up @@ -685,7 +689,10 @@ service MyService {
import "google/protobuf/descriptor.proto";
option (my_repeated_file_option) = 2;
option (my_file_option_b) = "Some other string";
option (my_repeated_file_option) = 1;
option (my_repeated_file_option) = 3;
option (my_file_option_a) = "Hello world!";
message MyMessage {
Expand Down Expand Up @@ -716,6 +723,7 @@ enum MyEnum {
extend google.protobuf.FileOptions {
optional string my_file_option_b = 50008;
optional string my_file_option_a = 50000;
repeated uint32 my_repeated_file_option = 60000;
}
extend google.protobuf.MessageOptions {
optional int32 my_message_option_b = 50009;
Expand Down Expand Up @@ -768,6 +776,9 @@ import "google/protobuf/descriptor.proto";
option (my_file_option_a) = "Hello world!";
option (my_file_option_b) = "Some other string";
option (my_repeated_file_option) = 1;
option (my_repeated_file_option) = 2;
option (my_repeated_file_option) = 3;
message MyMessage {
option (my_message_option_a) = 1234;
Expand Down Expand Up @@ -810,6 +821,7 @@ extend .google.protobuf.FileOptions {
optional string my_file_option_a = 50000;
optional string my_file_option_b = 50008;
optional string my_file_option_c = 50015;
repeated uint32 my_repeated_file_option = 60000;
}
extend .google.protobuf.MessageOptions {
optional int32 my_message_option_a = 50001;
Expand Down

0 comments on commit 0ec5585

Please sign in to comment.