Make error handling in wrapped legacy map consistent with modern version.

common/legacy_value.cc

@@ -700,7 +700,8 @@ absl::Status LegacyMapValue::Get(
     case ValueKind::kString:
       break;
     default:
-      return InvalidMapKeyTypeError(key.kind());
+      *result = ErrorValue(InvalidMapKeyTypeError(key.kind()));
+      return absl::OkStatus();
   }
   CEL_ASSIGN_OR_RETURN(auto cel_key, LegacyValue(arena, key));
   auto cel_value = impl_->Get(arena, cel_key);
@@ -732,7 +733,7 @@ absl::StatusOr<bool> LegacyMapValue::Find(
     case ValueKind::kString:
       break;
     default:
-      return InvalidMapKeyTypeError(key.kind());
+      *result = ErrorValue(InvalidMapKeyTypeError(key.kind()));
   }
   CEL_ASSIGN_OR_RETURN(auto cel_key, LegacyValue(arena, key));
   auto cel_value = impl_->Get(arena, cel_key);
@@ -764,11 +765,17 @@ absl::Status LegacyMapValue::Has(
     case ValueKind::kString:
       break;
     default:
-      return InvalidMapKeyTypeError(key.kind());
+      *result = ErrorValue(InvalidMapKeyTypeError(key.kind()));
+      return absl::OkStatus();
   }
   CEL_ASSIGN_OR_RETURN(auto cel_key, LegacyValue(arena, key));
-  CEL_ASSIGN_OR_RETURN(auto has, impl_->Has(cel_key));
-  *result = BoolValue{has};
+  absl::StatusOr<bool> has = impl_->Has(cel_key);
+  if (!has.ok()) {
+    *result = ErrorValue(std::move(has).status());
+    return absl::OkStatus();
+  }
+
+  *result = BoolValue(*has);
   return absl::OkStatus();
 }
 

common/values/custom_map_value.h

@@ -225,15 +225,15 @@ class CustomMapValueInterface {
   // Returns the number of entries in this map.
   virtual size_t Size() const = 0;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   virtual absl::Status ListKeys(
       const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
       google::protobuf::MessageFactory* absl_nonnull message_factory,
       google::protobuf::Arena* absl_nonnull arena,
       ListValue* absl_nonnull result) const = 0;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   virtual absl::Status ForEach(
       ForEachCallback callback,
@@ -347,7 +347,7 @@ class CustomMapValue final
 
   size_t Size() const;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status Get(const Value& key,
                    const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
@@ -356,7 +356,7 @@ class CustomMapValue final
                    Value* absl_nonnull result) const;
   using MapValueMixin::Get;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::StatusOr<bool> Find(
       const Value& key,
@@ -365,7 +365,7 @@ class CustomMapValue final
       google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const;
   using MapValueMixin::Find;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status Has(const Value& key,
                    const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
@@ -374,7 +374,7 @@ class CustomMapValue final
                    Value* absl_nonnull result) const;
   using MapValueMixin::Has;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status ListKeys(
       const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
@@ -386,15 +386,15 @@ class CustomMapValue final
   // documentation.
   using ForEachCallback = typename CustomMapValueInterface::ForEachCallback;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status ForEach(
       ForEachCallback callback,
       const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
       google::protobuf::MessageFactory* absl_nonnull message_factory,
       google::protobuf::Arena* absl_nonnull arena) const;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::StatusOr<absl_nonnull ValueIteratorPtr> NewIterator() const;
 

common/values/legacy_map_value.h

@@ -102,7 +102,7 @@ class LegacyMapValue final
 
   size_t Size() const;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status Get(const Value& key,
                    const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
@@ -111,7 +111,7 @@ class LegacyMapValue final
                    Value* absl_nonnull result) const;
   using MapValueMixin::Get;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::StatusOr<bool> Find(
       const Value& key,
@@ -120,7 +120,7 @@ class LegacyMapValue final
       google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const;
   using MapValueMixin::Find;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status Has(const Value& key,
                    const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
@@ -129,19 +129,19 @@ class LegacyMapValue final
                    Value* absl_nonnull result) const;
   using MapValueMixin::Has;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status ListKeys(
       const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
       google::protobuf::MessageFactory* absl_nonnull message_factory,
       google::protobuf::Arena* absl_nonnull arena, ListValue* absl_nonnull result) const;
   using MapValueMixin::ListKeys;
 
-  // See the corresponding type declaration of `MapValueInterface` for
+  // See the corresponding type declaration of `MapValue` for
   // documentation.
   using ForEachCallback = typename CustomMapValueInterface::ForEachCallback;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status ForEach(
       ForEachCallback callback,

common/values/map_value.h

@@ -15,10 +15,16 @@
 // IWYU pragma: private, include "common/value.h"
 // IWYU pragma: friend "common/value.h"
 
-// `MapValue` represents values of the primitive `map` type. `MapValueView`
-// is a non-owning view of `MapValue`. `MapValueInterface` is the abstract
-// base class of implementations. `MapValue` and `MapValueView` act as smart
-// pointers to `MapValueInterface`.
+// `MapValue` represents values of the primitive `map` type. It provides a
+// unified interface for accessing map contents, regardless of the underlying
+// implementation (e.g., JSON, protobuf map field, or custom implementation).
+//
+// Public member functions:
+// - `IsEmpty()` / `Size()`: Query map size.
+// - `Get()` / `Find()` / `Has()`: Access entries by key.
+// - `ListKeys()` / `NewIterator()` / `ForEach()`: Iterate over entries.
+// - `ConvertToJson()` / `ConvertToJsonObject()`: JSON conversion.
+// - `IsCustom()` / `AsCustom()` / `GetCustom()`: Access custom implementation.
 
 #ifndef THIRD_PARTY_CEL_CPP_COMMON_VALUES_MAP_VALUE_H_
 #define THIRD_PARTY_CEL_CPP_COMMON_VALUES_MAP_VALUE_H_
@@ -54,7 +60,6 @@
 
 namespace cel {
 
-class MapValueInterface;
 class MapValue;
 class Value;
 
@@ -119,55 +124,67 @@ class MapValue final : private common_internal::MapValueMixin<MapValue> {
 
   absl::StatusOr<size_t> Size() const;
 
-  // See the corresponding member function of `MapValueInterface` for
-  // documentation.
+  // `Get` sets the value `result` to (via `result`) the value associated with
+  // `key`. If `key` is not found, `no such key` is set to `result`. If an error
+  // occurs (e.g., invalid key type), an `no such key` is returned.
+  //
+  // A non-ok status may be returned if an unexpected error is encountered or to
+  // propagate an error from a custom implementation, in which case `result` is
+  // unspecified.
   absl::Status Get(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;
   using MapValueMixin::Get;
 
-  // See the corresponding member function of `MapValueInterface` for
-  // documentation.
+  // `Find` returns `true` if `key` is found in the map, and stores the
+  // associated value in `result`. If `key` is not found, `false` is returned
+  // and `result` is unchanged.
+  //
+  // A non-ok status may be returned if an unexpected error is encountered or to
+  // propagate an error from a custom implementation, in which case `result` is
+  // unspecified.
   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;
   using MapValueMixin::Find;
 
-  // See the corresponding member function of `MapValueInterface` for
-  // documentation.
+  // `Has` returns `true` if `key` is found in the map, and stores the BoolValue
+  // result in `result`. In case of an error, the result is set to an
+  // ErrorValue.
+  //
+  // A non-ok status may be returned if an unexpected error is encountered or to
+  // propagate an error from a custom implementation, in which case `result` is
+  // unspecified.
   absl::Status Has(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;
   using MapValueMixin::Has;
 
-  // See the corresponding member function of `MapValueInterface` for
-  // documentation.
+  // `ListKeys` returns a `ListValue` containing all keys in the map.
   absl::Status ListKeys(
       const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
       google::protobuf::MessageFactory* absl_nonnull message_factory,
       google::protobuf::Arena* absl_nonnull arena, ListValue* absl_nonnull result) const;
   using MapValueMixin::ListKeys;
 
-  // See the corresponding type declaration of `MapValueInterface` for
-  // documentation.
+  // `ForEachCallback` is the callback type for `ForEach`.
   using ForEachCallback = typename CustomMapValueInterface::ForEachCallback;
 
-  // See the corresponding member function of `MapValueInterface` for
-  // documentation.
+  // `ForEach` calls `callback` for each entry in the map. Iteration continues
+  // until all entries are visited or `callback` returns an error or `false`.
   absl::Status ForEach(
       ForEachCallback callback,
       const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
       google::protobuf::MessageFactory* absl_nonnull message_factory,
       google::protobuf::Arena* absl_nonnull arena) const;
 
-  // See the corresponding member function of `MapValueInterface` for
-  // documentation.
+  // `NewIterator` returns a new iterator for the map.
   absl::StatusOr<absl_nonnull ValueIteratorPtr> NewIterator() const;
 
   // Returns `true` if this value is an instance of a custom map value.

common/values/null_value.h

@@ -37,8 +37,7 @@ namespace cel {
 class Value;
 class NullValue;
 
-// `NullValue` represents values of the primitive `duration` type.
-
+// `NullValue` represents the CEL `null` value.
 class NullValue final : private common_internal::ValueMixin<NullValue> {
  public:
   static constexpr ValueKind kKind = ValueKind::kNull;

common/values/parsed_json_map_value.h

@@ -132,7 +132,7 @@ class ParsedJsonMapValue final
 
   size_t Size() const;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status Get(const Value& key,
                    const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
@@ -141,7 +141,7 @@ class ParsedJsonMapValue final
                    Value* absl_nonnull result) const;
   using MapValueMixin::Get;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::StatusOr<bool> Find(
       const Value& key,
@@ -150,7 +150,7 @@ class ParsedJsonMapValue final
       google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const;
   using MapValueMixin::Find;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status Has(const Value& key,
                    const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
@@ -159,19 +159,19 @@ class ParsedJsonMapValue final
                    Value* absl_nonnull result) const;
   using MapValueMixin::Has;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status ListKeys(
       const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
       google::protobuf::MessageFactory* absl_nonnull message_factory,
       google::protobuf::Arena* absl_nonnull arena, ListValue* absl_nonnull result) const;
   using MapValueMixin::ListKeys;
 
-  // See the corresponding type declaration of `MapValueInterface` for
+  // See the corresponding type declaration of `MapValue` for
   // documentation.
   using ForEachCallback = typename CustomMapValueInterface::ForEachCallback;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status ForEach(
       ForEachCallback callback,

common/values/parsed_map_field_value.h

@@ -117,7 +117,7 @@ class ParsedMapFieldValue final
 
   size_t Size() const;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status Get(const Value& key,
                    const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
@@ -126,7 +126,7 @@ class ParsedMapFieldValue final
                    Value* absl_nonnull result) const;
   using MapValueMixin::Get;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::StatusOr<bool> Find(
       const Value& key,
@@ -135,7 +135,7 @@ class ParsedMapFieldValue final
       google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const;
   using MapValueMixin::Find;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status Has(const Value& key,
                    const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
@@ -144,19 +144,19 @@ class ParsedMapFieldValue final
                    Value* absl_nonnull result) const;
   using MapValueMixin::Has;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status ListKeys(
       const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
       google::protobuf::MessageFactory* absl_nonnull message_factory,
       google::protobuf::Arena* absl_nonnull arena, ListValue* absl_nonnull result) const;
   using MapValueMixin::ListKeys;
 
-  // See the corresponding type declaration of `MapValueInterface` for
+  // See the corresponding type declaration of `MapValue` for
   // documentation.
   using ForEachCallback = typename CustomMapValueInterface::ForEachCallback;
 
-  // See the corresponding member function of `MapValueInterface` for
+  // See the corresponding member function of `MapValue` for
   // documentation.
   absl::Status ForEach(
       ForEachCallback callback,

common/values/values.h

@@ -48,7 +48,6 @@ namespace cel {
 
 class ValueInterface;
 class ListValueInterface;
-class MapValueInterface;
 class StructValueInterface;
 
 class Value;