Merge pull request #24993 from IoannisRP/ik-manual-backport-PR24992-v24.2.x

[v24.2.x] pandaproxy/sr: Fix issues discovered in the protobuf renderer followup

Author: IoannisRP

src/v/pandaproxy/schema_registry/protobuf.cc

@@ -28,6 +28,7 @@
 
 #include <absl/container/flat_hash_set.h>
 #include <absl/strings/ascii.h>
+#include <absl/strings/escaping.h>
 #include <boost/algorithm/string/trim.hpp>
 #include <boost/container/flat_set.hpp>
 #include <boost/range/combine.hpp>
@@ -72,12 +73,24 @@
 
 #include <algorithm>
 #include <charconv>
+#include <concepts>
+#include <cstdint>
 #include <functional>
 #include <optional>
 #include <ranges>
 #include <string_view>
 #include <unordered_set>
 
+namespace {
+
+// Protobuf string values need to be quoted and escaped
+// as they may contain characters like '\'
+auto pb_string_value(const std::string_view v) {
+    return fmt::format("\"{}\"", absl::CEscape(v));
+}
+
+} // namespace
+
 struct indent_formatter : fmt::formatter<std::string_view> {
     using Base = fmt::formatter<std::string_view>;
     constexpr auto parse(fmt::format_parse_context& ctx) {
@@ -133,10 +146,18 @@ struct fmt::formatter<google::protobuf::UninterpretedOption>
       const google::protobuf::UninterpretedOption& option,
       format_context& ctx) const {
         const auto fmt = [&](const auto& val) {
-            if (option.has_string_value()) {
-                return fmt::format_to(
-                  ctx.out(), "{} = \"{}\"", fmt::join(option.name(), "."), val);
-            } else if (option.has_aggregate_value()) {
+            if constexpr (std::convertible_to<
+                            std::decay_t<decltype(val)>,
+                            std::string_view>) {
+                if (option.has_string_value()) {
+                    return fmt::format_to(
+                      ctx.out(),
+                      "{} = {}",
+                      fmt::join(option.name(), "."),
+                      pb_string_value(val));
+                }
+            }
+            if (option.has_aggregate_value()) {
                 return fmt::format_to(
                   ctx.out(),
                   "{} = {{{}\n{:{}}}}",
@@ -255,6 +276,10 @@ auto field_options() {
         "packed", &pb::FieldOptions::has_packed, &pb::FieldOptions::packed},
       field_option{
         "lazy", &pb::FieldOptions::has_lazy, &pb::FieldOptions::lazy},
+      field_option{
+        "unverified_lazy",
+        &pb::FieldOptions::has_unverified_lazy,
+        &pb::FieldOptions::unverified_lazy},
       field_option{
         "weak", &pb::FieldOptions::has_weak, &pb::FieldOptions::weak},
       field_option{
@@ -919,7 +944,8 @@ struct protobuf_schema_definition::impl {
         }
         if (field.has_json_name()) {
             maybe_print_seperator();
-            fmt::print(os, "json_name = \"{}\"", field.json_name());
+            fmt::print(
+              os, "json_name = {}", pb_string_value(field.json_name()));
         }
         if (field.has_options()) {
             const auto& options = field.options();
@@ -1075,11 +1101,12 @@ struct protobuf_schema_definition::impl {
 
         if (decl.has_full_name()) {
             maybe_print_seperator();
-            fmt::print(os, "{}: \"{}\"", "full_name", decl.full_name());
+            fmt::print(
+              os, "{}: {}", "full_name", pb_string_value(decl.full_name()));
         }
         if (decl.has_type()) {
             maybe_print_seperator();
-            fmt::print(os, "{}: \"{}\"", "type", decl.type());
+            fmt::print(os, "{}: {}", "type", pb_string_value(decl.type()));
         }
         if (decl.has_number()) {
             maybe_print_seperator();
@@ -1161,12 +1188,16 @@ struct protobuf_schema_definition::impl {
         }
         auto reserved_names = maybe_sorted(message.reserved_name());
         if (!reserved_names.empty()) {
+            const auto to_debug_string = [](const std::string_view strv) {
+                return fmt::format("{}", pb_string_value(strv));
+            };
             fmt::print(
               os,
-              "{:{}}reserved \"{}\";\n",
+              "{:{}}reserved {};\n",
               "",
               indent + 2,
-              fmt::join(reserved_names, "\", \""));
+              fmt::join(
+                reserved_names | std::views::transform(to_debug_string), ", "));
         }
         if (!reserved_range.empty() || !reserved_names.empty()) {
             fmt::print(os, "\n");
@@ -1287,7 +1318,12 @@ struct protobuf_schema_definition::impl {
             fmt::print(os, ";\n");
         }
         for (const auto& value : maybe_sorted(enum_proto.reserved_name())) {
-            fmt::print(os, "{:{}}reserved \"{}\";\n", "", indent + 2, value);
+            fmt::print(
+              os,
+              "{:{}}reserved {};\n",
+              "",
+              indent + 2,
+              pb_string_value(value));
         }
         if (enum_proto.options().has_allow_alias()) {
             fmt::print(
@@ -1310,14 +1346,16 @@ struct protobuf_schema_definition::impl {
         for (const auto& option : uninterpreted_options) {
             fmt::print(os, "{:{}}option {};\n", "", indent + 2, option);
         }
-        std::optional<std::decay_t<decltype(enum_proto.value())>> values;
-        if (is_normalized) {
-            values = enum_proto.value();
-            std::ranges::sort(values.value(), std::less{}, [](const auto& v) {
-                return std::pair<int, std::string_view>{v.number(), v.name()};
-            });
-        }
-        for (const auto& value : values.value_or(enum_proto.value())) {
+        const auto values = maybe_sorted(
+          enum_proto.value(), std::less{}, [](const auto& v) {
+              // In proto3, enums are open and open enums need to
+              // have the first field being equal to zero. By casting
+              // to an unsigned integer for sorting, all the negative
+              // fields will be at the end, after all the positives.
+              return std::pair<uint32_t, std::string_view>{
+                static_cast<uint32_t>(v.number()), v.name()};
+          });
+        for (const auto& value : values) {
             fmt::print(
               os, "{:{}}{} = {}", "", indent + 2, value.name(), value.number());
             if (value.has_options()) {
@@ -1427,7 +1465,7 @@ struct protobuf_schema_definition::impl {
             first_option = false;
         };
         auto prints = [&](std::string_view name, const auto& val) {
-            fmt::print(os, "option {} = \"{}\";\n", name, val);
+            fmt::print(os, "option {} = {};\n", name, pb_string_value(val));
             first_option = false;
         };
         if (options.has_cc_enable_arenas()) {
@@ -1544,7 +1582,7 @@ struct protobuf_schema_definition::impl {
 
         auto print_deps = [&](const auto& view, std::string_view type) {
             for (const auto& dep : view) {
-                fmt::print(os, "import {}\"{}\";\n", type, dep);
+                fmt::print(os, "import {}{};\n", type, pb_string_value(dep));
             }
         };
 
@@ -1559,7 +1597,7 @@ struct protobuf_schema_definition::impl {
       const pb::FileDescriptor& descriptor) const {
         auto syntax = fdp.has_syntax() ? fdp.syntax() : "proto2";
         std::string_view edition = syntax;
-        fmt::print(os, "syntax = \"{}\";\n", syntax);
+        fmt::print(os, "syntax = {};\n", pb_string_value(syntax));
 
         if (fdp.has_package() && !fdp.package().empty()) {
             fmt::print(os, "package {};\n", fdp.package());

src/v/pandaproxy/schema_registry/test/compatibility_protobuf.cc

@@ -1513,6 +1513,7 @@ option go_package = "foo.example.com/fooservice";
 option cc_enable_arenas = true;
 option objc_class_prefix = "FS";
 option csharp_namespace = "Foo.FooService";
+option php_namespace = "my_php\ns";
 
 
 // public should come last
@@ -1529,6 +1530,8 @@ message Baz {
 
 enum Numbers {
   ZERO=0;
+  MINUS_ONE =-1;
+  MINUS_TWO =-2;
   TWO = 2;
   ONE=1;
   ALIAS = 1 [deprecated = true, debug_redact = false];
@@ -1615,6 +1618,7 @@ option go_package = "foo.example.com/fooservice";
 option cc_enable_arenas = true;
 option objc_class_prefix = "FS";
 option csharp_namespace = "Foo.FooService";
+option php_namespace = "my_php\ns";
 
 message Baz {
   .google.protobuf.Any any = 1;
@@ -1677,6 +1681,8 @@ enum Numbers {
   reserved "SIX";
   option allow_alias = true;
   ZERO = 0;
+  MINUS_ONE = -1;
+  MINUS_TWO = -2;
   TWO = 2;
   ONE = 1;
   ALIAS = 1 [deprecated = true, debug_redact = false];
@@ -1703,6 +1709,7 @@ option java_outer_classname = "FooService";
 option java_package = "com.example.foo";
 option objc_class_prefix = "FS";
 option optimize_for = SPEED;
+option php_namespace = "my_php\ns";
 
 message Baz {
   .google.protobuf.Any any = 1;
@@ -1768,6 +1775,8 @@ enum Numbers {
   ALIAS = 1 [deprecated = true, debug_redact = false];
   ONE = 1;
   TWO = 2;
+  MINUS_TWO = -2;
+  MINUS_ONE = -1;
 }
 
 service FooService {