Why is the map reflection interface private?

[jeremyong]

If I want to perform operations on messages that have maps, I have no easy way of handling the map internals without manually circumventing the public reflection API.

The MapReflectionTester does perfectly reasonable things but can only do so because it is a friend class of the reflection class.

[xfxyjwf]

It's private because we are not sure that's the right public API for users. Currently you can access map fields as repeated message fields in the reflection API. For example, Reflection::GetRepeatedMessage() can return a map entry message. Is that sufficient for your use case?

[jeremyong]

That should be fine. Is there documentation describing what the field descriptor for those repeated messages is?

[jeremyong]

The main issue with the repeated message field API that I can see is the following:

1. The internal representation of the fields isn't standardized in any way. Field names are internal as are field numbers, so any implementation that relies on them won't be reliable in the future.
2. The act of finding a key in a map through the reflection interface can't leverage the existing map implementation.

Any effort to make the map reflection API public would be appreciated. I'm considering forking the project just to move the API to the public scope...

[jeremyong]

Also, the other things you have to juggle yourself if you use the raw repeated field reflection representation are the types of the keys and values. It's a very cumbersome interface and I'm finding myself re-writing maps while doing it!

[haberman]

This API is currently experimental, and we want to get a little more feedback before committing to it. If you did want to try it out and offered feedback, it would help move us closer towards making the API public.

[haberman]

Closing as I believe we answered the original question.

[sewenew]

Hi,

Does the map reflection interface has any progress? It seem that these interfaces are still private with the latest release: 3.8.0.

Regards

[haberman]

Good question. The interface hasn't run into many problems, but we also haven't gotten much feedback about it yet. Also the maps code is a mess internally and needs to be cleaned up. I'd feel more comfortable committing to this interface once that cleanup has happened.

[sewenew]

Hi @haberman ,

I wrote a Redis Module: redis-protobuf, which uses Protobuf Reflection to read and write Protobuf messages with Redis. I was trying to use the map reflection interface by making the corresponding methods public, and recompiling the code. And I'd like to give you some feedback on the map reflection interface.

By now, I can only use the following method to get the map value of a given key:

bool Reflection::InsertOrLookupMapValue(Message* message, const FieldDescriptor* field, const MapKey& key, MapValueRef* val) const

This is fine if the Protobuf message is mutable. However, if the message is const, and I only want to get the map value without modifying the map, I cannot use that method.

const Message *msg = get_a_const_msg();
// CANNOT work, since msg is const.
msg->GetReflection()->InsertOrLookupMapValue(msg, field, key, val);

My suggestion is that you can define another method which does a readonly lookup of the map value:

bool Reflection::LookupMapValue(const Message &message, const FieldDescriptor *field, const MapKey &key, MapValueRef *val) const

If key is in map field, saves the value pointer to val and returns true. Otherwise, returns false, and leave the val untouched.

There're similar problems with the MapBegin and MapEnd methods:

MapIterator MapBegin(Message* message, const FieldDescriptor* field) const;

MapIterator MapEnd(Message* message, const FieldDescriptor* field) const;

These two methods return MapIterator, and I can only create begin and end iterators for mutable message.

You should also provide methods that return ConstMapIterator, so that I can iterate the map values of const message:

ConstMapIterator MapBegin(const Message &message, const FieldDescriptor* field) const;

ConstMapIterator MapEnd(const Message &message, const FieldDescriptor* field) const;

Hope this helps :)

Regards

[antoniovicente]

Here is some possible feedback based on what I'm seeing in some heapz profiles from some of my Envoy memory loadtests:
The utility function that Envoy implemented to detect deprecated fields based on proto reflection uses the repeated message API to iterate the contents of a map field. The iteration of the map field seems to result in some memory allocations as the repeated field representation of the map is populated. Having a direct const iterator interface to the values of the map would reduce memory usage.

Relevant envoy code link: https://github.com/envoyproxy/envoy/blob/6858df7eef5867b3ae91c4eab6798ccd4490cf11/source/common/protobuf/utility.cc#L215