Skip to content

Lock down custom value interfaces #1382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 26, 2025
Merged

Lock down custom value interfaces #1382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 20 additions & 52 deletions common/values/custom_list_value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,22 +60,6 @@ class EmptyListValue final : public common_internal::CompatListValue {

size_t Size() const override { return 0; }

absl::Status ConvertToJson(
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::Message*> 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<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
Expand All @@ -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];
Expand All @@ -112,12 +95,12 @@ class EmptyListValue final : public common_internal::CompatListValue {
}

private:
absl::Status GetImpl(size_t, absl::Nonnull<const google::protobuf::DescriptorPool*>,
absl::Nonnull<google::protobuf::MessageFactory*>,
absl::Nonnull<google::protobuf::Arena*>,
absl::Nonnull<Value*>) const override {
// Not reachable, `Get` performs index checking.
return absl::InternalError("unreachable");
absl::Status Get(size_t index, absl::Nonnull<const google::protobuf::DescriptorPool*>,
absl::Nonnull<google::protobuf::MessageFactory*>,
absl::Nonnull<google::protobuf::Arena*>,
absl::Nonnull<Value*> result) const override {
*result = IndexOutOfBoundsError(index);
return absl::OkStatus();
}
};

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -221,17 +204,6 @@ absl::Status CustomListValueInterface::SerializeTo(
return absl::OkStatus();
}

absl::Status CustomListValueInterface::Get(
size_t index, absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> 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<const google::protobuf::DescriptorPool*> descriptor_pool,
Expand All @@ -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;
Expand All @@ -256,16 +228,12 @@ CustomListValueInterface::NewIterator() const {
}

absl::Status CustomListValueInterface::Equal(
const Value& other,
const ListValue& other,
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> result) const {
if (auto list_value = other.As<ListValue>(); 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(
Expand Down Expand Up @@ -301,7 +269,7 @@ NativeTypeId CustomListValue::GetTypeId() const {
CustomListValueInterface::Content content =
content_.To<CustomListValueInterface::Content>();
ABSL_DCHECK(content.interface != nullptr);
return NativeTypeId::Of(*content.interface);
return content.interface->GetNativeTypeId();
}
return dispatcher_->get_type_id(dispatcher_, content_);
}
Expand Down Expand Up @@ -392,14 +360,14 @@ absl::Status CustomListValue::Equal(
ABSL_DCHECK(arena != nullptr);
ABSL_DCHECK(result != nullptr);

if (dispatcher_ == nullptr) {
CustomListValueInterface::Content content =
content_.To<CustomListValueInterface::Content>();
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<CustomListValueInterface::Content>();
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,
Expand Down
59 changes: 33 additions & 26 deletions common/values/custom_list_value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,43 +167,61 @@ struct CustomListValueDispatcher {
absl::Nonnull<Clone> 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<absl::StatusOr<bool>(const Value&)>;

using ForEachWithIndexCallback =
absl::FunctionRef<absl::StatusOr<bool>(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<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::io::ZeroCopyOutputStream*> output) const override;
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> result);

absl::Status Equal(
const Value& other,
virtual std::string DebugString() const = 0;

virtual absl::Status SerializeTo(
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::Arena*> arena,
absl::Nonnull<Value*> result) const override;
absl::Nonnull<google::protobuf::io::ZeroCopyOutputStream*> output) const;

virtual absl::Status ConvertToJsonArray(
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::Message*> json) const = 0;

bool IsZeroValue() const { return IsEmpty(); }
virtual absl::Status Equal(
const ListValue& other,
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> 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<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> result) const;
absl::Nonnull<google::protobuf::Arena*> arena,
absl::Nonnull<Value*> result) const = 0;

virtual absl::Status ForEach(
ForEachWithIndexCallback callback,
Expand All @@ -221,18 +239,7 @@ class CustomListValueInterface : public CustomValueInterface {

virtual CustomListValue Clone(absl::Nonnull<google::protobuf::Arena*> arena) const = 0;

protected:
friend class CustomListValueInterfaceIterator;

virtual absl::Status GetImpl(
size_t index,
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::Arena*> arena,
absl::Nonnull<Value*> result) const = 0;

private:
friend class CustomListValue;
virtual NativeTypeId GetNativeTypeId() const = 0;

struct Content {
absl::Nonnull<const CustomListValueInterface*> interface;
Expand All @@ -257,7 +264,7 @@ CustomListValue UnsafeCustomListValue(
class CustomListValue final
: private common_internal::ListValueMixin<CustomListValue> {
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
Expand Down
15 changes: 7 additions & 8 deletions common/values/custom_list_value_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -106,12 +105,11 @@ class CustomListValueInterfaceTest final : public CustomListValueInterface {
}

private:
absl::Status GetImpl(
size_t index,
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::Arena*> arena,
absl::Nonnull<Value*> result) const override {
absl::Status Get(size_t index,
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
absl::Nonnull<google::protobuf::Arena*> arena,
absl::Nonnull<Value*> result) const override {
if (index == 0) {
*result = TrueValue();
return absl::OkStatus();
Expand All @@ -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 {
Expand Down
Loading