From 76d39615019109808db8fe05afbe9dea1daba039 Mon Sep 17 00:00:00 2001 From: Adam Cozzette Date: Thu, 6 Dec 2018 10:02:42 -0800 Subject: [PATCH] Cherry-picked some internal fixes from Google - Avoid using typeid() so that we don't depend on RTTI - Change internal AP MapData() to MutableMapData() and added GetMapData() --- python/google/protobuf/pyext/map_container.cc | 8 +++--- .../protobuf/generated_message_reflection.cc | 10 ++++++- .../protobuf/generated_message_reflection.h | 7 +++-- src/google/protobuf/map_field.h | 2 +- src/google/protobuf/map_test.cc | 13 +++++++++- src/google/protobuf/message.h | 7 ++++- src/google/protobuf/reflection_ops.cc | 26 ++++++++++++------- src/google/protobuf/text_format.cc | 2 +- src/google/protobuf/wire_format.cc | 8 +++--- 9 files changed, 58 insertions(+), 25 deletions(-) diff --git a/python/google/protobuf/pyext/map_container.cc b/python/google/protobuf/pyext/map_container.cc index 9f74e2a5c5..d85840219d 100644 --- a/python/google/protobuf/pyext/map_container.cc +++ b/python/google/protobuf/pyext/map_container.cc @@ -346,11 +346,11 @@ PyObject* MapReflectionFriend::MergeFrom(PyObject* _self, PyObject* arg) { const Message* other_message = other_map->message; const Reflection* reflection = message->GetReflection(); const Reflection* other_reflection = other_message->GetReflection(); - internal::MapFieldBase* field = reflection->MapData( + internal::MapFieldBase* field = reflection->MutableMapData( message, self->parent_field_descriptor); - internal::MapFieldBase* other_field = - other_reflection->MapData(const_cast(other_message), - self->parent_field_descriptor); + const internal::MapFieldBase* other_field = + other_reflection->GetMapData(*other_message, + self->parent_field_descriptor); field->MergeFrom(*other_field); self->version++; Py_RETURN_NONE; diff --git a/src/google/protobuf/generated_message_reflection.cc b/src/google/protobuf/generated_message_reflection.cc index f926da274e..f1f9f20c37 100644 --- a/src/google/protobuf/generated_message_reflection.cc +++ b/src/google/protobuf/generated_message_reflection.cc @@ -2224,7 +2224,7 @@ void* GeneratedMessageReflection::RepeatedFieldData( } } -MapFieldBase* GeneratedMessageReflection::MapData( +MapFieldBase* GeneratedMessageReflection::MutableMapData( Message* message, const FieldDescriptor* field) const { USAGE_CHECK(IsMapFieldInApi(field), "GetMapData", @@ -2232,6 +2232,14 @@ MapFieldBase* GeneratedMessageReflection::MapData( return MutableRaw(message, field); } +const MapFieldBase* GeneratedMessageReflection::GetMapData( + const Message& message, const FieldDescriptor* field) const { + USAGE_CHECK(IsMapFieldInApi(field), + "GetMapData", + "Field is not a map field."); + return &(GetRaw(message, field)); +} + namespace { // Helper function to transform migration schema into reflection schema. diff --git a/src/google/protobuf/generated_message_reflection.h b/src/google/protobuf/generated_message_reflection.h index 0ffb3ff270..d1544784c0 100644 --- a/src/google/protobuf/generated_message_reflection.h +++ b/src/google/protobuf/generated_message_reflection.h @@ -670,8 +670,11 @@ class GeneratedMessageReflection final : public Reflection { Message* sub_message, const FieldDescriptor* field) const; - internal::MapFieldBase* MapData(Message* message, - const FieldDescriptor* field) const override; + internal::MapFieldBase* MutableMapData( + Message* message, const FieldDescriptor* field) const override; + + const internal::MapFieldBase* GetMapData( + const Message& message, const FieldDescriptor* field) const override; friend inline // inline so nobody can call this function. void diff --git a/src/google/protobuf/map_field.h b/src/google/protobuf/map_field.h index bba5d8ae7f..5e598b8e0d 100644 --- a/src/google/protobuf/map_field.h +++ b/src/google/protobuf/map_field.h @@ -742,7 +742,7 @@ class PROTOBUF_EXPORT MapIterator { public: MapIterator(Message* message, const FieldDescriptor* field) { const Reflection* reflection = message->GetReflection(); - map_ = reflection->MapData(message, field); + map_ = reflection->MutableMapData(message, field); key_.SetType(field->message_type()->FindFieldByName("key")->cpp_type()); value_.SetType(field->message_type()->FindFieldByName("value")->cpp_type()); map_->InitializeIterator(this); diff --git a/src/google/protobuf/map_test.cc b/src/google/protobuf/map_test.cc index 41d9c379ec..06ebf5d3f8 100644 --- a/src/google/protobuf/map_test.cc +++ b/src/google/protobuf/map_test.cc @@ -2042,19 +2042,30 @@ TEST(GeneratedMapFieldTest, DynamicMessageMergeFromDynamicMessage) { unittest::TestMap::descriptor()); reflection_tester.SetMapFieldsViaMapReflection(message1.get()); + // message2 is created by same factory. std::unique_ptr message2; message2.reset( factory.GetPrototype(unittest::TestMap::descriptor())->New()); reflection_tester.SetMapFieldsViaMapReflection(message2.get()); + // message3 is created by different factory. + DynamicMessageFactory factory3; + std::unique_ptr message3; + message3.reset( + factory3.GetPrototype(unittest::TestMap::descriptor())->New()); + reflection_tester.SetMapFieldsViaMapReflection(message3.get()); + message2->MergeFrom(*message1); + message3->MergeFrom(*message1); // Test MergeFrom does not sync to repeated fields and // there is no duplicate keys in text format. - string output1, output2; + string output1, output2, output3; TextFormat::PrintToString(*message1, &output1); TextFormat::PrintToString(*message2, &output2); + TextFormat::PrintToString(*message3, &output3); EXPECT_EQ(output1, output2); + EXPECT_EQ(output1, output3); } TEST(GeneratedMapFieldTest, DynamicMessageCopyFrom) { diff --git a/src/google/protobuf/message.h b/src/google/protobuf/message.h index 0734df5c58..02cca530c9 100644 --- a/src/google/protobuf/message.h +++ b/src/google/protobuf/message.h @@ -1049,11 +1049,16 @@ class PROTOBUF_EXPORT Reflection { // Help method for MapIterator. friend class MapIterator; - virtual internal::MapFieldBase* MapData( + virtual internal::MapFieldBase* MutableMapData( Message* /* message */, const FieldDescriptor* /* field */) const { return NULL; } + virtual const internal::MapFieldBase* GetMapData( + const Message& /* message */, const FieldDescriptor* /* field */) const { + return NULL; + } + GOOGLE_DISALLOW_EVIL_CONSTRUCTORS(Reflection); }; diff --git a/src/google/protobuf/reflection_ops.cc b/src/google/protobuf/reflection_ops.cc index 233ba7df12..86229aa7ba 100644 --- a/src/google/protobuf/reflection_ops.cc +++ b/src/google/protobuf/reflection_ops.cc @@ -77,6 +77,10 @@ void ReflectionOps::Merge(const Message& from, Message* to) { const Reflection* from_reflection = GetReflectionOrDie(from); const Reflection* to_reflection = GetReflectionOrDie(*to); + bool is_from_generated = (from_reflection->GetMessageFactory() == + google::protobuf::MessageFactory::generated_factory()); + bool is_to_generated = (to_reflection->GetMessageFactory() == + google::protobuf::MessageFactory::generated_factory()); std::vector fields; from_reflection->ListFields(from, &fields); @@ -84,15 +88,17 @@ void ReflectionOps::Merge(const Message& from, Message* to) { const FieldDescriptor* field = fields[i]; if (field->is_repeated()) { - if (field->is_map()) { - MapFieldBase* from_field = - from_reflection->MapData(const_cast(&from), field); + // Use map reflection if both are in map status and have the + // same map type to avoid sync with repeated field. + // Note: As from and to messages have the same descriptor, the + // map field types are the same if they are both generated + // messages or both dynamic messages. + if (is_from_generated == is_to_generated && field->is_map()) { + const MapFieldBase* from_field = + from_reflection->GetMapData(from, field); MapFieldBase* to_field = - to_reflection->MapData(const_cast(to), field); - // Use map reflection if both are in map status and have the - // same map type to avoid sync with repeated field. - if (to_field->IsMapValid() && from_field->IsMapValid() - && typeid(*from_field) == typeid(*to_field)) { + to_reflection->MutableMapData(to, field); + if (to_field->IsMapValid() && from_field->IsMapValid()) { to_field->MergeFrom(*from_field); continue; } @@ -189,8 +195,8 @@ bool ReflectionOps::IsInitialized(const Message& message) { if (field->is_map()) { const FieldDescriptor* value_field = field->message_type()->field(1); if (value_field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) { - MapFieldBase* map_field = - reflection->MapData(const_cast(&message), field); + const MapFieldBase* map_field = + reflection->GetMapData(message, field); if (map_field->IsMapValid()) { MapIterator iter(const_cast(&message), field); MapIterator end(const_cast(&message), field); diff --git a/src/google/protobuf/text_format.cc b/src/google/protobuf/text_format.cc index a434509ef8..50e2b1cb3c 100644 --- a/src/google/protobuf/text_format.cc +++ b/src/google/protobuf/text_format.cc @@ -2049,7 +2049,7 @@ bool MapFieldPrinterHelper::SortMap( std::vector* sorted_map_field) { bool need_release = false; const MapFieldBase& base = - *reflection->MapData(const_cast(&message), field); + *reflection->GetMapData(message, field); if (base.IsRepeatedFieldValid()) { const RepeatedPtrField& map_field = diff --git a/src/google/protobuf/wire_format.cc b/src/google/protobuf/wire_format.cc index 0342a101a8..1ddc1d51be 100644 --- a/src/google/protobuf/wire_format.cc +++ b/src/google/protobuf/wire_format.cc @@ -907,8 +907,8 @@ void WireFormat::SerializeFieldWithCachedSizes( // internal state and existing references that came from map reflection remain // valid for both reading and writing. if (field->is_map()) { - MapFieldBase* map_field = - message_reflection->MapData(const_cast(&message), field); + const MapFieldBase* map_field = + message_reflection->GetMapData(message, field); if (map_field->IsMapValid()) { if (output->IsSerializationDeterministic()) { std::vector sorted_key_list = @@ -1243,8 +1243,8 @@ size_t WireFormat::FieldDataOnlyByteSize( size_t data_size = 0; if (field->is_map()) { - MapFieldBase* map_field = - message_reflection->MapData(const_cast(&message), field); + const MapFieldBase* map_field = + message_reflection->GetMapData(message, field); if (map_field->IsMapValid()) { MapIterator iter(const_cast(&message), field); MapIterator end(const_cast(&message), field);