Skip to content

Fix bug in ::Clone impl for repeated fields #2220

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
copybara-service merged 1 commit into from
Aug 20, 2026
Merged

Fix bug in ::Clone impl for repeated fields #2220

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
40 changes: 39 additions & 1 deletion common/values/parsed_json_list_value_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

#include <cstddef>
#include <optional>
#include <utility>
#include <vector>

Expand All @@ -21,14 +22,15 @@
#include "absl/status/status_matchers.h"
#include "absl/status/statusor.h"
#include "absl/strings/string_view.h"
#include "absl/types/optional.h"
#include "common/memory.h"
#include "common/type.h"
#include "common/value.h"
#include "common/value_kind.h"
#include "common/value_testing.h"
#include "internal/parse_text_proto.h"
#include "internal/testing.h"
#include "cel/expr/conformance/proto3/test_all_types.pb.h"
#include "google/protobuf/arena.h"
#include "google/protobuf/io/zero_copy_stream_impl_lite.h"

namespace cel {
Expand Down Expand Up @@ -285,5 +287,41 @@ TEST_F(ParsedJsonListValueTest, Contains_Dynamic) {
IsOkAndHolds(BoolValueIs(true)));
}

TEST_F(ParsedJsonListValueTest, CloneDefault) {
ParsedJsonListValue value;
EXPECT_FALSE(value.Clone(arena()));
}

TEST_F(ParsedJsonListValueTest, CloneSameArena) {
ParsedJsonListValue value(DynamicParseTextProto<google::protobuf::ListValue>(
R"pb(values { null_value: NULL_VALUE }
values { bool_value: true })pb"),
arena());
auto cloned = value.Clone(arena());
EXPECT_THAT(
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
}

TEST_F(ParsedJsonListValueTest, CloneDifferentArena) {
google::protobuf::Arena other_arena;
ParsedJsonListValue value(
::cel::internal::DynamicParseTextProto<google::protobuf::ListValue>(
&other_arena,
R"pb(values { null_value: NULL_VALUE }
values { bool_value: true })pb",
descriptor_pool(), message_factory()),
&other_arena);
auto cloned = value.Clone(arena());
EXPECT_THAT(
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
EXPECT_EQ(cloned.Size(), 2);
EXPECT_THAT(cloned.Get(0, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(IsNullValue()));
EXPECT_THAT(cloned.Get(1, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
}

} // namespace
} // namespace cel
51 changes: 51 additions & 0 deletions common/values/parsed_json_map_value_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,5 +336,56 @@ TEST_F(ParsedJsonMapValueTest, NewIterator2) {
IsOkAndHolds(Eq(std::nullopt)));
}

TEST_F(ParsedJsonMapValueTest, CloneDefault) {
ParsedJsonMapValue value;
EXPECT_FALSE(value.Clone(arena()));
}

TEST_F(ParsedJsonMapValueTest, CloneSameArena) {
ParsedJsonMapValue value(DynamicParseTextProto<google::protobuf::Struct>(R"pb(
fields {
key: "foo"
value: { null_value: NULL_VALUE }
}
fields {
key: "bar"
value: { bool_value: true }
})pb"),
arena());
auto cloned = value.Clone(arena());
EXPECT_THAT(
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
}

TEST_F(ParsedJsonMapValueTest, CloneDifferentArena) {
google::protobuf::Arena other_arena;
ParsedJsonMapValue value(
::cel::internal::DynamicParseTextProto<google::protobuf::Struct>(
&other_arena,
R"pb(
fields {
key: "foo"
value: { null_value: NULL_VALUE }
}
fields {
key: "bar"
value: { bool_value: true }
})pb",
descriptor_pool(), message_factory()),
&other_arena);
auto cloned = value.Clone(arena());
EXPECT_THAT(
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
EXPECT_EQ(cloned.Size(), 2);
EXPECT_THAT(cloned.Get(StringValue("foo"), descriptor_pool(),
message_factory(), arena()),
IsOkAndHolds(IsNullValue()));
EXPECT_THAT(cloned.Get(StringValue("bar"), descriptor_pool(),
message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
}

} // namespace
} // namespace cel
3 changes: 1 addition & 2 deletions common/values/parsed_map_field_value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,8 @@ bool ParsedMapFieldValue::IsZeroValue() const { return IsEmpty(); }
ParsedMapFieldValue ParsedMapFieldValue::Clone(
google::protobuf::Arena* absl_nonnull arena) const {
ABSL_DCHECK(arena != nullptr);
ABSL_DCHECK(*this);

if (ABSL_PREDICT_FALSE(field_ == nullptr)) {
if (ABSL_PREDICT_FALSE(!*this)) {
return ParsedMapFieldValue();
}
if (arena_ == arena) {
Expand Down
42 changes: 42 additions & 0 deletions common/values/parsed_map_field_value_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,5 +640,47 @@ TEST_F(ParsedMapFieldValueTest, NewIterator2) {
IsOkAndHolds(Eq(std::nullopt)));
}

TEST_F(ParsedMapFieldValueTest, CloneDefault) {
ParsedMapFieldValue value;
EXPECT_FALSE(value.Clone(arena()));
}

TEST_F(ParsedMapFieldValueTest, CloneSameArena) {
ParsedMapFieldValue value(
DynamicParseTextProto<TestAllTypesProto3>(R"pb(
map_string_string { key: "foo" value: "bar" }
map_string_string { key: "bar" value: "foo" }
)pb"),
DynamicGetField<TestAllTypesProto3>("map_string_string"), arena());
auto cloned = value.Clone(arena());
EXPECT_THAT(
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
}

TEST_F(ParsedMapFieldValueTest, CloneDifferentArena) {
google::protobuf::Arena other_arena;
ParsedMapFieldValue value(
::cel::internal::DynamicParseTextProto<TestAllTypesProto3>(
&other_arena,
R"pb(
map_string_string { key: "foo" value: "bar" }
map_string_string { key: "bar" value: "foo" }
)pb",
descriptor_pool(), message_factory()),
DynamicGetField<TestAllTypesProto3>("map_string_string"), &other_arena);
auto cloned = value.Clone(arena());
EXPECT_THAT(
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
EXPECT_EQ(cloned.Size(), 2);
EXPECT_THAT(cloned.Get(StringValue("foo"), descriptor_pool(),
message_factory(), arena()),
IsOkAndHolds(StringValueIs("bar")));
EXPECT_THAT(cloned.Get(StringValue("bar"), descriptor_pool(),
message_factory(), arena()),
IsOkAndHolds(StringValueIs("foo")));
}

} // namespace
} // namespace cel
72 changes: 64 additions & 8 deletions common/values/parsed_repeated_field_value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "common/values/parsed_repeated_field_value.h"

#include <cstddef>
#include <cstdint>
#include <limits>
#include <memory>
#include <string>
Expand Down Expand Up @@ -147,24 +148,79 @@ absl::Status ParsedRepeatedFieldValue::Equal(

bool ParsedRepeatedFieldValue::IsZeroValue() const { return IsEmpty(); }

namespace {

template <typename T>
void CopyRepeatedFieldImpl(const google::protobuf::Reflection* absl_nonnull reflection,
const google::protobuf::Message& src_message,
google::protobuf::Message* absl_nonnull dst_message,
const google::protobuf::FieldDescriptor* absl_nonnull field) {
auto src_field = reflection->GetRepeatedFieldRef<T>(src_message, field);
auto dst_field =
reflection->GetMutableRepeatedFieldRef<T>(dst_message, field);
dst_field.CopyFrom(src_field);
}

void CopyRepeatedField(const google::protobuf::Reflection* absl_nonnull reflection,
const google::protobuf::Message& src_message,
google::protobuf::Message* absl_nonnull dst_message,
const google::protobuf::FieldDescriptor* absl_nonnull field) {
switch (field->cpp_type()) {
case google::protobuf::FieldDescriptor::CPPTYPE_INT32:
CopyRepeatedFieldImpl<int32_t>(reflection, src_message, dst_message,
field);
break;
case google::protobuf::FieldDescriptor::CPPTYPE_INT64:
CopyRepeatedFieldImpl<int64_t>(reflection, src_message, dst_message,
field);
break;
case google::protobuf::FieldDescriptor::CPPTYPE_UINT32:
CopyRepeatedFieldImpl<uint32_t>(reflection, src_message, dst_message,
field);
break;
case google::protobuf::FieldDescriptor::CPPTYPE_UINT64:
CopyRepeatedFieldImpl<uint64_t>(reflection, src_message, dst_message,
field);
break;
case google::protobuf::FieldDescriptor::CPPTYPE_DOUBLE:
CopyRepeatedFieldImpl<double>(reflection, src_message, dst_message,
field);
break;
case google::protobuf::FieldDescriptor::CPPTYPE_FLOAT:
CopyRepeatedFieldImpl<float>(reflection, src_message, dst_message, field);
break;
case google::protobuf::FieldDescriptor::CPPTYPE_BOOL:
CopyRepeatedFieldImpl<bool>(reflection, src_message, dst_message, field);
break;
case google::protobuf::FieldDescriptor::CPPTYPE_ENUM:
CopyRepeatedFieldImpl<int32_t>(reflection, src_message, dst_message,
field);
break;
case google::protobuf::FieldDescriptor::CPPTYPE_STRING:
CopyRepeatedFieldImpl<std::string>(reflection, src_message, dst_message,
field);
break;
case google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE:
CopyRepeatedFieldImpl<google::protobuf::Message>(reflection, src_message,
dst_message, field);
break;
}
}

} // namespace

ParsedRepeatedFieldValue ParsedRepeatedFieldValue::Clone(
google::protobuf::Arena* absl_nonnull arena) const {
ABSL_DCHECK(arena != nullptr);
ABSL_DCHECK(*this);

if (ABSL_PREDICT_FALSE(field_ == nullptr)) {
if (ABSL_PREDICT_FALSE(!*this)) {
return ParsedRepeatedFieldValue();
}
if (arena_ == arena) {
return *this;
}
auto field = message_->GetReflection()->GetRepeatedFieldRef<google::protobuf::Message>(
*message_, field_);
auto* cloned_message = message_->New(arena);
auto cloned_field =
cloned_message->GetReflection()
->GetMutableRepeatedFieldRef<google::protobuf::Message>(cloned_message, field_);
cloned_field.CopyFrom(field);
CopyRepeatedField(GetReflection(), *message_, cloned_message, field_);
return ParsedRepeatedFieldValue(cloned_message, field_, arena);
}

Expand Down
74 changes: 73 additions & 1 deletion common/values/parsed_repeated_field_value_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

#include <cstddef>
#include <optional>
#include <utility>
#include <vector>

Expand All @@ -22,14 +23,15 @@
#include "absl/status/statusor.h"
#include "absl/strings/string_view.h"
#include "absl/time/time.h"
#include "absl/types/optional.h"
#include "common/memory.h"
#include "common/type.h"
#include "common/value.h"
#include "common/value_kind.h"
#include "common/value_testing.h"
#include "internal/parse_text_proto.h"
#include "internal/testing.h"
#include "cel/expr/conformance/proto3/test_all_types.pb.h"
#include "google/protobuf/arena.h"
#include "google/protobuf/io/zero_copy_stream_impl_lite.h"

namespace cel {
Expand All @@ -45,6 +47,7 @@ using ::cel::test::DurationValueIs;
using ::cel::test::ErrorValueIs;
using ::cel::test::IntValueIs;
using ::cel::test::IsNullValue;
using ::cel::test::StringValueIs;
using ::cel::test::UintValueIs;
using ::testing::_;
using ::testing::ElementsAre;
Expand Down Expand Up @@ -446,5 +449,74 @@ TEST_F(ParsedRepeatedFieldValueTest, Contains) {
IsOkAndHolds(BoolValueIs(false)));
}

TEST_F(ParsedRepeatedFieldValueTest, CloneDefault) {
ParsedRepeatedFieldValue value;
EXPECT_FALSE(value.Clone(arena()));
}

TEST_F(ParsedRepeatedFieldValueTest, CloneSameArena) {
ParsedRepeatedFieldValue value(
DynamicParseTextProto<TestAllTypesProto3>(R"pb(repeated_int64: 1
repeated_int64: 2)pb"),
DynamicGetField<TestAllTypesProto3>("repeated_int64"), arena());
auto cloned = value.Clone(arena());
EXPECT_THAT(
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
}

TEST_F(ParsedRepeatedFieldValueTest, CloneDifferentArenaInt64) {
google::protobuf::Arena other_arena;
ParsedRepeatedFieldValue value(
::cel::internal::DynamicParseTextProto<TestAllTypesProto3>(
&other_arena, R"pb(repeated_int64: 1 repeated_int64: 2)pb",
descriptor_pool(), message_factory()),
DynamicGetField<TestAllTypesProto3>("repeated_int64"), &other_arena);
auto cloned = value.Clone(arena());
EXPECT_THAT(
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
EXPECT_EQ(cloned.Size(), 2);
EXPECT_THAT(cloned.Get(0, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(IntValueIs(1)));
EXPECT_THAT(cloned.Get(1, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(IntValueIs(2)));
}

TEST_F(ParsedRepeatedFieldValueTest, CloneDifferentArenaString) {
google::protobuf::Arena other_arena;
ParsedRepeatedFieldValue value(
::cel::internal::DynamicParseTextProto<TestAllTypesProto3>(
&other_arena, R"pb(repeated_string: "foo" repeated_string: "bar")pb",
descriptor_pool(), message_factory()),
DynamicGetField<TestAllTypesProto3>("repeated_string"), &other_arena);
auto cloned = value.Clone(arena());
EXPECT_THAT(
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
EXPECT_EQ(cloned.Size(), 2);
EXPECT_THAT(cloned.Get(0, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(StringValueIs("foo")));
EXPECT_THAT(cloned.Get(1, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(StringValueIs("bar")));
}

TEST_F(ParsedRepeatedFieldValueTest, CloneDifferentArenaMessage) {
google::protobuf::Arena other_arena;
ParsedRepeatedFieldValue value(
::cel::internal::DynamicParseTextProto<TestAllTypesProto3>(
&other_arena,
R"pb(repeated_nested_message: { bb: 1 }
repeated_nested_message: { bb: 2 })pb",
descriptor_pool(), message_factory()),
DynamicGetField<TestAllTypesProto3>("repeated_nested_message"),
&other_arena);
auto cloned = value.Clone(arena());
EXPECT_THAT(
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
IsOkAndHolds(BoolValueIs(true)));
EXPECT_EQ(cloned.Size(), 2);
}

} // namespace
} // namespace cel
Loading