Protocol Buffers : Printing embedded messages in unknown messages

Heuristically detect and print embedded messages within messages whose .proto file is unknown.

Patch attached.

The PrintUnknownFields() in this patch attempts to display embedded messages, by default.  In some cases, it wrongly displays fields intended to be regular strings as embedded messages.  Maybe we should have a flag to indicate whether we want to attempt sub-message detection?  Another option is to first print the embedded message value as a regular length-delimited string and then expand it on the next line,  explicitly indicating that the expansion is a guess.

I am not sure which option is better. I can submit an updated patch if we decide on any of the options above.


Dilip

[dilip.antony.joseph, 2008-07-30 20:13:07]

I am not sure if I need to "Publish + Mail Draft Comments" to make the issue
visible to you.  Trying anyway.

[kenton, 2008-07-30 23:36:50]

I would like for this feature to be on by default, since I think it will help in
debugging.  I think the most common false-positive would be zero-length strings,
so let's explicitly detect that and assume the data is *not* a sub-message if it
is zero-length.  Other than that, it should be pretty rare for bytes that were
not supposed to be a message to parse correctly as one.  Or are you seeing a lot
of false positives in practice?

http://codereview.appspot.com/2679/diff/1/4
File src/google/protobuf/text_format.cc (right):

http://codereview.appspot.com/2679/diff/1/4#newcode752
Line 752: /* static */ bool TextFormat::Print(const void* data, int size,
Please include a blank line between method definitions.

http://codereview.appspot.com/2679/diff/1/2
File src/google/protobuf/text_format.h (right):

http://codereview.appspot.com/2679/diff/1/2#newcode53
Line 53: static bool Print(const void* data, int size, io::ZeroCopyOutputStream*
output);
Please keep all code lines to 80 chars or less.

http://codereview.appspot.com/2679/diff/1/2#newcode53
Line 53: static bool Print(const void* data, int size, io::ZeroCopyOutputStream*
output);
Hmm, I don't think it should be TextFormat's responsibility to parse raw data. 
Instead, the input should be an UnknownFieldSet, and we should add a method to
UnknownFieldSet which allows you to parse directly into it.  This would be more
broadly useful -- many people would probably like to be able to parse data
directly into an UnknownFieldSet.

So, can you add a method to UnknownFieldSet called MergeFromCodedInput() which
works like the equivalent method of Message?  That method should just call
WireFormat::SkipMessage(), of course.  If you want, you can also add
UnknownFieldSet::ParseFromString() and/or UnknownFieldSet::ParseFromArray() as
convenience wrappers.

Then, rename *this* method to TextFormat::PrintUnknownFields() and have it take
a const UnknownFieldSet& as input.

http://codereview.appspot.com/2679/diff/1/2#newcode54
Line 54: static bool PrintToString(const void* data, int size, string* output);
Similar comments here -- PrintUnknownFieldsToString(), and have the input be an
UnknownFieldSet.

http://codereview.appspot.com/2679/diff/1/3
File src/google/protobuf/text_format_unittest.cc (right):

http://codereview.appspot.com/2679/diff/1/3#newcode167
Line 167: unsigned char data[] = {  0x0a,0x18,0x0a,0x04,0x41,0x62,0x63,0x64,
It's hard to verify this raw data.  Can you instead use a defined message (like
TestAllTypes from unittest.proto) to generate the data initially, and then print
that back using PrintUnknownFields()?  This way it's very easy to see that the
end result is correct, and the test does not depend on the protobuf wire format.

http://codereview.appspot.com/2679/diff/1/3#newcode175
Line 175: "1:\n"
Sub-messages in the output should be wrapped in braces.

[dilip.antony.joseph, 2008-08-01 05:11:19]

On 2008/07/30 23:36:50, kenton wrote:
> I would like for this feature to be on by default, since I think it will help
in
> debugging.  I think the most common false-positive would be zero-length
strings,
> so let's explicitly detect that and assume the data is *not* a sub-message if
it
> is zero-length.  Other than that, it should be pretty rare for bytes that were
> not supposed to be a message to parse correctly as one.  Or are you seeing a
lot
> of false positives in practice?
> 

I have added a check for 0 length strings.  I haven't seen a lot of false
positives in practice.  But the very first message I randomly tried had a false
positive :-(.

Strings of length 5 or 9 may be confused with embedded 32-bits or 64-bits, if
the first character byte ends with 001 or 101.  That makes at least 64 (2^5 +
2^5) of the 256 possible start characters for length 5 or 9 strings problematic.

I have individually replied to the other comments inline. I have added the more
comprehensible test case messages you suggested.  Waiting for  your feedback on
some issues before proceeding further with some code and submitting a new patch.

[dilip.antony.joseph, 2008-08-01 05:11:43]

replied to your comments inline

http://codereview.appspot.com/2679/diff/1/4
File src/google/protobuf/text_format.cc (right):

http://codereview.appspot.com/2679/diff/1/4#newcode752
Line 752: /* static */ bool TextFormat::Print(const void* data, int size,
On 2008/07/30 23:36:50, kenton wrote:
> Please include a blank line between method definitions.

Will do that.

http://codereview.appspot.com/2679/diff/1/2
File src/google/protobuf/text_format.h (right):

http://codereview.appspot.com/2679/diff/1/2#newcode53
Line 53: static bool Print(const void* data, int size, io::ZeroCopyOutputStream*
output);
On 2008/07/30 23:36:50, kenton wrote:
> Please keep all code lines to 80 chars or less.

Sorry, I missed this line.  Will double check next time.

http://codereview.appspot.com/2679/diff/1/2#newcode53
Line 53: static bool Print(const void* data, int size, io::ZeroCopyOutputStream*
output);
On 2008/07/30 23:36:50, kenton wrote:
> Hmm, I don't think it should be TextFormat's responsibility to parse raw data.

> Instead, the input should be an UnknownFieldSet, and we should add a method to
> UnknownFieldSet which allows you to parse directly into it.  This would be
more
> broadly useful -- many people would probably like to be able to parse data
> directly into an UnknownFieldSet.
> 
> So, can you add a method to UnknownFieldSet called MergeFromCodedInput() which
> works like the equivalent method of Message?  That method should just call
> WireFormat::SkipMessage(), of course.  If you want, you can also add
> UnknownFieldSet::ParseFromString() and/or UnknownFieldSet::ParseFromArray() as
> convenience wrappers.
> 
> Then, rename *this* method to TextFormat::PrintUnknownFields() and have it
take
> a const UnknownFieldSet& as input.


I agree that TextFormat should not be responsible for parsing raw data. I will
add the UnknownFieldSet::MergeFromCodedInput() and convenience wrappers you
suggested.  

Should this MergeFromCodedInput() recursively attempt to parse the
length-delimited fields using our heuristic?  I think it should.  Then, we will
need a RepeatedPtrField<UnknownFieldSet>* embedded_msg_ for embedded messages in
class UnknownField.  There is already a RepeatedPtr<UnknownFieldSet>* group_. 
But it may not be a good idea to mix well-defined groups and heuristically
identified embedded messages.  Do you think this makes sense?  Wanted to get
your feedback before writing the code.

There was already a private static TextFormat::PrintUnknownFields(const
UnknownFieldSet& unknown_fields, generator).  I modified this function to
heuristically parse and print embedded messages.  We should probably remove
message parsing from this function as well.  If we heuristically detect embedded
messages in UnknownFieldSet::MergeFromCodedInput itself, as desribed above, we
can easily avoid parsing -- just need to recursively call PrintUnknownFields. 
However, this won't work with UknownFieldSets created by regular Message
parsing.  Is that a problem?

http://codereview.appspot.com/2679/diff/1/2#newcode54
Line 54: static bool PrintToString(const void* data, int size, string* output);
On 2008/07/30 23:36:50, kenton wrote:
> Similar comments here -- PrintUnknownFieldsToString(), and have the input be
an
> UnknownFieldSet.

Will do that.

http://codereview.appspot.com/2679/diff/1/3
File src/google/protobuf/text_format_unittest.cc (right):

http://codereview.appspot.com/2679/diff/1/3#newcode167
Line 167: unsigned char data[] = {  0x0a,0x18,0x0a,0x04,0x41,0x62,0x63,0x64,
On 2008/07/30 23:36:50, kenton wrote:
> It's hard to verify this raw data.  Can you instead use a defined message
(like
> TestAllTypes from unittest.proto) to generate the data initially, and then
print
> that back using PrintUnknownFields()?  This way it's very easy to see that the
> end result is correct, and the test does not depend on the protobuf wire
format.

Will do that

http://codereview.appspot.com/2679/diff/1/3#newcode175
Line 175: "1:\n"
On 2008/07/30 23:36:50, kenton wrote:
> Sub-messages in the output should be wrapped in braces.

Oops.  Will do that.

[dilip.antony.joseph, 2008-08-04 23:20:47]

Heuristically print messages with unknown .proto (revised)