From 06ad429acdb4b28cf7f6360bdf2f1075ae35aa0e Mon Sep 17 00:00:00 2001 From: Justin King Date: Wed, 26 Mar 2025 09:48:25 -0700 Subject: [PATCH] Lock down custom value interfaces PiperOrigin-RevId: 740803518 --- common/values/custom_list_value.cc | 72 ++++++-------------- common/values/custom_list_value.h | 59 +++++++++------- common/values/custom_list_value_test.cc | 15 ++-- common/values/custom_map_value.cc | 70 +++++++------------ common/values/custom_map_value.h | 53 +++++++++------ common/values/custom_map_value_test.cc | 5 +- common/values/custom_struct_value.cc | 38 +++-------- common/values/custom_struct_value.h | 59 ++++++++++------ common/values/custom_value.cc | 80 ---------------------- common/values/custom_value.h | 87 ------------------------ common/values/list_value.h | 2 +- common/values/list_value_builder.h | 8 --- common/values/map_value.h | 4 +- common/values/mutable_list_value_test.cc | 41 ++++++----- common/values/mutable_map_value_test.cc | 33 +++++---- common/values/opaque_value.cc | 2 +- common/values/opaque_value.h | 29 +++++--- common/values/struct_value.h | 2 +- common/values/value_builder.cc | 74 +++++++------------- eval/tests/modern_benchmark_test.cc | 10 +-- 20 files changed, 261 insertions(+), 482 deletions(-) delete mode 100644 common/values/custom_value.cc diff --git a/common/values/custom_list_value.cc b/common/values/custom_list_value.cc index 6c39f8bec..f67da1712 100644 --- a/common/values/custom_list_value.cc +++ b/common/values/custom_list_value.cc @@ -60,22 +60,6 @@ class EmptyListValue final : public common_internal::CompatListValue { size_t Size() const override { return 0; } - absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - ABSL_DCHECK(descriptor_pool != nullptr); - ABSL_DCHECK(message_factory != nullptr); - ABSL_DCHECK(json != nullptr); - 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())); - value_reflection.MutableListValue(json)->Clear(); - return absl::OkStatus(); - } - absl::Status ConvertToJsonArray( absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -102,7 +86,6 @@ class EmptyListValue final : public common_internal::CompatListValue { return CelValue::CreateError(&*error); } - using CompatListValue::Get; CelValue Get(google::protobuf::Arena* arena, int index) const override { if (arena == nullptr) { return (*this)[index]; @@ -112,12 +95,12 @@ class EmptyListValue final : public common_internal::CompatListValue { } private: - absl::Status GetImpl(size_t, absl::Nonnull, - absl::Nonnull, - absl::Nonnull, - absl::Nonnull) const override { - // Not reachable, `Get` performs index checking. - return absl::InternalError("unreachable"); + absl::Status Get(size_t index, absl::Nonnull, + absl::Nonnull, + absl::Nonnull, + absl::Nonnull result) const override { + *result = IndexOutOfBoundsError(index); + return absl::OkStatus(); } }; @@ -149,8 +132,8 @@ class CustomListValueInterfaceIterator final : public ValueIterator { "ValueIterator::Next() called when " "ValueIterator::HasNext() returns false"); } - return interface_.GetImpl(index_++, descriptor_pool, message_factory, arena, - result); + return interface_.Get(index_++, descriptor_pool, message_factory, arena, + result); } private: @@ -221,17 +204,6 @@ absl::Status CustomListValueInterface::SerializeTo( return absl::OkStatus(); } -absl::Status CustomListValueInterface::Get( - size_t index, absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, absl::Nonnull result) const { - if (ABSL_PREDICT_FALSE(index >= Size())) { - *result = IndexOutOfBoundsError(index); - return absl::OkStatus(); - } - return GetImpl(index, descriptor_pool, message_factory, arena, result); -} - absl::Status CustomListValueInterface::ForEach( ForEachWithIndexCallback callback, absl::Nonnull descriptor_pool, @@ -241,7 +213,7 @@ absl::Status CustomListValueInterface::ForEach( for (size_t index = 0; index < size; ++index) { Value element; CEL_RETURN_IF_ERROR( - GetImpl(index, descriptor_pool, message_factory, arena, &element)); + Get(index, descriptor_pool, message_factory, arena, &element)); CEL_ASSIGN_OR_RETURN(auto ok, callback(index, element)); if (!ok) { break; @@ -256,16 +228,12 @@ CustomListValueInterface::NewIterator() const { } absl::Status CustomListValueInterface::Equal( - const Value& other, + const ListValue& other, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, absl::Nonnull arena, absl::Nonnull result) const { - if (auto list_value = other.As(); list_value.has_value()) { - return ListValueEqual(*this, *list_value, descriptor_pool, message_factory, - arena, result); - } - *result = FalseValue(); - return absl::OkStatus(); + return ListValueEqual(*this, other, descriptor_pool, message_factory, arena, + result); } absl::Status CustomListValueInterface::Contains( @@ -301,7 +269,7 @@ NativeTypeId CustomListValue::GetTypeId() const { CustomListValueInterface::Content content = content_.To(); ABSL_DCHECK(content.interface != nullptr); - return NativeTypeId::Of(*content.interface); + return content.interface->GetNativeTypeId(); } return dispatcher_->get_type_id(dispatcher_, content_); } @@ -392,14 +360,14 @@ absl::Status CustomListValue::Equal( ABSL_DCHECK(arena != nullptr); ABSL_DCHECK(result != nullptr); - if (dispatcher_ == nullptr) { - CustomListValueInterface::Content content = - content_.To(); - ABSL_DCHECK(content.interface != nullptr); - return content.interface->Equal(other, descriptor_pool, message_factory, - arena, result); - } if (auto other_list_value = other.AsList(); other_list_value) { + if (dispatcher_ == nullptr) { + CustomListValueInterface::Content content = + content_.To(); + ABSL_DCHECK(content.interface != nullptr); + return content.interface->Equal(*other_list_value, descriptor_pool, + message_factory, arena, result); + } if (dispatcher_->equal != nullptr) { return dispatcher_->equal(dispatcher_, content_, *other_list_value, descriptor_pool, message_factory, arena, diff --git a/common/values/custom_list_value.h b/common/values/custom_list_value.h index 8d2d31430..e8dcfe080 100644 --- a/common/values/custom_list_value.h +++ b/common/values/custom_list_value.h @@ -167,43 +167,61 @@ struct CustomListValueDispatcher { absl::Nonnull clone; }; -class CustomListValueInterface : public CustomValueInterface { +class CustomListValueInterface { public: - static constexpr ValueKind kKind = ValueKind::kList; + CustomListValueInterface() = default; + CustomListValueInterface(const CustomListValueInterface&) = delete; + CustomListValueInterface(CustomListValueInterface&&) = delete; - ValueKind kind() const final { return kKind; } + virtual ~CustomListValueInterface() = default; - absl::string_view GetTypeName() const final { return "list"; } + CustomListValueInterface& operator=(const CustomListValueInterface&) = delete; + CustomListValueInterface& operator=(CustomListValueInterface&&) = delete; using ForEachCallback = absl::FunctionRef(const Value&)>; using ForEachWithIndexCallback = absl::FunctionRef(size_t, const Value&)>; - absl::Status SerializeTo( + private: + friend class CustomListValueInterfaceIterator; + friend class CustomListValue; + friend absl::Status common_internal::ListValueEqual( + const CustomListValueInterface& lhs, const ListValue& rhs, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, - absl::Nonnull output) const override; + absl::Nonnull arena, absl::Nonnull result); - absl::Status Equal( - const Value& other, + virtual std::string DebugString() const = 0; + + virtual absl::Status SerializeTo( absl::Nonnull descriptor_pool, absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override; + absl::Nonnull output) const; + + virtual absl::Status ConvertToJsonArray( + absl::Nonnull descriptor_pool, + absl::Nonnull message_factory, + absl::Nonnull json) const = 0; - bool IsZeroValue() const { return IsEmpty(); } + virtual absl::Status Equal( + const ListValue& other, + absl::Nonnull descriptor_pool, + absl::Nonnull message_factory, + absl::Nonnull arena, absl::Nonnull result) const; + + virtual bool IsZeroValue() const { return IsEmpty(); } virtual bool IsEmpty() const { return Size() == 0; } virtual size_t Size() const = 0; - // See ListValueInterface::Get for documentation. virtual absl::Status Get( size_t index, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, - absl::Nonnull arena, absl::Nonnull result) const; + absl::Nonnull arena, + absl::Nonnull result) const = 0; virtual absl::Status ForEach( ForEachWithIndexCallback callback, @@ -221,18 +239,7 @@ class CustomListValueInterface : public CustomValueInterface { virtual CustomListValue Clone(absl::Nonnull arena) const = 0; - protected: - friend class CustomListValueInterfaceIterator; - - virtual absl::Status GetImpl( - size_t index, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const = 0; - - private: - friend class CustomListValue; + virtual NativeTypeId GetNativeTypeId() const = 0; struct Content { absl::Nonnull interface; @@ -257,7 +264,7 @@ CustomListValue UnsafeCustomListValue( class CustomListValue final : private common_internal::ListValueMixin { public: - static constexpr ValueKind kKind = CustomListValueInterface::kKind; + static constexpr ValueKind kKind = ValueKind::kList; // Constructs a custom list value from an implementation of // `CustomListValueInterface` `interface` whose lifetime is tied to that of diff --git a/common/values/custom_list_value_test.cc b/common/values/custom_list_value_test.cc index 73d2557ce..4ed94ebc5 100644 --- a/common/values/custom_list_value_test.cc +++ b/common/values/custom_list_value_test.cc @@ -19,7 +19,6 @@ #include "google/protobuf/struct.pb.h" #include "absl/base/nullability.h" -#include "absl/base/optimization.h" #include "absl/status/status.h" #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" @@ -106,12 +105,11 @@ class CustomListValueInterfaceTest final : public CustomListValueInterface { } private: - absl::Status GetImpl( - size_t index, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override { + absl::Status Get(size_t index, + absl::Nonnull descriptor_pool, + absl::Nonnull message_factory, + absl::Nonnull arena, + absl::Nonnull result) const override { if (index == 0) { *result = TrueValue(); return absl::OkStatus(); @@ -120,7 +118,8 @@ class CustomListValueInterfaceTest final : public CustomListValueInterface { *result = IntValue(1); return absl::OkStatus(); } - ABSL_UNREACHABLE(); + *result = IndexOutOfBoundsError(index); + return absl::OkStatus(); } NativeTypeId GetNativeTypeId() const override { diff --git a/common/values/custom_map_value.cc b/common/values/custom_map_value.cc index 45c0d7b5f..894adad7f 100644 --- a/common/values/custom_map_value.cc +++ b/common/values/custom_map_value.cc @@ -87,22 +87,6 @@ class EmptyMapValue final : public common_internal::CompatMapValue { return NewEmptyValueIterator(); } - absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - ABSL_DCHECK(descriptor_pool != nullptr); - ABSL_DCHECK(message_factory != nullptr); - ABSL_DCHECK(json != nullptr); - 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())); - value_reflection.MutableListValue(json)->Clear(); - return absl::OkStatus(); - } - absl::Status ConvertToJsonObject( absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -131,7 +115,6 @@ class EmptyMapValue final : public common_internal::CompatMapValue { return absl::nullopt; } - using CompatMapValue::Has; absl::StatusOr Has(const CelValue& key) const override { return false; } int size() const override { return static_cast(Size()); } @@ -145,7 +128,7 @@ class EmptyMapValue final : public common_internal::CompatMapValue { } private: - absl::StatusOr FindImpl( + absl::StatusOr Find( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -154,7 +137,7 @@ class EmptyMapValue final : public common_internal::CompatMapValue { return false; } - absl::StatusOr HasImpl( + absl::StatusOr Has( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -173,11 +156,9 @@ absl::Nonnull EmptyCompatMapValue() { } // namespace common_internal -namespace { - -class CustomMapValueInterfaceKeysIterator final : public ValueIterator { +class CustomMapValueInterfaceIterator final : public ValueIterator { public: - explicit CustomMapValueInterfaceKeysIterator( + explicit CustomMapValueInterfaceIterator( absl::Nonnull interface) : interface_(interface) {} @@ -214,6 +195,8 @@ class CustomMapValueInterfaceKeysIterator final : public ValueIterator { absl::Nullable keys_iterator_; }; +namespace { + class CustomMapValueDispatcherKeysIterator final : public ValueIterator { public: explicit CustomMapValueDispatcherKeysIterator( @@ -302,8 +285,8 @@ absl::Status CustomMapValueInterface::ForEach( Value value; CEL_RETURN_IF_ERROR( iterator->Next(descriptor_pool, message_factory, arena, &key)); - CEL_ASSIGN_OR_RETURN(bool found, FindImpl(key, descriptor_pool, - message_factory, arena, &value)); + CEL_ASSIGN_OR_RETURN( + bool found, Find(key, descriptor_pool, message_factory, arena, &value)); if (!found) { value = ErrorValue(NoSuchKeyError(key)); } @@ -317,20 +300,16 @@ absl::Status CustomMapValueInterface::ForEach( absl::StatusOr> CustomMapValueInterface::NewIterator() const { - return std::make_unique(this); + return std::make_unique(this); } absl::Status CustomMapValueInterface::Equal( - const Value& other, + const MapValue& other, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, absl::Nonnull arena, absl::Nonnull result) const { - if (auto list_value = other.As(); list_value.has_value()) { - return MapValueEqual(*this, *list_value, descriptor_pool, message_factory, - arena, result); - } - *result = FalseValue(); - return absl::OkStatus(); + return MapValueEqual(*this, other, descriptor_pool, message_factory, arena, + result); } CustomMapValue::CustomMapValue() { @@ -343,7 +322,7 @@ NativeTypeId CustomMapValue::GetTypeId() const { CustomMapValueInterface::Content content = content_.To(); ABSL_DCHECK(content.interface != nullptr); - return NativeTypeId::Of(*content.interface); + return content.interface->GetNativeTypeId(); } return dispatcher_->get_type_id(dispatcher_, content_); } @@ -434,14 +413,14 @@ absl::Status CustomMapValue::Equal( ABSL_DCHECK(arena != nullptr); ABSL_DCHECK(result != nullptr); - if (dispatcher_ == nullptr) { - CustomMapValueInterface::Content content = - content_.To(); - ABSL_DCHECK(content.interface != nullptr); - return content.interface->Equal(other, descriptor_pool, message_factory, - arena, result); - } if (auto other_map_value = other.AsMap(); other_map_value) { + if (dispatcher_ == nullptr) { + CustomMapValueInterface::Content content = + content_.To(); + ABSL_DCHECK(content.interface != nullptr); + return content.interface->Equal(*other_map_value, descriptor_pool, + message_factory, arena, result); + } if (dispatcher_->equal != nullptr) { return dispatcher_->equal(dispatcher_, content_, *other_map_value, descriptor_pool, message_factory, arena, @@ -565,8 +544,8 @@ absl::StatusOr CustomMapValue::Find( content_.To(); ABSL_DCHECK(content.interface != nullptr); CEL_ASSIGN_OR_RETURN( - ok, content.interface->FindImpl(key, descriptor_pool, message_factory, - arena, result)); + ok, content.interface->Find(key, descriptor_pool, message_factory, + arena, result)); } else { CEL_ASSIGN_OR_RETURN( ok, dispatcher_->find(dispatcher_, content_, key, descriptor_pool, @@ -612,9 +591,8 @@ absl::Status CustomMapValue::Has( CustomMapValueInterface::Content content = content_.To(); ABSL_DCHECK(content.interface != nullptr); - CEL_ASSIGN_OR_RETURN( - has, content.interface->HasImpl(key, descriptor_pool, message_factory, - arena)); + 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, diff --git a/common/values/custom_map_value.h b/common/values/custom_map_value.h index 49e5b40ca..4520941ee 100644 --- a/common/values/custom_map_value.h +++ b/common/values/custom_map_value.h @@ -50,6 +50,7 @@ namespace cel { class Value; class ListValue; class CustomMapValueInterface; +class CustomMapValueInterfaceKeysIterator; class CustomMapValue; using CustomMapValueContent = CustomValueContent; @@ -175,30 +176,48 @@ struct CustomMapValueDispatcher { absl::Nonnull clone; }; -class CustomMapValueInterface : public CustomValueInterface { +class CustomMapValueInterface { public: - static constexpr ValueKind kKind = ValueKind::kMap; + CustomMapValueInterface() = default; + CustomMapValueInterface(const CustomMapValueInterface&) = delete; + CustomMapValueInterface(CustomMapValueInterface&&) = delete; - ValueKind kind() const final { return kKind; } + virtual ~CustomMapValueInterface() = default; - absl::string_view GetTypeName() const final { return "map"; } + CustomMapValueInterface& operator=(const CustomMapValueInterface&) = delete; + CustomMapValueInterface& operator=(CustomMapValueInterface&&) = delete; using ForEachCallback = absl::FunctionRef(const Value&, const Value&)>; - absl::Status SerializeTo( + private: + friend class CustomMapValueInterfaceIterator; + friend class CustomMapValue; + friend absl::Status common_internal::MapValueEqual( + const CustomMapValueInterface& lhs, const MapValue& rhs, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, - absl::Nonnull output) const override; + absl::Nonnull arena, absl::Nonnull result); - absl::Status Equal( - const Value& other, + virtual std::string DebugString() const = 0; + + virtual absl::Status SerializeTo( absl::Nonnull descriptor_pool, absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override; + absl::Nonnull output) const; - bool IsZeroValue() const { return IsEmpty(); } + virtual absl::Status ConvertToJsonObject( + absl::Nonnull descriptor_pool, + absl::Nonnull message_factory, + absl::Nonnull json) const = 0; + + virtual absl::Status Equal( + const MapValue& other, + absl::Nonnull descriptor_pool, + absl::Nonnull message_factory, + absl::Nonnull arena, absl::Nonnull result) const; + + virtual bool IsZeroValue() const { return IsEmpty(); } // Returns `true` if this map contains no entries, `false` otherwise. virtual bool IsEmpty() const { return Size() == 0; } @@ -228,24 +247,20 @@ class CustomMapValueInterface : public CustomValueInterface { virtual CustomMapValue Clone(absl::Nonnull arena) const = 0; - protected: - // Called by `Find` after performing various argument checks. - virtual absl::StatusOr FindImpl( + virtual absl::StatusOr Find( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, absl::Nonnull arena, absl::Nonnull result) const = 0; - // Called by `Has` after performing various argument checks. - virtual absl::StatusOr HasImpl( + virtual absl::StatusOr Has( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, absl::Nonnull arena) const = 0; - private: - friend class CustomMapValue; + virtual NativeTypeId GetNativeTypeId() const = 0; struct Content { absl::Nonnull interface; @@ -270,7 +285,7 @@ CustomMapValue UnsafeCustomMapValue( class CustomMapValue final : private common_internal::MapValueMixin { public: - static constexpr ValueKind kKind = CustomMapValueInterface::kKind; + static constexpr ValueKind kKind = ValueKind::kMap; // Constructs a custom map value from an implementation of // `CustomMapValueInterface` `interface` whose lifetime is tied to that of diff --git a/common/values/custom_map_value_test.cc b/common/values/custom_map_value_test.cc index 643d2f3f0..863779dc1 100644 --- a/common/values/custom_map_value_test.cc +++ b/common/values/custom_map_value_test.cc @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include #include #include #include @@ -125,7 +124,7 @@ class CustomMapValueInterfaceTest final : public CustomMapValueInterface { } private: - absl::StatusOr FindImpl( + absl::StatusOr Find( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -144,7 +143,7 @@ class CustomMapValueInterfaceTest final : public CustomMapValueInterface { return false; } - absl::StatusOr HasImpl( + absl::StatusOr Has( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, diff --git a/common/values/custom_struct_value.cc b/common/values/custom_struct_value.cc index 74b383bd0..9a05f7870 100644 --- a/common/values/custom_struct_value.cc +++ b/common/values/custom_struct_value.cc @@ -28,7 +28,6 @@ #include "common/native_type.h" #include "common/type.h" #include "common/value.h" -#include "common/values/custom_value.h" #include "common/values/values.h" #include "internal/status_macros.h" #include "internal/well_known_types.h" @@ -47,26 +46,7 @@ using ::cel::well_known_types::ValueReflection; } // namespace absl::Status CustomStructValueInterface::Equal( - const Value& other, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, absl::Nonnull result) const { - if (auto parsed_struct_value = other.AsCustomStruct(); - parsed_struct_value.has_value() && - NativeTypeId::Of(*this) == NativeTypeId::Of(*parsed_struct_value)) { - return EqualImpl(*parsed_struct_value, descriptor_pool, message_factory, - arena, result); - } - if (auto struct_value = other.AsStruct(); struct_value.has_value()) { - return common_internal::StructValueEqual( - *this, *struct_value, descriptor_pool, message_factory, arena, result); - } - *result = FalseValue(); - return absl::OkStatus(); -} - -absl::Status CustomStructValueInterface::EqualImpl( - const CustomStructValue& other, + const StructValue& other, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, absl::Nonnull arena, absl::Nonnull result) const { @@ -91,7 +71,7 @@ NativeTypeId CustomStructValue::GetTypeId() const { if (content.interface == nullptr) { return NativeTypeId(); } - return NativeTypeId::Of(*content.interface); + return content.interface->GetNativeTypeId(); } return dispatcher_->get_type_id(dispatcher_, content_); } @@ -215,14 +195,14 @@ absl::Status CustomStructValue::Equal( ABSL_DCHECK(result != nullptr); ABSL_DCHECK(*this); - if (dispatcher_ == nullptr) { - CustomStructValueInterface::Content content = - content_.To(); - ABSL_DCHECK(content.interface != nullptr); - return content.interface->Equal(other, descriptor_pool, message_factory, - arena, result); - } if (auto other_struct_value = other.AsStruct(); other_struct_value) { + if (dispatcher_ == nullptr) { + CustomStructValueInterface::Content content = + content_.To(); + ABSL_DCHECK(content.interface != nullptr); + return content.interface->Equal(*other_struct_value, descriptor_pool, + message_factory, arena, result); + } if (dispatcher_->equal != nullptr) { return dispatcher_->equal(dispatcher_, content_, *other_struct_value, descriptor_pool, message_factory, arena, diff --git a/common/values/custom_struct_value.h b/common/values/custom_struct_value.h index e7377a55e..614ffb8a6 100644 --- a/common/values/custom_struct_value.h +++ b/common/values/custom_struct_value.h @@ -176,25 +176,52 @@ struct CustomStructValueDispatcher { absl::Nonnull clone; }; -class CustomStructValueInterface : public CustomValueInterface { +class CustomStructValueInterface { public: - static constexpr ValueKind kKind = ValueKind::kStruct; + CustomStructValueInterface() = default; + CustomStructValueInterface(const CustomStructValueInterface&) = delete; + CustomStructValueInterface(CustomStructValueInterface&&) = delete; - ValueKind kind() const final { return kKind; } + virtual ~CustomStructValueInterface() = default; - virtual StructType GetRuntimeType() const { - return common_internal::MakeBasicStructType(GetTypeName()); - } + CustomStructValueInterface& operator=(const CustomStructValueInterface&) = + delete; + CustomStructValueInterface& operator=(CustomStructValueInterface&&) = delete; using ForEachFieldCallback = absl::FunctionRef(absl::string_view, const Value&)>; - absl::Status Equal( - const Value& other, + private: + friend class CustomStructValue; + friend absl::Status common_internal::StructValueEqual( + const CustomStructValueInterface& lhs, const StructValue& rhs, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override; + absl::Nonnull arena, absl::Nonnull result); + + virtual std::string DebugString() const = 0; + + virtual absl::Status SerializeTo( + absl::Nonnull descriptor_pool, + absl::Nonnull message_factory, + absl::Nonnull output) const = 0; + + virtual absl::Status ConvertToJsonObject( + absl::Nonnull descriptor_pool, + absl::Nonnull message_factory, + absl::Nonnull json) const = 0; + + virtual absl::string_view GetTypeName() const = 0; + + virtual StructType GetRuntimeType() const { + return common_internal::MakeBasicStructType(GetTypeName()); + } + + virtual absl::Status Equal( + const StructValue& other, + absl::Nonnull descriptor_pool, + absl::Nonnull message_factory, + absl::Nonnull arena, absl::Nonnull result) const; virtual bool IsZeroValue() const = 0; @@ -232,15 +259,7 @@ class CustomStructValueInterface : public CustomValueInterface { virtual CustomStructValue Clone( absl::Nonnull arena) const = 0; - protected: - virtual absl::Status EqualImpl( - const CustomStructValue& other, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, absl::Nonnull result) const; - - private: - friend class CustomStructValue; + virtual NativeTypeId GetNativeTypeId() const = 0; struct Content { absl::Nonnull interface; @@ -265,7 +284,7 @@ CustomStructValue UnsafeCustomStructValue( class CustomStructValue final : private common_internal::StructValueMixin { public: - static constexpr ValueKind kKind = CustomStructValueInterface::kKind; + static constexpr ValueKind kKind = ValueKind::kStruct; // Constructs a custom struct value from an implementation of // `CustomStructValueInterface` `interface` whose lifetime is tied to that of diff --git a/common/values/custom_value.cc b/common/values/custom_value.cc deleted file mode 100644 index 09d024ff1..000000000 --- a/common/values/custom_value.cc +++ /dev/null @@ -1,80 +0,0 @@ -// Copyright 2023 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include "absl/base/nullability.h" -#include "absl/log/absl_check.h" -#include "absl/status/status.h" -#include "absl/strings/str_cat.h" -#include "common/value.h" -#include "google/protobuf/descriptor.h" -#include "google/protobuf/io/zero_copy_stream.h" -#include "google/protobuf/message.h" - -namespace cel { - -absl::Status CustomValueInterface::SerializeTo( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull output) const { - ABSL_DCHECK(descriptor_pool != nullptr); - ABSL_DCHECK(message_factory != nullptr); - ABSL_DCHECK(output != nullptr); - - return absl::FailedPreconditionError( - absl::StrCat(GetTypeName(), " is unserializable")); -} - -absl::Status CustomValueInterface::ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const { - ABSL_DCHECK(descriptor_pool != nullptr); - ABSL_DCHECK(message_factory != nullptr); - ABSL_DCHECK(json != nullptr); - ABSL_DCHECK_EQ(json->GetDescriptor()->well_known_type(), - google::protobuf::Descriptor::WELLKNOWNTYPE_VALUE); - - return absl::FailedPreconditionError( - absl::StrCat(GetTypeName(), " is not convertable to JSON")); -} - -absl::Status CustomValueInterface::ConvertToJsonArray( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const { - ABSL_DCHECK(descriptor_pool != nullptr); - ABSL_DCHECK(message_factory != nullptr); - ABSL_DCHECK(json != nullptr); - ABSL_DCHECK_EQ(json->GetDescriptor()->well_known_type(), - google::protobuf::Descriptor::WELLKNOWNTYPE_LISTVALUE); - - return absl::FailedPreconditionError( - absl::StrCat(GetTypeName(), " is not convertable to JSON")); -} - -absl::Status CustomValueInterface::ConvertToJsonObject( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const { - ABSL_DCHECK(descriptor_pool != nullptr); - ABSL_DCHECK(message_factory != nullptr); - ABSL_DCHECK(json != nullptr); - ABSL_DCHECK_EQ(json->GetDescriptor()->well_known_type(), - google::protobuf::Descriptor::WELLKNOWNTYPE_STRUCT); - - return absl::FailedPreconditionError( - absl::StrCat(GetTypeName(), " is not convertable to JSON")); -} - -} // namespace cel diff --git a/common/values/custom_value.h b/common/values/custom_value.h index bc6daf7fb..8d3d9e165 100644 --- a/common/values/custom_value.h +++ b/common/values/custom_value.h @@ -21,23 +21,10 @@ #include #include #include -#include #include -#include "absl/base/nullability.h" -#include "absl/status/status.h" -#include "absl/strings/string_view.h" -#include "common/native_type.h" -#include "common/value_kind.h" -#include "google/protobuf/arena.h" -#include "google/protobuf/descriptor.h" -#include "google/protobuf/io/zero_copy_stream.h" -#include "google/protobuf/message.h" - namespace cel { -class Value; - // CustomValueContent is an opaque 16-byte trivially copyable value. The format // of the data stored within is unknown to everything except the the caller // which creates it. Do not try to interpret it otherwise. @@ -87,80 +74,6 @@ class CustomValueContent final { alignas(void*) std::byte raw_[16]; }; -class CustomValueInterface { - public: - CustomValueInterface(const CustomValueInterface&) = delete; - CustomValueInterface(CustomValueInterface&&) = delete; - - virtual ~CustomValueInterface() = default; - - CustomValueInterface& operator=(const CustomValueInterface&) = delete; - CustomValueInterface& operator=(CustomValueInterface&&) = delete; - - virtual ValueKind kind() const = 0; - - virtual absl::string_view GetTypeName() const = 0; - - virtual std::string DebugString() const = 0; - - // See Value::SerializeTo(). - virtual absl::Status SerializeTo( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull output) const; - - // See Value::ConvertToJson(). - virtual absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const; - - // See Value::ConvertToJsonArray(). - virtual absl::Status ConvertToJsonArray( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const; - - // See Value::ConvertToJsonObject(). - virtual absl::Status ConvertToJsonObject( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const; - - virtual absl::Status Equal( - const Value& other, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const = 0; - - protected: - CustomValueInterface() = default; - - private: - friend struct NativeTypeTraits; - - virtual NativeTypeId GetNativeTypeId() const = 0; -}; - -template <> -struct NativeTypeTraits final { - static NativeTypeId Id(const CustomValueInterface& custom_value_interface) { - return custom_value_interface.GetNativeTypeId(); - } -}; - -template -struct NativeTypeTraits< - T, std::enable_if_t, - std::negation>>>> - final { - static NativeTypeId Id(const CustomValueInterface& custom_value_interface) { - return NativeTypeTraits::Id(custom_value_interface); - } -}; - } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_VALUES_CUSTOM_VALUE_H_ diff --git a/common/values/list_value.h b/common/values/list_value.h index 1de4d864d..9a014e8a9 100644 --- a/common/values/list_value.h +++ b/common/values/list_value.h @@ -60,7 +60,7 @@ class TypeManager; class ListValue final : private common_internal::ListValueMixin { public: - static constexpr ValueKind kKind = CustomListValueInterface::kKind; + static constexpr ValueKind kKind = ValueKind::kList; // Move constructor for alternative struct values. template < diff --git a/common/values/list_value_builder.h b/common/values/list_value_builder.h index 0b3ebd133..542f61804 100644 --- a/common/values/list_value_builder.h +++ b/common/values/list_value_builder.h @@ -39,10 +39,6 @@ namespace common_internal { // `list_value_builder.cc`. class CompatListValue : public CustomListValueInterface, public google::api::expr::runtime::CelList { - public: - using CelList::Get; - using CustomListValueInterface::Get; - private: NativeTypeId GetNativeTypeId() const final { return NativeTypeId::For(); @@ -82,10 +78,6 @@ class MutableListValue : public CustomListValueInterface { // inheritance and `dynamic_cast`. class MutableCompatListValue : public MutableListValue, public google::api::expr::runtime::CelList { - public: - using CustomListValueInterface::Get; - using MutableListValue::Get; - private: NativeTypeId GetNativeTypeId() const final { return NativeTypeId::For(); diff --git a/common/values/map_value.h b/common/values/map_value.h index d08b9fd18..028345dd3 100644 --- a/common/values/map_value.h +++ b/common/values/map_value.h @@ -63,9 +63,7 @@ absl::Status CheckMapKey(const Value& key); class MapValue final : private common_internal::MapValueMixin { public: - using interface_type = MapValueInterface; - - static constexpr ValueKind kKind = CustomMapValueInterface::kKind; + static constexpr ValueKind kKind = ValueKind::kMap; // Move constructor for alternative struct values. template ; TEST_F(MutableListValueTest, DebugString) { auto* mutable_list_value = NewMutableListValue(arena()); - EXPECT_THAT(mutable_list_value->DebugString(), "[]"); + EXPECT_THAT(CustomListValue(mutable_list_value, arena()).DebugString(), "[]"); } TEST_F(MutableListValueTest, IsEmpty) { auto* mutable_list_value = NewMutableListValue(arena()); mutable_list_value->Reserve(1); - EXPECT_TRUE(mutable_list_value->IsEmpty()); + EXPECT_TRUE(CustomListValue(mutable_list_value, arena()).IsEmpty()); EXPECT_THAT(mutable_list_value->Append(StringValue("foo")), IsOk()); - EXPECT_FALSE(mutable_list_value->IsEmpty()); + EXPECT_FALSE(CustomListValue(mutable_list_value, arena()).IsEmpty()); } TEST_F(MutableListValueTest, Size) { auto* mutable_list_value = NewMutableListValue(arena()); mutable_list_value->Reserve(1); - EXPECT_THAT(mutable_list_value->Size(), 0); + EXPECT_THAT(CustomListValue(mutable_list_value, arena()).Size(), 0); EXPECT_THAT(mutable_list_value->Append(StringValue("foo")), IsOk()); - EXPECT_THAT(mutable_list_value->Size(), 1); + EXPECT_THAT(CustomListValue(mutable_list_value, arena()).Size(), 1); } TEST_F(MutableListValueTest, ForEach) { @@ -68,13 +68,15 @@ TEST_F(MutableListValueTest, ForEach) { elements.push_back(std::pair{index, value}); return true; }; - EXPECT_THAT(mutable_list_value->ForEach(for_each_callback, descriptor_pool(), - message_factory(), arena()), + EXPECT_THAT(CustomListValue(mutable_list_value, arena()) + .ForEach(for_each_callback, descriptor_pool(), + message_factory(), arena()), IsOk()); EXPECT_THAT(elements, IsEmpty()); EXPECT_THAT(mutable_list_value->Append(StringValue("foo")), IsOk()); - EXPECT_THAT(mutable_list_value->ForEach(for_each_callback, descriptor_pool(), - message_factory(), arena()), + EXPECT_THAT(CustomListValue(mutable_list_value, arena()) + .ForEach(for_each_callback, descriptor_pool(), + message_factory(), arena()), IsOk()); EXPECT_THAT(elements, UnorderedElementsAre(Pair(0, StringValueIs("foo")))); } @@ -82,11 +84,14 @@ TEST_F(MutableListValueTest, ForEach) { TEST_F(MutableListValueTest, NewIterator) { auto* mutable_list_value = NewMutableListValue(arena()); mutable_list_value->Reserve(1); - ASSERT_OK_AND_ASSIGN(auto iterator, mutable_list_value->NewIterator()); + ASSERT_OK_AND_ASSIGN( + auto iterator, + CustomListValue(mutable_list_value, arena()).NewIterator()); EXPECT_THAT(iterator->Next(descriptor_pool(), message_factory(), arena()), StatusIs(absl::StatusCode::kFailedPrecondition)); EXPECT_THAT(mutable_list_value->Append(StringValue("foo")), IsOk()); - ASSERT_OK_AND_ASSIGN(iterator, mutable_list_value->NewIterator()); + ASSERT_OK_AND_ASSIGN( + iterator, CustomListValue(mutable_list_value, arena()).NewIterator()); EXPECT_TRUE(iterator->HasNext()); EXPECT_THAT(iterator->Next(descriptor_pool(), message_factory(), arena()), IsOkAndHolds(StringValueIs("foo"))); @@ -99,15 +104,17 @@ TEST_F(MutableListValueTest, Get) { auto* mutable_list_value = NewMutableListValue(arena()); mutable_list_value->Reserve(1); Value value; - EXPECT_THAT(mutable_list_value->Get(0, descriptor_pool(), message_factory(), - arena(), &value), - IsOk()); + EXPECT_THAT( + CustomListValue(mutable_list_value, arena()) + .Get(0, descriptor_pool(), message_factory(), arena(), &value), + IsOk()); EXPECT_THAT(value, ErrorValueIs(StatusIs(absl::StatusCode::kInvalidArgument))); EXPECT_THAT(mutable_list_value->Append(StringValue("foo")), IsOk()); - EXPECT_THAT(mutable_list_value->Get(0, descriptor_pool(), message_factory(), - arena(), &value), - IsOk()); + EXPECT_THAT( + CustomListValue(mutable_list_value, arena()) + .Get(0, descriptor_pool(), message_factory(), arena(), &value), + IsOk()); EXPECT_THAT(value, StringValueIs("foo")); } diff --git a/common/values/mutable_map_value_test.cc b/common/values/mutable_map_value_test.cc index d0e12a815..2f08abe3f 100644 --- a/common/values/mutable_map_value_test.cc +++ b/common/values/mutable_map_value_test.cc @@ -45,23 +45,23 @@ using MutableMapValueTest = common_internal::ValueTest<>; TEST_F(MutableMapValueTest, DebugString) { auto mutable_map_value = NewMutableMapValue(arena()); - EXPECT_THAT(mutable_map_value->DebugString(), "{}"); + EXPECT_THAT(CustomMapValue(mutable_map_value, arena()).DebugString(), "{}"); } TEST_F(MutableMapValueTest, IsEmpty) { auto mutable_map_value = NewMutableMapValue(arena()); mutable_map_value->Reserve(1); - EXPECT_TRUE(mutable_map_value->IsEmpty()); + EXPECT_TRUE(CustomMapValue(mutable_map_value, arena()).IsEmpty()); EXPECT_THAT(mutable_map_value->Put(StringValue("foo"), IntValue(1)), IsOk()); - EXPECT_FALSE(mutable_map_value->IsEmpty()); + EXPECT_FALSE(CustomMapValue(mutable_map_value, arena()).IsEmpty()); } TEST_F(MutableMapValueTest, Size) { auto mutable_map_value = NewMutableMapValue(arena()); mutable_map_value->Reserve(1); - EXPECT_THAT(mutable_map_value->Size(), 0); + EXPECT_THAT(CustomMapValue(mutable_map_value, arena()).Size(), 0); EXPECT_THAT(mutable_map_value->Put(StringValue("foo"), IntValue(1)), IsOk()); - EXPECT_THAT(mutable_map_value->Size(), 1); + EXPECT_THAT(CustomMapValue(mutable_map_value, arena()).Size(), 1); } TEST_F(MutableMapValueTest, ListKeys) { @@ -69,9 +69,10 @@ TEST_F(MutableMapValueTest, ListKeys) { mutable_map_value->Reserve(1); ListValue keys; EXPECT_THAT(mutable_map_value->Put(StringValue("foo"), IntValue(1)), IsOk()); - EXPECT_THAT(mutable_map_value->ListKeys(descriptor_pool(), message_factory(), - arena(), &keys), - IsOk()); + EXPECT_THAT( + CustomMapValue(mutable_map_value, arena()) + .ListKeys(descriptor_pool(), message_factory(), arena(), &keys), + IsOk()); EXPECT_THAT(keys, ListValueIs(ListValueElements( UnorderedElementsAre(StringValueIs("foo")), descriptor_pool(), message_factory(), arena()))); @@ -86,13 +87,15 @@ TEST_F(MutableMapValueTest, ForEach) { entries.push_back(std::pair{key, value}); return true; }; - EXPECT_THAT(mutable_map_value->ForEach(for_each_callback, descriptor_pool(), - message_factory(), arena()), + EXPECT_THAT(CustomMapValue(mutable_map_value, arena()) + .ForEach(for_each_callback, descriptor_pool(), + message_factory(), arena()), IsOk()); EXPECT_THAT(entries, IsEmpty()); EXPECT_THAT(mutable_map_value->Put(StringValue("foo"), IntValue(1)), IsOk()); - EXPECT_THAT(mutable_map_value->ForEach(for_each_callback, descriptor_pool(), - message_factory(), arena()), + EXPECT_THAT(CustomMapValue(mutable_map_value, arena()) + .ForEach(for_each_callback, descriptor_pool(), + message_factory(), arena()), IsOk()); EXPECT_THAT(entries, UnorderedElementsAre(Pair(StringValueIs("foo"), IntValueIs(1)))); @@ -101,12 +104,14 @@ TEST_F(MutableMapValueTest, ForEach) { TEST_F(MutableMapValueTest, NewIterator) { auto mutable_map_value = NewMutableMapValue(arena()); mutable_map_value->Reserve(1); - ASSERT_OK_AND_ASSIGN(auto iterator, mutable_map_value->NewIterator()); + ASSERT_OK_AND_ASSIGN( + auto iterator, CustomMapValue(mutable_map_value, arena()).NewIterator()); EXPECT_FALSE(iterator->HasNext()); EXPECT_THAT(iterator->Next(descriptor_pool(), message_factory(), arena()), StatusIs(absl::StatusCode::kFailedPrecondition)); EXPECT_THAT(mutable_map_value->Put(StringValue("foo"), IntValue(1)), IsOk()); - ASSERT_OK_AND_ASSIGN(iterator, mutable_map_value->NewIterator()); + ASSERT_OK_AND_ASSIGN( + iterator, CustomMapValue(mutable_map_value, arena()).NewIterator()); EXPECT_TRUE(iterator->HasNext()); EXPECT_THAT(iterator->Next(descriptor_pool(), message_factory(), arena()), IsOkAndHolds(StringValueIs("foo"))); diff --git a/common/values/opaque_value.cc b/common/values/opaque_value.cc index c649ceac7..b99874c9b 100644 --- a/common/values/opaque_value.cc +++ b/common/values/opaque_value.cc @@ -156,7 +156,7 @@ NativeTypeId OpaqueValue::GetTypeId() const { if (content.interface == nullptr) { return NativeTypeId(); } - return NativeTypeId::Of(*content.interface); + return content.interface->GetNativeTypeId(); } return dispatcher_->get_type_id(dispatcher_, content_); } diff --git a/common/values/opaque_value.h b/common/values/opaque_value.h index 7414d78f4..bb689302f 100644 --- a/common/values/opaque_value.h +++ b/common/values/opaque_value.h @@ -102,25 +102,36 @@ struct OpaqueValueDispatcher { absl::Nonnull clone; }; -class OpaqueValueInterface : public CustomValueInterface { +class OpaqueValueInterface { public: - static constexpr ValueKind kKind = ValueKind::kOpaque; + OpaqueValueInterface() = default; + OpaqueValueInterface(const OpaqueValueInterface&) = delete; + OpaqueValueInterface(OpaqueValueInterface&&) = delete; + + virtual ~OpaqueValueInterface() = default; + + OpaqueValueInterface& operator=(const OpaqueValueInterface&) = delete; + OpaqueValueInterface& operator=(OpaqueValueInterface&&) = delete; + + private: + friend class OpaqueValue; - ValueKind kind() const final { return kKind; } + virtual std::string DebugString() const = 0; + + virtual absl::string_view GetTypeName() const = 0; virtual OpaqueType GetRuntimeType() const = 0; - absl::Status Equal( - const Value& other, + virtual absl::Status Equal( + const OpaqueValue& other, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, absl::Nonnull arena, - absl::Nonnull result) const override = 0; + absl::Nonnull result) const = 0; virtual OpaqueValue Clone(absl::Nonnull arena) const = 0; - private: - friend class OpaqueValue; + virtual NativeTypeId GetNativeTypeId() const = 0; struct Content { absl::Nonnull interface; @@ -143,7 +154,7 @@ OpaqueValue UnsafeOpaqueValue(absl::Nonnull class OpaqueValue : private common_internal::OpaqueValueMixin { public: - static constexpr ValueKind kKind = OpaqueValueInterface::kKind; + static constexpr ValueKind kKind = ValueKind::kOpaque; // Constructs an opaque value from an implementation of // `OpaqueValueInterface` `interface` whose lifetime is tied to that of diff --git a/common/values/struct_value.h b/common/values/struct_value.h index 26f492b58..325e80646 100644 --- a/common/values/struct_value.h +++ b/common/values/struct_value.h @@ -65,7 +65,7 @@ class TypeManager; class StructValue final : private common_internal::StructValueMixin { public: - static constexpr ValueKind kKind = CustomStructValueInterface::kKind; + static constexpr ValueKind kKind = ValueKind::kStruct; template < typename T, diff --git a/common/values/value_builder.cc b/common/values/value_builder.cc index 6c4bf18aa..300fb60ac 100644 --- a/common/values/value_builder.cc +++ b/common/values/value_builder.cc @@ -219,13 +219,6 @@ class CompatListValueImpl final : public CompatListValue { "]"); } - absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return ListValueToJson(elements_, descriptor_pool, message_factory, json); - } - absl::Status ConvertToJsonArray( absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -273,7 +266,6 @@ class CompatListValueImpl final : public CompatListValue { // Like `operator[](int)` above, but also accepts an arena. Prefer calling // this variant if the arena is known. - using CompatListValue::Get; CelValue Get(google::protobuf::Arena* arena, int index) const override { if (arena == nullptr) { arena = elements_.get_allocator().arena(); @@ -291,13 +283,16 @@ class CompatListValueImpl final : public CompatListValue { int size() const override { return static_cast(Size()); } protected: - absl::Status GetImpl( - size_t index, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override { - *result = elements_[index]; + absl::Status Get(size_t index, + absl::Nonnull descriptor_pool, + absl::Nonnull message_factory, + absl::Nonnull arena, + absl::Nonnull result) const override { + if (index >= elements_.size()) { + *result = IndexOutOfBoundsError(index); + } else { + *result = elements_[index]; + } return absl::OkStatus(); } @@ -361,13 +356,6 @@ class MutableCompatListValueImpl final : public MutableCompatListValue { "]"); } - absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return ListValueToJson(elements_, descriptor_pool, message_factory, json); - } - absl::Status ConvertToJsonArray( absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -415,7 +403,6 @@ class MutableCompatListValueImpl final : public MutableCompatListValue { // Like `operator[](int)` above, but also accepts an arena. Prefer calling // this variant if the arena is known. - using MutableCompatListValue::Get; CelValue Get(google::protobuf::Arena* arena, int index) const override { if (arena == nullptr) { arena = elements_.get_allocator().arena(); @@ -448,13 +435,16 @@ class MutableCompatListValueImpl final : public MutableCompatListValue { void Reserve(size_t capacity) const override { elements_.reserve(capacity); } protected: - absl::Status GetImpl( - size_t index, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override { - *result = elements_[index]; + absl::Status Get(size_t index, + absl::Nonnull descriptor_pool, + absl::Nonnull message_factory, + absl::Nonnull arena, + absl::Nonnull result) const override { + if (index >= elements_.size()) { + *result = IndexOutOfBoundsError(index); + } else { + *result = elements_[index]; + } return absl::OkStatus(); } @@ -937,13 +927,6 @@ class CompatMapValueImpl final : public CompatMapValue { return absl::StrCat("{", absl::StrJoin(map_, ", ", ValueFormatter{}), "}"); } - absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return MapValueToJson(map_, descriptor_pool, message_factory, json); - } - absl::Status ConvertToJsonObject( absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -1010,7 +993,6 @@ class CompatMapValueImpl final : public CompatMapValue { return absl::nullopt; } - using CompatMapValue::Has; absl::StatusOr Has(const CelValue& key) const override { // This check safeguards against issues with invalid key types such as NaN. CEL_RETURN_IF_ERROR(CelValue::CheckMapKeyType(key)); @@ -1028,7 +1010,7 @@ class CompatMapValueImpl final : public CompatMapValue { } protected: - absl::StatusOr FindImpl( + absl::StatusOr Find( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -1042,7 +1024,7 @@ class CompatMapValueImpl final : public CompatMapValue { return false; } - absl::StatusOr HasImpl( + absl::StatusOr Has( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -1109,13 +1091,6 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { return absl::StrCat("{", absl::StrJoin(map_, ", ", ValueFormatter{}), "}"); } - absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return MapValueToJson(map_, descriptor_pool, message_factory, json); - } - absl::Status ConvertToJsonObject( absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -1182,7 +1157,6 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { return absl::nullopt; } - using MutableCompatMapValue::Has; absl::StatusOr Has(const CelValue& key) const override { // This check safeguards against issues with invalid key types such as NaN. CEL_RETURN_IF_ERROR(CelValue::CheckMapKeyType(key)); @@ -1222,7 +1196,7 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { void Reserve(size_t capacity) const override { map_.reserve(capacity); } protected: - absl::StatusOr FindImpl( + absl::StatusOr Find( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -1236,7 +1210,7 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { return false; } - absl::StatusOr HasImpl( + absl::StatusOr Has( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, diff --git a/eval/tests/modern_benchmark_test.cc b/eval/tests/modern_benchmark_test.cc index 43e761f53..46afe6520 100644 --- a/eval/tests/modern_benchmark_test.cc +++ b/eval/tests/modern_benchmark_test.cc @@ -384,12 +384,6 @@ class RequestMapImpl : public CustomMapValueInterface { std::string DebugString() const override { return "RequestMapImpl"; } - absl::Status ConvertToJson(absl::Nonnull, - absl::Nonnull, - absl::Nonnull) const override { - return absl::UnimplementedError("Unsupported"); - } - absl::Status ConvertToJsonObject( absl::Nonnull, absl::Nonnull, @@ -403,7 +397,7 @@ class RequestMapImpl : public CustomMapValueInterface { protected: // Called by `Find` after performing various argument checks. - absl::StatusOr FindImpl( + absl::StatusOr Find( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -426,7 +420,7 @@ class RequestMapImpl : public CustomMapValueInterface { } // Called by `Has` after performing various argument checks. - absl::StatusOr HasImpl( + absl::StatusOr Has( const Value& key, absl::Nonnull descriptor_pool, absl::Nonnull message_factory,