Fix bug in map field to JSON conversion.

Any packing logic for a field-backed map would silently fail.

PiperOrigin-RevId: 967426921

Author: jnthntatum

common/value.cc

@@ -22,6 +22,7 @@
 #include <string>
 #include <type_traits>
 #include <utility>
+#include <variant>
 
 #include "google/protobuf/struct.pb.h"
 #include "absl/base/attributes.h"
@@ -1512,10 +1513,14 @@ Value WrapFieldImpl(
   ABSL_DCHECK(!IsWellKnownMessageType(message->GetDescriptor()));
 
   const auto* reflection = message->GetReflection();
+  if (ABSL_PREDICT_FALSE(reflection == nullptr)) {
+    // This only happens for special implementations of Message that
+    // should not normally be used with CEL.
+    return ErrorValue(absl::InvalidArgumentError(
+        absl::StrCat("failed to get reflection for message type: ",
+                     message->GetDescriptor()->full_name())));
+  }
   if (field->is_map()) {
-    if (reflection->FieldSize(*message, field) == 0) {
-      return MapValue();
-    }
     if constexpr (Unsafe::value) {
       return UnsafeParsedMapFieldValue(message, field);
     } else {
@@ -1524,9 +1529,6 @@ Value WrapFieldImpl(
     }
   }
   if (field->is_repeated()) {
-    if (reflection->FieldSize(*message, field) == 0) {
-      return ListValue();
-    }
     if constexpr (Unsafe::value) {
       return UnsafeParsedRepeatedFieldValue(message, field);
     } else {
@@ -1653,6 +1655,13 @@ Value WrapRepeatedFieldImpl(
   ABSL_DCHECK(arena != nullptr);
 
   const auto* reflection = message->GetReflection();
+  if (ABSL_PREDICT_FALSE(reflection == nullptr)) {
+    // This only happens for special implementations of Message that
+    // should not normally be used with CEL.
+    return ErrorValue(absl::InvalidArgumentError(
+        absl::StrCat("failed to get reflection for message type: ",
+                     message->GetDescriptor()->full_name())));
+  }
   const int size = reflection->FieldSize(*message, field);
   if (ABSL_PREDICT_FALSE(index < 0 || index >= size)) {
     return ErrorValue(absl::InvalidArgumentError(

common/values/custom_map_value.cc

@@ -14,7 +14,9 @@
 
 #include <cstddef>
 #include <memory>
+#include <optional>
 #include <string>
+#include <utility>
 
 #include "absl/base/attributes.h"
 #include "absl/base/no_destructor.h"
@@ -673,24 +675,34 @@ absl::StatusOr<bool> CustomMapValue::Find(
       return false;
   }
 
-  bool ok;
   if (dispatcher_ == nullptr) {
     CustomMapValueInterface::Content content =
         content_.To<CustomMapValueInterface::Content>();
     ABSL_DCHECK(content.interface != nullptr);
-    CEL_ASSIGN_OR_RETURN(
-        ok, content.interface->Find(key, descriptor_pool, message_factory,
-                                    arena, result));
-  } else {
-    CEL_ASSIGN_OR_RETURN(
-        ok, dispatcher_->find(dispatcher_, content_, key, descriptor_pool,
-                              message_factory, arena, result));
-  }
-  if (ok) {
+    auto status_or_found = content.interface->Find(
+        key, descriptor_pool, message_factory, arena, result);
+    if (!status_or_found.ok()) {
+      *result = ErrorValue(std::move(status_or_found).status());
+      return false;
+    }
+    if (!*status_or_found) {
+      *result = NullValue();
+      return false;
+    }
     return true;
   }
-  *result = NullValue{};
-  return false;
+  auto status_or_found =
+      dispatcher_->find(dispatcher_, content_, key, descriptor_pool,
+                        message_factory, arena, result);
+  if (!status_or_found.ok()) {
+    *result = ErrorValue(std::move(status_or_found).status());
+    return false;
+  }
+  if (!*status_or_found) {
+    *result = NullValue();
+    return false;
+  }
+  return true;
 }
 
 absl::Status CustomMapValue::Has(
@@ -721,19 +733,26 @@ absl::Status CustomMapValue::Has(
       *result = ErrorValue(InvalidMapKeyTypeError(key.kind()));
       return absl::OkStatus();
   }
-  bool has;
   if (dispatcher_ == nullptr) {
     CustomMapValueInterface::Content content =
         content_.To<CustomMapValueInterface::Content>();
     ABSL_DCHECK(content.interface != nullptr);
-    CEL_ASSIGN_OR_RETURN(has, content.interface->Has(key, descriptor_pool,
-                                                     message_factory, arena));
-  } else {
-    CEL_ASSIGN_OR_RETURN(
-        has, dispatcher_->has(dispatcher_, content_, key, descriptor_pool,
-                              message_factory, arena));
+    auto status_or_has =
+        content.interface->Has(key, descriptor_pool, message_factory, arena);
+    if (!status_or_has.ok()) {
+      *result = ErrorValue(std::move(status_or_has).status());
+      return absl::OkStatus();
+    }
+    *result = BoolValue(*status_or_has);
+    return absl::OkStatus();
+  }
+  auto status_or_has = dispatcher_->has(
+      dispatcher_, content_, key, descriptor_pool, message_factory, arena);
+  if (!status_or_has.ok()) {
+    *result = ErrorValue(std::move(status_or_has).status());
+    return absl::OkStatus();
   }
-  *result = BoolValue(has);
+  *result = BoolValue(*status_or_has);
   return absl::OkStatus();
 }
 

common/values/custom_map_value.h

@@ -54,6 +54,12 @@ class CustomMapValueInterfaceKeysIterator;
 class CustomMapValue;
 using CustomMapValueContent = CustomValueContent;
 
+// Dispatch table for `CustomMapValue`.
+//
+// See the documentation for `CustomMapValueInterface` for more details on
+// composite functions.
+//
+// See documentation for `UnsafeCustomMapValue` on how to use this class.
 struct CustomMapValueDispatcher {
   using GetTypeId =
       NativeTypeId (*)(const CustomMapValueDispatcher* absl_nonnull dispatcher,
@@ -247,12 +253,21 @@ class CustomMapValueInterface {
 
   virtual CustomMapValue Clone(google::protobuf::Arena* absl_nonnull arena) const = 0;
 
+  // Tests whether the map contains the given key. If it does, the value
+  // associated with the key is written to `result` and the function returns
+  // true. Otherwise, the function returns false and `result` is set to
+  // `NullValue`.
+  //
+  // A non-ok status is converted to an ErrorValue (e.g. wrong key type).
   virtual absl::StatusOr<bool> Find(
       const Value& key,
       const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
       google::protobuf::MessageFactory* absl_nonnull message_factory,
       google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const = 0;
 
+  // Whether the map has the given key.
+  //
+  // A non-ok status is converted to an ErrorValue (e.g. wrong key type).
   virtual absl::StatusOr<bool> Has(
       const Value& key,
       const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,