Parser: cannot import more than one .proto file from the same package

[GoogleCodeExporter]

If a .proto file imports several other .proto files that are defined in the 
same package, the parser only remembers the messages defined in the last .proto 
file. I would expect to be able to reference messages defined in any of the 
imported .proto files; this is how things work using Google's protoc compiler.

Compiling p1.proto from the attached .zip file will illustrate the problem.

Original issue reported on code.google.com by avas...@gmail.com on 4 Nov 2010 at 11:12

Attachments:

• test_proto.zip

[GoogleCodeExporter]

Original comment by david.yu...@gmail.com on 7 Nov 2010 at 10:09

• Changed state: Accepted

[GoogleCodeExporter]

I've attached a patch which would solve the problem.
Although I'm still not sure how to handle messages that exist on both protos. 
(message's name  clash ... w/c is usually the mistake of the developer)
I'm leaning on throwing an exception if there is a clash ... sound good?

Original comment by david.yu...@gmail.com on 8 Nov 2010 at 8:30

Attachments:

• proto-import.patch

[GoogleCodeExporter]

Thanks for the quick response! 

I agree that throwing an exception is the right answer if there is a message 
name clash between two different .proto files. Google's compiler also gives an 
error in that situation.

One potential issue with your patch is that it throws away the association 
between a message and which .proto file the message was defined in. This would 
be a problem if, for example, I am writing an rpc/services code generator and 
within this code generator I want to make use of schemas generated by 
java_v2protoc_schema. Since I don't know which .proto file a message was 
defined in (because all of the .proto files got merged into one Proto class), I 
can't determine what java outer classname to use to access the schema.

I also discovered a few more issues with message name resolution. Specifically, 
this algorithm of package name searching is currently not being used:

"Type name resolution in the protocol buffer language works like C++: first the 
innermost scope is searched, then the next-innermost, and so on, with each 
package considered to be "inner" to its parent package. A leading '.' (for 
example, .foo.bar.Baz) means to start from the outermost scope instead." (from: 
http://code.google.com/apis/protocolbuffers/docs/proto.html#packages)

As an example: say I have a message 'M1' defined in package 'p1.test.inner'. 
protostuff doesn't let me refer to that message as 'test.inner.M1' from within 
a message defined in the 'p1' package, even though the protobuf spec allows for 
this and Google's compiler happily accepts it.

I took a stab at fixing these issues (along with the initial issue), and have 
attached a potential patch. Any thoughts? It works by building a hashtable of 
the fully-qualified name of every message and enum group defined in a .proto 
file, and then uses the above package-searching algorithm to try to resolve the 
name. I think it simplifies the reference resolution code a bit too, which is a 
nice side-effect. 

One note about this patch: the test_rpc.proto and test_deep_reference.proto 
test files do not compile with this patch. But actually, they are not valid 
.proto files and Google's compiler does not accept them either. This is because 
they reference messages (Foo and Baz) defined in a different non-parent package 
without qualifying the name.

Original comment by avas...@gmail.com on 9 Nov 2010 at 12:37

Attachments:

• proto_package_resolution.patch

[GoogleCodeExporter]

Thanks for the patch.  I'm gonna test it out.
I agree that the protostuff compiler isn't 100% compatible with google's 
compiler.
The lookup algorithm is similar to java class lookup.
I was also adding java features on top of the compiler. (See 
http://code.google.com/p/protostuff/issues/detail?id=21&can=1)
With that, the message structure allows for deep nested messages.
I'm not sure but does the protobuf spec allow nested messages 2+ levels deep?
E.g a nested message cannot have a nested message?

I'll try to apply this patch and still allow test_rpc.proto and 
test_deep_reference.proto to compile. (being google protobuf compatible and yet 
still have the java features intact)
If you can modify your patch for this, would be awesome :-)

Original comment by david.yu...@gmail.com on 9 Nov 2010 at 5:10

[GoogleCodeExporter]

Ah, gotcha! I didn't realize that you had implemented an add-on feature to 
support the auto-importing of other packages' messages. Fair enough. I've added 
a few lines to my patch and now if it fails to resolve a message/enum using the 
standard protobuf rules, it will look for messages of the same name in each of 
the imported .proto files. So now test_rpc.proto and test_deep_reference.proto 
compile, and my standard google protobuf messages do too.

Also, yeah, the protobuf spec allows messages to be nested as deep as you want 
them to be; there's definitely no arbitrary limit as far as that goes.

My updated patch is attached.

Original comment by avas...@gmail.com on 9 Nov 2010 at 6:24

Attachments:

• proto_package_resolution_v2.patch

[GoogleCodeExporter]

Looks good.
A few remarks:
- In Extension.java, the resolving of fullRefName is currently inversed.
  String fullRefName = (this.packageName == null ? this.type : this.type + '.' + this.packageName);

- formatting ... To apply this patch correctly, can you use spaces(4) instead 
of tabs?

- naming convention ... In Message.java, can you rename the newly added method 
getFullJavaName() to getJavaFullName() (resembles getPackageJavaName() to 
getJavaPackageName())

All in all.  I think the patch is solid.  Thanks!

Original comment by david.yu...@gmail.com on 9 Nov 2010 at 9:12

[GoogleCodeExporter]

Nice catch in Extension.java. Of course right below that function there is 
getExtendedMessageFullName, which does exactly what I need -- I don't know why 
I didn't use that in the first place. :-)

Updated patch is attached.

Original comment by avas...@gmail.com on 9 Nov 2010 at 4:29

Attachments:

• proto_package_resolution_v3.patch

[GoogleCodeExporter]

A few additional changes: the gwt_overlay compiler needs to use the new 
getJavaFullName function instead of getFullName.

Original comment by avas...@gmail.com on 9 Nov 2010 at 8:39

Attachments:

• proto_package_resolution_v4.patch

[GoogleCodeExporter]

Right.  I forgot one thing.  The reason for the on-the-fly computation 
(no-caching) was because of the compiler option that allows packages to be 
renamed at compile time.

This is especially useful when generating gwt overlays where you move the 
package to ${package}.client

That functionality breaks with your patch.
Do you have a test-case for the patch?  (Something similar to the structure of 
your project's proto ... better if with the junit test-case)
My goal is to retain current features and make the test pass for your standard 
google protobuf messages.

Thanks for your work.  I'll incorporate portions of the patches.

Original comment by david.yu...@gmail.com on 10 Nov 2010 at 2:26

[GoogleCodeExporter]

Hmm... I'm not sure that I follow. How does the patch break that functionality? 
I have been using my patched version of the compiler along with the java 
package renaming feature with no problems whatsoever. I've been doing it for 
the exact reason you describe -- generating GWT overlays in the GWT client 
package, while generating the server-side classes in a separate package. It's 
been working great. Am I misunderstanding something?

I'll try and get you some test cases soon.

Original comment by avas...@gmail.com on 10 Nov 2010 at 4:21

[GoogleCodeExporter]

You're right!  I thought the caching would break that functionality but was 
wrong.
As long as the fullName/etc is dynamic, it's all good.

TIA for the test-cases.

Original comment by david.yu...@gmail.com on 10 Nov 2010 at 7:09

[GoogleCodeExporter]

I've committed your patch and added some few changes after the patch (attached) 
to allow getting imported protos by file or url.

Sorry it took too long.  (Hopefully won't happen again ... was busy on 
something else)
Thanks for your contribution.

P.S Your custom code generators are welcome here if you want to contribute them.

Original comment by david.yu...@gmail.com on 30 Dec 2010 at 10:14

• Changed state: Fixed

Attachments:

• load_from_classpath.patch