Fix the DynamicMessage implementation of DeleteMapValue.

PiperOrigin-RevId: 702879013

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",

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<DynamicMapField&>(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<DynamicMapField&>(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());

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<Message> dyn_message(factory.GetPrototype(descriptor)->New());
+  std::unique_ptr<Message> gen_message = std::make_unique<TestMap>();
+  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());

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();

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_;