Yarp: Serializing maps with thrift idl does not work properly

[diegoferigo]

Describe the bug

Let's start with the following thrift:

struct SmallContainer {
    1: string name
}

struct BigContainer {
    1: SmallContainer c1,
    2: SmallContainer c2
}

struct test {
    1: map <string, BigContainer> myMap
}

The autogenerated class has problems in serializing the map, obtaining a serialization that screws up parentheses.

With the help of @drdanz we figured out the problem is due to the nesting of the value of the map (BigContainer is a struct containing two structs).

Possible solution

Digging in the code, here is what I found. Maps are handled by:

https://github.com/robotology/yarp/blob/716b87f9975b85bf459b325e956fb86a9b38ce7a/src/idls/thrift/src/t_yarp_generator.cc#L2565-L2577

And the relevant part of the generate_serialize_field is line 2445 of

https://github.com/robotology/yarp/blob/716b87f9975b85bf459b325e956fb86a9b38ce7a/src/idls/thrift/src/t_yarp_generator.cc#L2429-L2451

In this case generate_serialize_struct accepts a force_nesting flag which is not set for maps. A possible solution would be transforming

https://github.com/robotology/yarp/blob/716b87f9975b85bf459b325e956fb86a9b38ce7a/src/idls/thrift/src/t_yarp_generator.cc#L2574

to

generate_serialize_field(out, &vfield, "", "", true);

This seems working also with non-nested structs, but a more thorough testing would be needed. I'm not an expert on this, and I don't know if this would affect other use cases.

More details

The autogenerated code is the following:

bool test::write_myMap(const yarp::os::idl::WireWriter& writer) const {
  {
    if (!writer.writeMapBegin(BOTTLE_TAG_STRING, BOTTLE_TAG_LIST, static_cast<uint32_t>(test.size()))) return false;
    std::map<std::string, BigContainer> ::const_iterator _iter322;
    for (_iter322 = test.begin(); _iter322 != test.end(); ++_iter322)
    {
      if (!writer.writeListBegin(0,2)) return false;
      if (!writer.writeString(_iter322->first)) return false;
      if (!writer.write(_iter322->second)) return false; // Here is the error
      if (!writer.writeListEnd()) return false;
    }
    if (!writer.writeMapEnd()) return false;
  }
  return true;
}

The line that writes the map's value should be:

if (!writer.writeNested(_iter322->second)) return false;

Configuration

• OS: Ubuntu 18.04
• yarp version: latest devel @ 716b87f9975b85bf459b325e956fb86a9b38ce7a
  
  
  
  • compiler: clang 5
  
  

[drdanz]

Thanks for the very detailed analysis.
I'm not sure if forcing the nested is always, the right thing to do, when second is not a struct, perhaps it should be serialized normally... We can probably check it whith one of these methods:

https://github.com/robotology/yarp/blob/716b87f9975b85bf459b325e956fb86a9b38ce7a/extern/thrift/thrift/compiler/cpp/src/thrift/parse/t_type.h#L47-L60

Have you checked if by applying your fix, the deserialization part is correct?

[Nicogene]

The thrift of yarp has another problem related to the pointclouds.

When it is sent as Bottle (after the call of toBottle), on the receiver side it comes as a Bottle of 1 element that it is a list containing the N points.

Without thrift the Bottle received contains N elements as expected(it is tested in yarp unit tests)
@fbottarel has better insight in this issue.

Probably we should open another issue, but I think it is another problem in the de-serialization

[drdanz]

I'm not sure if forcing the nested is always, the right thing to do, when second is not a struct, perhaps it should be serialized normally

Actually, the write nested might be already doing this, we should check

[diegoferigo]

@drdanz I dug a bit more on this problem which is blocking for me at the current moment. Lists and maps are fairly similar, but if I understood correctly lists always set force_nested=true both for serializing and deserializing. For this reason they always work.

https://github.com/robotology/yarp/blob/716b87f9975b85bf459b325e956fb86a9b38ce7a/src/idls/thrift/src/t_yarp_generator.cc#L2586-L2591

https://github.com/robotology/yarp/blob/716b87f9975b85bf459b325e956fb86a9b38ce7a/src/idls/thrift/src/t_yarp_generator.cc#L2819-L2834

So here what I see is:

1. Align maps to lists and always enable the nesting flag
2. Improve the code and check if the data type to serialize is not a base type and enable the flag if it is

I don't have the bandwidth to fill a PR complete with unit tests but I can submit a hot fix.

Have you checked if by applying your fix, the deserialization part is correct?

From preliminary tests, editing in the same way the deserializing methods, yes.

[drdanz]

if I understood correctly lists always set force_nested=true both for serializing and deserializing

In this case I'm ok with forcing it for maps as well...

[francesco-romano]

Just to reference the old issue (on which I did not provide any information): https://github.com/robotology/yarp/issues/1143

[drdanz]

I closed #1143 in favour of this issue that contains a lot of extra information. Thanks for pointing that out.