eddelbuettel/rprotobuf

The "fetch" method uses wrong indexing for repeated messages/groups

Closed this issue · 2 comments

I have tested the below example with RProtobuf 0.4.9 on R version 3.4.1 (x86_64-pc-linux-gnu).

The fetch method disregards the provided index for repeated message types:

> p <- new(tutorial.Person, name = "John", id = 1)
> p$add("phone", new(tutorial.Person.PhoneNumber, number = "one"))
> p$add("phone", new(tutorial.Person.PhoneNumber, number = "two"))
> p$add("phone", new(tutorial.Person.PhoneNumber, number = "three"))
> for (i in 1:3) { cat(p$fetch("phone", i)[[1]]$toString()) }
number: "one"
number: "one"
number: "one"    # expected to see: one, two, three

I believe the issue is with the get_field_values code in wrapper_Message.cpp. The code uses raw loop variable i instead of index[i] for repeated messages and groups:

res[i] = S4_Message(CLONE(&ref->GetRepeatedMessage(*message, field_desc, i)));

Hence the fix should be trivial:

res[i] = S4_Message(CLONE(&ref->GetRepeatedMessage(*message, field_desc, index[i])));

Please also note that a reasonable workaround for this issue is to iterate through the repeated messages without using the fetch method:

> p <- new(tutorial.Person, name = "John", id = 1)
> p$add("phone", new(tutorial.Person.PhoneNumber, number = "one"))
> p$add("phone", new(tutorial.Person.PhoneNumber, number = "two"))
> p$add("phone", new(tutorial.Person.PhoneNumber, number = "three"))
> for (telephone in p$phone) { cat(telephone$toString()) }
number: "one"
number: "two"
number: "three"

Good analysis, this looks "likely" also based on the lines above it all using index[i].

Thanks again for the report, and suggested fix. The new version is now on CRAN.

Next time we can do a proper pull request so that you get proper credit (but we'd still prefer an identifiable name and email with that....)