Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
knoguchi opened this issue Apr 4, 2017 · 3 comments
Closed

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

knoguchi opened this issue Apr 4, 2017 · 3 comments
Milestone

Comments

@knoguchi
Copy link
Contributor

knoguchi commented Apr 4, 2017

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
Copy link
Member

cowtowncoder commented Apr 4, 2017

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
Copy link
Contributor Author

knoguchi commented Apr 15, 2017

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.

@knoguchi knoguchi changed the title [protobuf] . parser fails with /* comment */ [protobuf] . parser fails with /* comment */ [v2.8] Apr 15, 2017
@cowtowncoder
Copy link
Member

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

@cowtowncoder cowtowncoder added this to the 2.8.9 milestone Apr 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants