Skip to content

Commit c702523

Browse files
jckingcopybara-github
jcking
authored and
copybara-github
committed
Lock down custom value interfaces
PiperOrigin-RevId: 740781275
1 parent f852cd0 commit c702523

20 files changed

+261
-482
lines changed

‎common/values/custom_list_value.cc‎

Lines changed: 20 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -60,22 +60,6 @@ class EmptyListValue final : public common_internal::CompatListValue {
6060

6161
size_t Size() const override { return 0; }
6262

63-
absl::Status ConvertToJson(
64-
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
65-
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
66-
absl::Nonnull<google::protobuf::Message*> json) const override {
67-
ABSL_DCHECK(descriptor_pool != nullptr);
68-
ABSL_DCHECK(message_factory != nullptr);
69-
ABSL_DCHECK(json != nullptr);
70-
ABSL_DCHECK_EQ(json->GetDescriptor()->well_known_type(),
71-
google::protobuf::Descriptor::WELLKNOWNTYPE_VALUE);
72-
73-
ValueReflection value_reflection;
74-
CEL_RETURN_IF_ERROR(value_reflection.Initialize(json->GetDescriptor()));
75-
value_reflection.MutableListValue(json)->Clear();
76-
return absl::OkStatus();
77-
}
78-
7963
absl::Status ConvertToJsonArray(
8064
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
8165
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
@@ -102,7 +86,6 @@ class EmptyListValue final : public common_internal::CompatListValue {
10286
return CelValue::CreateError(&*error);
10387
}
10488

105-
using CompatListValue::Get;
10689
CelValue Get(google::protobuf::Arena* arena, int index) const override {
10790
if (arena == nullptr) {
10891
return (*this)[index];
@@ -112,12 +95,12 @@ class EmptyListValue final : public common_internal::CompatListValue {
11295
}
11396

11497
private:
115-
absl::Status GetImpl(size_t, absl::Nonnull<const google::protobuf::DescriptorPool*>,
116-
absl::Nonnull<google::protobuf::MessageFactory*>,
117-
absl::Nonnull<google::protobuf::Arena*>,
118-
absl::Nonnull<Value*>) const override {
119-
// Not reachable, `Get` performs index checking.
120-
return absl::InternalError("unreachable");
98+
absl::Status Get(size_t index, absl::Nonnull<const google::protobuf::DescriptorPool*>,
99+
absl::Nonnull<google::protobuf::MessageFactory*>,
100+
absl::Nonnull<google::protobuf::Arena*>,
101+
absl::Nonnull<Value*> result) const override {
102+
*result = IndexOutOfBoundsError(index);
103+
return absl::OkStatus();
121104
}
122105
};
123106

@@ -149,8 +132,8 @@ class CustomListValueInterfaceIterator final : public ValueIterator {
149132
"ValueIterator::Next() called when "
150133
"ValueIterator::HasNext() returns false");
151134
}
152-
return interface_.GetImpl(index_++, descriptor_pool, message_factory, arena,
153-
result);
135+
return interface_.Get(index_++, descriptor_pool, message_factory, arena,
136+
result);
154137
}
155138

156139
private:
@@ -221,17 +204,6 @@ absl::Status CustomListValueInterface::SerializeTo(
221204
return absl::OkStatus();
222205
}
223206

224-
absl::Status CustomListValueInterface::Get(
225-
size_t index, absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
226-
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
227-
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> result) const {
228-
if (ABSL_PREDICT_FALSE(index >= Size())) {
229-
*result = IndexOutOfBoundsError(index);
230-
return absl::OkStatus();
231-
}
232-
return GetImpl(index, descriptor_pool, message_factory, arena, result);
233-
}
234-
235207
absl::Status CustomListValueInterface::ForEach(
236208
ForEachWithIndexCallback callback,
237209
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
@@ -241,7 +213,7 @@ absl::Status CustomListValueInterface::ForEach(
241213
for (size_t index = 0; index < size; ++index) {
242214
Value element;
243215
CEL_RETURN_IF_ERROR(
244-
GetImpl(index, descriptor_pool, message_factory, arena, &element));
216+
Get(index, descriptor_pool, message_factory, arena, &element));
245217
CEL_ASSIGN_OR_RETURN(auto ok, callback(index, element));
246218
if (!ok) {
247219
break;
@@ -256,16 +228,12 @@ CustomListValueInterface::NewIterator() const {
256228
}
257229

258230
absl::Status CustomListValueInterface::Equal(
259-
const Value& other,
231+
const ListValue& other,
260232
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
261233
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
262234
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> result) const {
263-
if (auto list_value = other.As<ListValue>(); list_value.has_value()) {
264-
return ListValueEqual(*this, *list_value, descriptor_pool, message_factory,
265-
arena, result);
266-
}
267-
*result = FalseValue();
268-
return absl::OkStatus();
235+
return ListValueEqual(*this, other, descriptor_pool, message_factory, arena,
236+
result);
269237
}
270238

271239
absl::Status CustomListValueInterface::Contains(
@@ -301,7 +269,7 @@ NativeTypeId CustomListValue::GetTypeId() const {
301269
CustomListValueInterface::Content content =
302270
content_.To<CustomListValueInterface::Content>();
303271
ABSL_DCHECK(content.interface != nullptr);
304-
return NativeTypeId::Of(*content.interface);
272+
return content.interface->GetNativeTypeId();
305273
}
306274
return dispatcher_->get_type_id(dispatcher_, content_);
307275
}
@@ -392,14 +360,14 @@ absl::Status CustomListValue::Equal(
392360
ABSL_DCHECK(arena != nullptr);
393361
ABSL_DCHECK(result != nullptr);
394362

395-
if (dispatcher_ == nullptr) {
396-
CustomListValueInterface::Content content =
397-
content_.To<CustomListValueInterface::Content>();
398-
ABSL_DCHECK(content.interface != nullptr);
399-
return content.interface->Equal(other, descriptor_pool, message_factory,
400-
arena, result);
401-
}
402363
if (auto other_list_value = other.AsList(); other_list_value) {
364+
if (dispatcher_ == nullptr) {
365+
CustomListValueInterface::Content content =
366+
content_.To<CustomListValueInterface::Content>();
367+
ABSL_DCHECK(content.interface != nullptr);
368+
return content.interface->Equal(*other_list_value, descriptor_pool,
369+
message_factory, arena, result);
370+
}
403371
if (dispatcher_->equal != nullptr) {
404372
return dispatcher_->equal(dispatcher_, content_, *other_list_value,
405373
descriptor_pool, message_factory, arena,

‎common/values/custom_list_value.h‎

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -167,43 +167,61 @@ struct CustomListValueDispatcher {
167167
absl::Nonnull<Clone> clone;
168168
};
169169

170-
class CustomListValueInterface : public CustomValueInterface {
170+
class CustomListValueInterface {
171171
public:
172-
static constexpr ValueKind kKind = ValueKind::kList;
172+
CustomListValueInterface() = default;
173+
CustomListValueInterface(const CustomListValueInterface&) = delete;
174+
CustomListValueInterface(CustomListValueInterface&&) = delete;
173175

174-
ValueKind kind() const final { return kKind; }
176+
virtual ~CustomListValueInterface() = default;
175177

176-
absl::string_view GetTypeName() const final { return "list"; }
178+
CustomListValueInterface& operator=(const CustomListValueInterface&) = delete;
179+
CustomListValueInterface& operator=(CustomListValueInterface&&) = delete;
177180

178181
using ForEachCallback = absl::FunctionRef<absl::StatusOr<bool>(const Value&)>;
179182

180183
using ForEachWithIndexCallback =
181184
absl::FunctionRef<absl::StatusOr<bool>(size_t, const Value&)>;
182185

183-
absl::Status SerializeTo(
186+
private:
187+
friend class CustomListValueInterfaceIterator;
188+
friend class CustomListValue;
189+
friend absl::Status common_internal::ListValueEqual(
190+
const CustomListValueInterface& lhs, const ListValue& rhs,
184191
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
185192
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
186-
absl::Nonnull<google::protobuf::io::ZeroCopyOutputStream*> output) const override;
193+
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> result);
187194

188-
absl::Status Equal(
189-
const Value& other,
195+
virtual std::string DebugString() const = 0;
196+
197+
virtual absl::Status SerializeTo(
190198
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
191199
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
192-
absl::Nonnull<google::protobuf::Arena*> arena,
193-
absl::Nonnull<Value*> result) const override;
200+
absl::Nonnull<google::protobuf::io::ZeroCopyOutputStream*> output) const;
201+
202+
virtual absl::Status ConvertToJsonArray(
203+
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
204+
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
205+
absl::Nonnull<google::protobuf::Message*> json) const = 0;
194206

195-
bool IsZeroValue() const { return IsEmpty(); }
207+
virtual absl::Status Equal(
208+
const ListValue& other,
209+
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
210+
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
211+
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> result) const;
212+
213+
virtual bool IsZeroValue() const { return IsEmpty(); }
196214

197215
virtual bool IsEmpty() const { return Size() == 0; }
198216

199217
virtual size_t Size() const = 0;
200218

201-
// See ListValueInterface::Get for documentation.
202219
virtual absl::Status Get(
203220
size_t index,
204221
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
205222
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
206-
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> result) const;
223+
absl::Nonnull<google::protobuf::Arena*> arena,
224+
absl::Nonnull<Value*> result) const = 0;
207225

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

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

224-
protected:
225-
friend class CustomListValueInterfaceIterator;
226-
227-
virtual absl::Status GetImpl(
228-
size_t index,
229-
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
230-
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
231-
absl::Nonnull<google::protobuf::Arena*> arena,
232-
absl::Nonnull<Value*> result) const = 0;
233-
234-
private:
235-
friend class CustomListValue;
242+
virtual NativeTypeId GetNativeTypeId() const = 0;
236243

237244
struct Content {
238245
absl::Nonnull<const CustomListValueInterface*> interface;
@@ -257,7 +264,7 @@ CustomListValue UnsafeCustomListValue(
257264
class CustomListValue final
258265
: private common_internal::ListValueMixin<CustomListValue> {
259266
public:
260-
static constexpr ValueKind kKind = CustomListValueInterface::kKind;
267+
static constexpr ValueKind kKind = ValueKind::kList;
261268

262269
// Constructs a custom list value from an implementation of
263270
// `CustomListValueInterface` `interface` whose lifetime is tied to that of

‎common/values/custom_list_value_test.cc‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919

2020
#include "google/protobuf/struct.pb.h"
2121
#include "absl/base/nullability.h"
22-
#include "absl/base/optimization.h"
2322
#include "absl/status/status.h"
2423
#include "absl/status/status_matchers.h"
2524
#include "absl/status/statusor.h"
@@ -106,12 +105,11 @@ class CustomListValueInterfaceTest final : public CustomListValueInterface {
106105
}
107106

108107
private:
109-
absl::Status GetImpl(
110-
size_t index,
111-
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
112-
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
113-
absl::Nonnull<google::protobuf::Arena*> arena,
114-
absl::Nonnull<Value*> result) const override {
108+
absl::Status Get(size_t index,
109+
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
110+
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
111+
absl::Nonnull<google::protobuf::Arena*> arena,
112+
absl::Nonnull<Value*> result) const override {
115113
if (index == 0) {
116114
*result = TrueValue();
117115
return absl::OkStatus();
@@ -120,7 +118,8 @@ class CustomListValueInterfaceTest final : public CustomListValueInterface {
120118
*result = IntValue(1);
121119
return absl::OkStatus();
122120
}
123-
ABSL_UNREACHABLE();
121+
*result = IndexOutOfBoundsError(index);
122+
return absl::OkStatus();
124123
}
125124

126125
NativeTypeId GetNativeTypeId() const override {

0 commit comments

Comments
 (0)