From 422950834999d43876802b8f7b0e8692a7d818ce Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Wed, 4 Dec 2024 15:18:30 -0800 Subject: [PATCH] Fix the DynamicMessage implementation of DeleteMapValue. PiperOrigin-RevId: 702879013 --- src/google/protobuf/BUILD.bazel | 2 + src/google/protobuf/dynamic_message.cc | 13 ++++ src/google/protobuf/map_test.inc | 86 ++++++++++++++++++++++++ src/google/protobuf/reflection_tester.cc | 8 ++- src/google/protobuf/reflection_tester.h | 5 +- 5 files changed, 112 insertions(+), 2 deletions(-) diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel index 22020db4aed8..174966d44a4a 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/src/google/protobuf/BUILD.bazel @@ -1274,6 +1274,7 @@ cc_library( ":lite_test_util", ":port", ":protobuf", + ":test_textproto", "//src/google/protobuf/testing", "//src/google/protobuf/testing:file", "@com_google_absl//absl/base:config", @@ -1702,6 +1703,7 @@ cc_test( ":port", ":protobuf", ":protobuf_lite", + ":test_textproto", ":test_util", ":test_util2", "//src/google/protobuf/io", diff --git a/src/google/protobuf/dynamic_message.cc b/src/google/protobuf/dynamic_message.cc index d71cc8713a37..7ad7d2997052 100644 --- a/src/google/protobuf/dynamic_message.cc +++ b/src/google/protobuf/dynamic_message.cc @@ -216,6 +216,7 @@ class DynamicMapField final const MapKey& map_key, MapValueRef* val); static void ClearMapNoSyncImpl(MapFieldBase& base); + static bool DeleteMapValueImpl(MapFieldBase& map, const MapKey& map_key); static void UnsafeShallowSwapImpl(MapFieldBase& lhs, MapFieldBase& rhs) { static_cast(lhs).Swap( @@ -259,6 +260,18 @@ void DynamicMapField::ClearMapNoSyncImpl(MapFieldBase& base) { self.map_.clear(); } +bool DynamicMapField::DeleteMapValueImpl(MapFieldBase& base, + const MapKey& map_key) { + auto& self = static_cast(base); + auto it = self.map_.find(map_key); + if (it == self.map_.end()) return false; + if (self.arena() == nullptr) { + it->second.DeleteData(); + } + self.map_.erase(it); + return true; +} + void DynamicMapField::AllocateMapValue(MapValueRef* map_val) { const FieldDescriptor* val_des = default_entry_->GetDescriptor()->map_value(); map_val->SetType(val_des->cpp_type()); diff --git a/src/google/protobuf/map_test.inc b/src/google/protobuf/map_test.inc index af699eda3534..741455e5ef44 100644 --- a/src/google/protobuf/map_test.inc +++ b/src/google/protobuf/map_test.inc @@ -49,6 +49,7 @@ #include "google/protobuf/port.h" #include "google/protobuf/reflection.h" #include "google/protobuf/reflection_ops.h" +#include "google/protobuf/test_textproto.h" #include "google/protobuf/test_util2.h" #include "google/protobuf/text_format.h" #include "google/protobuf/util/message_differencer.h" @@ -1247,6 +1248,91 @@ TEST_F(MapImplTest, SwapFieldArenaReflection) { } } +TEST_F(MapImplTest, DeleteMapValues) { + auto* descriptor = UNITTEST::TestMap::descriptor(); + + DynamicMessageFactory factory; + std::unique_ptr dyn_message(factory.GetPrototype(descriptor)->New()); + std::unique_ptr gen_message = std::make_unique(); + Arena arena; + + for (auto* message : {gen_message.get(), dyn_message.get(), + gen_message->New(&arena), dyn_message->New(&arena)}) { + MapReflectionTester reflection_tester(descriptor); + reflection_tester.SetMapFieldsViaMapReflection(message); + + // Each maps has two elements. Let's remove the smallest one first, so that + // we can test the result. Then remove the rest and test again. + + for (int i = 0; i < descriptor->field_count(); ++i) { + auto* field = descriptor->field(i); + if (!field->is_map()) continue; + + MapIterator it = reflection_tester.MapBegin(message, field->name()); + MapIterator end = reflection_tester.MapEnd(message, field->name()); + // It's unordered, so search for it. + if (it == end) continue; + auto next = it; + ++next; + ASSERT_NE(next, end); + if (next.GetKey() < it.GetKey()) { + it = next; + } + + EXPECT_EQ(reflection_tester.MapSize(*message, field->name()), 2); + reflection_tester.DeleteMapValueViaMapReflection( + message, field->name(), it.GetKey()); + EXPECT_EQ(reflection_tester.MapSize(*message, field->name()), 1); + } + + EXPECT_THAT(*message, EqualsProto(R"pb( + map_int32_int32 { key: 1 value: 1 } + map_int64_int64 { key: 1 value: 1 } + map_uint32_uint32 { key: 1 value: 1 } + map_uint64_uint64 { key: 1 value: 1 } + map_sint32_sint32 { key: 1 value: 1 } + map_sint64_sint64 { key: 1 value: 1 } + map_fixed32_fixed32 { key: 1 value: 1 } + map_fixed64_fixed64 { key: 1 value: 1 } + map_sfixed32_sfixed32 { key: 1 value: 1 } + map_sfixed64_sfixed64 { key: 1 value: 1 } + map_int32_float { key: 1 value: 1 } + map_int32_double { key: 1 value: 1 } + map_bool_bool { key: true value: true } + map_string_string { + key: "This is another very long string that goes in the heap" + value: "This is another very long string that goes in the heap" + } + map_int32_bytes { + key: 1 + value: "This is another very long string that goes in the heap" + } + map_int32_enum { key: 1 value: MAP_ENUM_BAZ } + map_int32_foreign_message { + key: 1 + value { c: 1 } + } + )pb")); + + for (int i = 0; i < descriptor->field_count(); ++i) { + auto* field = descriptor->field(i); + if (!field->is_map()) continue; + + MapIterator it = reflection_tester.MapBegin(message, field->name()); + MapIterator end = reflection_tester.MapEnd(message, field->name()); + + while (it != end) { + auto next = it; + ++next; + reflection_tester.DeleteMapValueViaMapReflection( + message, field->name(), it.GetKey()); + it = next; + } + EXPECT_EQ(reflection_tester.MapSize(*message, field->name()), 0); + } + } +} + TEST_F(MapImplTest, CopyAssignMapIterator) { TestMap message; MapReflectionTester reflection_tester(UNITTEST::TestMap::descriptor()); diff --git a/src/google/protobuf/reflection_tester.cc b/src/google/protobuf/reflection_tester.cc index e7ed260c729c..b02d3d7d3246 100644 --- a/src/google/protobuf/reflection_tester.cc +++ b/src/google/protobuf/reflection_tester.cc @@ -192,7 +192,7 @@ MapReflectionTester::MapReflectionTester(const Descriptor* base_descriptor) } // Shorthand to get a FieldDescriptor for a field of unittest::TestMap. -const FieldDescriptor* MapReflectionTester::F(const std::string& name) { +const FieldDescriptor* MapReflectionTester::F(absl::string_view name) { const FieldDescriptor* result = nullptr; result = base_descriptor_->FindFieldByName(name); ABSL_CHECK(result != nullptr); @@ -611,6 +611,12 @@ void MapReflectionTester::GetMapValueViaMapReflection( map_key, map_val)); } +void MapReflectionTester::DeleteMapValueViaMapReflection( + Message* message, absl::string_view field_name, const MapKey& map_key) { + const Reflection* reflection = message->GetReflection(); + reflection->DeleteMapValue(message, F(field_name), map_key); +} + Message* MapReflectionTester::GetMapEntryViaReflection( Message* message, const std::string& field_name, int index) { const Reflection* reflection = message->GetReflection(); diff --git a/src/google/protobuf/reflection_tester.h b/src/google/protobuf/reflection_tester.h index 4d258485b109..263b99285a7f 100644 --- a/src/google/protobuf/reflection_tester.h +++ b/src/google/protobuf/reflection_tester.h @@ -40,6 +40,9 @@ class MapReflectionTester { void GetMapValueViaMapReflection(Message* message, const std::string& field_name, const MapKey& map_key, MapValueRef* map_val); + void DeleteMapValueViaMapReflection(Message* message, + absl::string_view field_name, + const MapKey& map_key); Message* GetMapEntryViaReflection(Message* message, const std::string& field_name, int index); MapIterator MapBegin(Message* message, const std::string& field_name); @@ -63,7 +66,7 @@ class MapReflectionTester { } private: - const FieldDescriptor* F(const std::string& name); + const FieldDescriptor* F(absl::string_view name); const Descriptor* base_descriptor_;