Fix CEL policy YAML parser to use codepoint positions instead of byte positions.

common/values/parsed_message_value.cc

@@ -60,14 +60,6 @@ EmptyParsedMessageValue() {
   return &T::default_instance();
 }
 
-template <typename T>
-std::enable_if_t<
-    std::conjunction_v<std::is_base_of<google::protobuf::MessageLite, T>,
-                       std::negation<std::is_base_of<google::protobuf::Message, T>>>,
-    const google::protobuf::Message* absl_nonnull>
-EmptyParsedMessageValue() {
-  return internal::GetEmptyDefaultInstance();
-}
 
 }  // namespace
 
@@ -114,12 +106,8 @@ absl::Status ParsedMessageValue::ConvertToJson(
   ABSL_DCHECK_EQ(json->GetDescriptor()->well_known_type(),
                  google::protobuf::Descriptor::WELLKNOWNTYPE_VALUE);
 
-  ValueReflection value_reflection;
-  CEL_RETURN_IF_ERROR(value_reflection.Initialize(json->GetDescriptor()));
-  google::protobuf::Message* json_object = value_reflection.MutableStructValue(json);
-
   return internal::MessageToJson(*value_, descriptor_pool, message_factory,
-                                 json_object);
+                                 json);
 }
 
 absl::Status ParsedMessageValue::ConvertToJsonObject(

conformance/BUILD

@@ -181,7 +181,6 @@ _TESTS_TO_SKIP = [
     "enums/legacy_proto2/select_big,select_neg",
 
     # Skip until fixed.
-    "wrappers/field_mask/to_json",
     "wrappers/empty/to_json",
     "fields/qualified_identifier_resolution/map_value_repeat_key_heterogeneous",
     "parse/receiver_function_names",

eval/public/structs/BUILD

@@ -81,6 +81,7 @@ cc_library(
         "@com_google_absl//absl/types:variant",
         "@com_google_protobuf//:any_cc_proto",
         "@com_google_protobuf//:duration_cc_proto",
+        "@com_google_protobuf//:json_util",
         "@com_google_protobuf//:protobuf",
         "@com_google_protobuf//:struct_cc_proto",
         "@com_google_protobuf//:timestamp_cc_proto",
@@ -111,6 +112,7 @@ cc_test(
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/time",
+        "@com_google_absl//absl/types:span",
         "@com_google_protobuf//:any_cc_proto",
         "@com_google_protobuf//:duration_cc_proto",
         "@com_google_protobuf//:empty_cc_proto",
@@ -218,6 +220,7 @@ cc_test(
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/time",
+        "@com_google_absl//absl/types:span",
         "@com_google_protobuf//:any_cc_proto",
         "@com_google_protobuf//:duration_cc_proto",
         "@com_google_protobuf//:empty_cc_proto",

eval/public/structs/cel_proto_wrap_util.cc

@@ -17,6 +17,7 @@
 #include <cstddef>
 #include <cstdint>
 #include <limits>
+#include <optional>
 #include <string>
 #include <type_traits>
 #include <utility>
@@ -52,6 +53,7 @@
 #include "google/protobuf/descriptor.h"
 #include "google/protobuf/message.h"
 #include "google/protobuf/message_lite.h"
+#include "google/protobuf/util/json_util.h"
 
 namespace google::api::expr::runtime::internal {
 
@@ -79,7 +81,10 @@ using google::protobuf::Descriptor;
 using google::protobuf::DescriptorPool;
 using google::protobuf::Message;
 using google::protobuf::MessageFactory;
-
+using google::protobuf::util::JsonParseOptions;
+using google::protobuf::util::JsonPrintOptions;
+using google::protobuf::util::JsonStringToMessage;
+using google::protobuf::util::MessageToJsonString;
 // kMaxIntJSON is defined as the Number.MAX_SAFE_INTEGER value per EcmaScript 6.
 constexpr int64_t kMaxIntJSON = (1ll << 53) - 1;
 
@@ -1079,6 +1084,26 @@ google::protobuf::Message* ValueFromValue(google::protobuf::Message* message, co
         return message;
       }
     } break;
+    case CelValue::Type::kMessage: {
+      const google::protobuf::Message* message_ptr = value.MessageOrDie();
+      if (message_ptr->GetDescriptor()->full_name() ==
+          "google.protobuf.FieldMask") {
+        JsonPrintOptions json_options;
+        std::string json_str;
+        auto status =
+            MessageToJsonString(*message_ptr, &json_str, json_options);
+        if (!status.ok()) {
+          return nullptr;
+        }
+        JsonParseOptions json_parse_options;
+        status = JsonStringToMessage(json_str, message, json_parse_options);
+        if (!status.ok()) {
+          return nullptr;
+        }
+        return message;
+      }
+      return nullptr;
+    } break;
     case CelValue::Type::kNullType:
       reflection.SetNullValue(message);
       return message;
@@ -1229,6 +1254,26 @@ bool ValueFromValue(Value* json, const CelValue& value, google::protobuf::Arena*
       return ListFromValue(json->mutable_list_value(), value, arena);
     case CelValue::Type::kMap:
       return StructFromValue(json->mutable_struct_value(), value, arena);
+    case CelValue::Type::kMessage: {
+      const google::protobuf::Message* message_ptr = value.MessageOrDie();
+      if (message_ptr->GetDescriptor()->full_name() ==
+          "google.protobuf.FieldMask") {
+        JsonPrintOptions json_options;
+        std::string json_str;
+        auto status =
+            MessageToJsonString(*message_ptr, &json_str, json_options);
+        if (!status.ok()) {
+          return false;
+        }
+        JsonParseOptions json_parse_options;
+        status = JsonStringToMessage(json_str, json, json_parse_options);
+        if (!status.ok()) {
+          return false;
+        }
+        return true;
+      }
+      return false;
+    }
     case CelValue::Type::kNullType:
       json->set_null_value(protobuf::NULL_VALUE);
       return true;

eval/public/structs/cel_proto_wrap_util_test.cc

@@ -15,6 +15,7 @@
 #include "eval/public/structs/cel_proto_wrap_util.h"
 
 #include <cassert>
+#include <cstdint>
 #include <limits>
 #include <memory>
 #include <string>
@@ -30,6 +31,7 @@
 #include "absl/status/status.h"
 #include "absl/strings/str_cat.h"
 #include "absl/time/time.h"
+#include "absl/types/span.h"
 #include "eval/public/cel_value.h"
 #include "eval/public/containers/container_backed_list_impl.h"
 #include "eval/public/containers/container_backed_map_impl.h"
@@ -41,6 +43,7 @@
 #include "internal/status_macros.h"
 #include "internal/testing.h"
 #include "testutil/util.h"
+#include "google/protobuf/arena.h"
 #include "google/protobuf/dynamic_message.h"
 #include "google/protobuf/message.h"
 

eval/public/structs/cel_proto_wrapper_test.cc

@@ -1,6 +1,7 @@
 #include "eval/public/structs/cel_proto_wrapper.h"
 
 #include <cassert>
+#include <cstdint>
 #include <limits>
 #include <memory>
 #include <string>
@@ -16,14 +17,15 @@
 #include "absl/status/statusor.h"
 #include "absl/strings/str_cat.h"
 #include "absl/time/time.h"
+#include "absl/types/span.h"
 #include "eval/public/cel_value.h"
 #include "eval/public/containers/container_backed_list_impl.h"
 #include "eval/public/containers/container_backed_map_impl.h"
 #include "eval/testutil/test_message.pb.h"
 #include "internal/proto_time_encoding.h"
-#include "internal/status_macros.h"
 #include "internal/testing.h"
 #include "testutil/util.h"
+#include "google/protobuf/arena.h"
 #include "google/protobuf/dynamic_message.h"
 #include "google/protobuf/message.h"
 

policy/BUILD

@@ -84,6 +84,7 @@ cc_library(
         ":cel_policy_parser",
         "//common:source",
         "//internal:status_macros",
+        "//internal:utf8",
         "//policy/internal:yaml_string_element_scanner",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",

policy/yaml_policy_parser.cc

@@ -14,35 +14,101 @@
 
 #include "policy/yaml_policy_parser.h"
 
+#include <cstddef>
 #include <memory>
 #include <optional>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 #include "common/source.h"
 #include "internal/status_macros.h"
+#include "internal/utf8.h"
 #include "policy/cel_policy.h"
 #include "policy/cel_policy_parse_context.h"
 #include "policy/cel_policy_parse_result.h"
 #include "policy/cel_policy_parser.h"
 #include "policy/internal/yaml_string_element_scanner.h"
 #include "yaml-cpp/exceptions.h"
+#include "yaml-cpp/mark.h"
 #include "yaml-cpp/node/node.h"
 #include "yaml-cpp/node/parse.h"
 #include "yaml-cpp/null.h"
 #include "yaml-cpp/yaml.h"  // IWYU pragma: keep
 
 namespace cel {
+namespace {
+
+class ByteToCodepointMapper {
+ public:
+  explicit ByteToCodepointMapper(const Source* source) {
+    if (source == nullptr) return;
+    std::string text = source->content().ToString();
+    mapping_.resize(text.size() + 1, 0);
+    size_t byte_offset = 0;
+    SourcePosition codepoint = 0;
+    absl::string_view view = text;
+    while (!view.empty()) {
+      auto [code_point, code_units] = cel::internal::Utf8Decode(view);
+      if (code_units == 0) break;
+      for (size_t i = 0; i < code_units; ++i) {
+        if (byte_offset + i < mapping_.size()) {
+          mapping_[byte_offset + i] = codepoint;
+        }
+      }
+      byte_offset += code_units;
+      view.remove_prefix(code_units);
+      codepoint++;
+    }
+    for (size_t i = byte_offset; i < mapping_.size(); ++i) {
+      mapping_[i] = codepoint;
+    }
+  }
+
+  SourcePosition GetMarkCodepointPosition(const YAML::Mark& mark) const {
+    if (mark.is_null() || mark.pos < 0) return -1;
+    size_t byte_pos = static_cast<size_t>(mark.pos);
+    if (mapping_.empty()) return static_cast<SourcePosition>(byte_pos);
+    if (byte_pos >= mapping_.size()) return mapping_.back();
+    return mapping_[byte_pos];
+  }
+
+ private:
+  std::vector<SourcePosition> mapping_;
+};
+
+thread_local const ByteToCodepointMapper* current_mapper = nullptr;
+
+struct MapperScope {
+  explicit MapperScope(const ByteToCodepointMapper* mapper)
+      : prev(current_mapper) {
+    current_mapper = mapper;
+  }
+  ~MapperScope() { current_mapper = prev; }
+  const ByteToCodepointMapper* prev;
+};
+
+SourcePosition GetMarkCodepointPosition(const CelPolicyParseContext& ctx,
+                                        const YAML::Mark& mark) {
+  if (current_mapper != nullptr) {
+    return current_mapper->GetMarkCodepointPosition(mark);
+  }
+  if (mark.is_null() || mark.pos < 0) return -1;
+  return static_cast<SourcePosition>(mark.pos);
+}
+
+}  // namespace
 
 CelPolicyElementId YamlPolicyParser::CollectMetadata(
     CelPolicyParseContext& ctx, const YAML::Node& node) const {
   CelPolicyElementId element_id = ctx.next_element_id();
   if (!node.Mark().is_null()) {
-    ctx.policy_source().NoteSourcePosition(element_id, node.Mark().pos);
+    ctx.policy_source().NoteSourcePosition(
+        element_id, GetMarkCodepointPosition(ctx, node.Mark()));
   }
   return element_id;
 }
@@ -62,9 +128,10 @@ std::optional<ValueString> YamlPolicyParser::GetValueString(
   }
 
   if (!node.Mark().is_null() && ctx.policy_source().content() != nullptr) {
+    SourcePosition codepoint_pos = GetMarkCodepointPosition(ctx, node.Mark());
     policy_internal::YamlStringElement element =
         policy_internal::ScanYamlStringElement(
-            ctx.policy_source().content()->content(), node.Mark().pos,
+            ctx.policy_source().content()->content(), codepoint_pos,
             node.as<std::string>());
 
     ctx.policy_source().NoteSourcePosition(id, element.starting_position);
@@ -87,14 +154,18 @@ absl::Status YamlPolicyParser::ParsePolicy(CelPolicyParseContext& ctx) const {
     return absl::OkStatus();
   }
 
+  ByteToCodepointMapper mapper(source);
+  MapperScope scope(&mapper);
+
   ctx.policy().set_description(ValueString(-1, source->description()));
   std::string text = source->content().ToString();
   YAML::Node node;
   try {
     node = YAML::Load(text);
   } catch (YAML::Exception& e) {
     if (!e.mark.is_null()) {
-      ctx.policy_source().NoteSourcePosition(0, e.mark.pos);
+      ctx.policy_source().NoteSourcePosition(
+          0, GetMarkCodepointPosition(ctx, e.mark));
     }
     ctx.ReportError(0, "Invalid CEL policy YAML syntax");
     return absl::OkStatus();

policy/yaml_policy_parser_test.cc

@@ -198,6 +198,17 @@ std::vector<ParseTestCase> GetParseTestCases() {
                             " |                     - cel.expr.conformance\n"
                             " | ....................^",
       },
+      ParseTestCase{
+          .yaml = R"yaml(
+                  # Comment with multi-byte char: €
+                  imports:
+                  - name:
+                    - cel.expr.conformance
+                )yaml",
+          .expected_error = "5:21: Import name is not a string\n"
+                            " |                     - cel.expr.conformance\n"
+                            " | ....................^",
+      },
       ParseTestCase{
           .yaml = R"yaml(
                   rule: do something