FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
316 stars 136 forks source link

[protobuf] . parser fails with /* comment */ [v2.8] #72

Closed knoguchi closed 7 years ago

knoguchi commented 7 years ago

The Proto parser fails to parse / / comment. I modified the SchemaParsingTest to include the comment like this

    final protected static String PROTOC_STRINGS_PACKED =
            "message Strings {\n"
            +" repeated string values = 2 [packed=true]; /* comment */\n"
            +"}\n"
    ;

Then run the test

Tests run: 6, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.007 sec <<< FAILURE! - in com.fasterxml.jackson.dataformat.protobuf.SchemaParsingTest
testPacked(com.fasterxml.jackson.dataformat.protobuf.SchemaParsingTest)  Time elapsed: 0.006 sec  <<< ERROR!
java.lang.IllegalStateException: Syntax error in Unnamed-protobuf-schema at 2:45: expected '/'
    at com.squareup.protoparser.ProtoParser.unexpected(ProtoParser.java:903)
    at com.squareup.protoparser.ProtoParser.tryAppendTrailingDocumentation(ProtoParser.java:845)
    at com.squareup.protoparser.ProtoParser.readField(ProtoParser.java:347)
    at com.squareup.protoparser.ProtoParser.readDeclaration(ProtoParser.java:165)
    at com.squareup.protoparser.ProtoParser.readMessage(ProtoParser.java:218)
    at com.squareup.protoparser.ProtoParser.readDeclaration(ProtoParser.java:152)
    at com.squareup.protoparser.ProtoParser.readProtoFile(ProtoParser.java:92)
    at com.squareup.protoparser.ProtoParser.parse(ProtoParser.java:61)
    at com.fasterxml.jackson.dataformat.protobuf.schema.ProtobufSchemaLoader._loadNative(ProtobufSchemaLoader.java:157)
    at com.fasterxml.jackson.dataformat.protobuf.schema.ProtobufSchemaLoader.parseNative(ProtobufSchemaLoader.java:131)
    at com.fasterxml.jackson.dataformat.protobuf.schema.ProtobufSchemaLoader.parse(ProtobufSchemaLoader.java:105)
    at com.fasterxml.jackson.dataformat.protobuf.SchemaParsingTest.testPacked(SchemaParsingTest.java:118)

I know the problem is ProtoParser, and it's deprecated. Are you going to migrate it to Wire Protocol Buffers ? As far as I see the WPB SyntaxReader class knows the comment syntax.

cowtowncoder commented 7 years ago

That is: while it would be nice to upgrade, no one has had need or time to do that, so there is no active plan. But if anyone has time & itch, I would help with PR.

knoguchi commented 7 years ago

I just noticed ProtoParser supports the trailing / comment /. There is a test case.

The fix was made in the v4.0.3 (latest version)

Version 4.0.3 (2015-06-27)

  • Fix: Support trailing star-style comments (/* hi */) on enum values and fields.

I noticed that you've upgraded in the master branch on the March 31st. So the problem was already fixed when I reported. I was testing 2.8 branch. We can close this issue.

cowtowncoder commented 7 years ago

@knoguchi Ah. Thank you for verifying. Given this, I will backport version upgrade and add unit test. Thanks!