ProtoBuf.protoFromFile has odd path construction for imports

[jandrieu]

ProtoBuf.protoFromFile prepends the initial path onto import paths when fetching imports. Eg.

ProtoBuf.protoFromFile("MyThing.proto")

Appropriately requests "MyThing.proto" from the webserver using fetch. MyThing.proto has several imports, e.g.,

import "MyLib/MyObject/MyDog.proto";

Which, reasonably, trigger fetches of their own to get the proto files. Unfortunately, the path looks like this:

MyThing.proto/MyLib/MyObject/MyDog.proto

This is challenging because on the server, MyThing.proto is a file, which means it can't also be a directory to hold the imported protos.

I can filter this out server side, but that seems like an odd way to solve it. Shouldn't the request path just be the import path, e.g., "MyLib/MyObject/MyDog.proto"? That would be easier to work with just using essentially a static fileserver.

[jandrieu]

Update: this is actually only a problem if the file has no /s in the path, as indicated above. Changing the path to http://example.com/proto/MyThing.proto worked like a charm.

So, this is a partial documentation bug: this could be clearer.

However, you might want to consider supporting relative URLs, so that MyThing.proto would be made absolute before processing if in the browser..

[dcodeIO]

You are right, that import root code is really crappy. I wonder that noone else had problems with this already.

Nevertheless, I rewrote that, notably https://github.com/dcodeIO/ProtoBuf.js/blob/master/ProtoBuf.js#L3203

Let me know if it's working.

[jandrieu]

Very cool. That helps with the relative paths, but it highlights another quandary.

Given our proto hierarchy, we specify multiple include paths when compiling .proto files. This means that almost all of our protos have imports relative to the include folders, not to the proto files.

Consider this tree structure, with imports listed for the relevant protos.

Electronics/Radio.proto
  import Manufacturers/Sony.proto
Hardware/Hammer.proto
  import Manufacturers/Crafstman.proto
Manufacturers/Sony.proto
Manufacturers/Crafstman.proto

Unfortunately, after parsing Radio.proto, the import algorithm will try to fetch
"Electronics/Manufacturers/Sony.proto".

Then after parsing the Hammer.proto, it will try to fetch
"Hardware/Manufacturers/Craftsman.proto"

I believe "right way" to address this would be to configure an ImportRoot that would be used for all imports. We can move all of our protos to a common tree that maps to our namespace.

I'm going to hack a solution using a new ProtoBuf.importRoot to see if we can at least get the code to traverse our proto library. I'm not sure the "right" way to add such a configuration variable, but I can at least prove the concept and show you what I mean.

[dcodeIO]

One option could be, besides specifying the file name to protoFromFile, to also allow to give that method an object like { root: "./", file: "Electronics/Radio.proto" } which will then override the automatically computed root. Wdyt?

[jandrieu]

That sounds like a good API. We'd need to store it and use it for all subsequent parses & imports.

[dcodeIO]

One more thought: A workaround might also be to create a Radio.proto in the root folder, that just includes the main radio file, if I get your directory structure right. This way around the library will get the correct root path.

[jandrieu]

Unfortunately, we have imported protos with their own imports. And those would create an importRoot based on their full filename rather than the desired importRoot.

[dcodeIO]

https://github.com/dcodeIO/ProtoBuf.js/wiki#loading-proto-files

[dcodeIO]

Btw: If you need to mix files using different relative path behaviour, you may do this by using one builder for each subset of files, providing them as the third parameter to ProtoBuf.protoFromFile(filename[, callback], builder) in a proper chain in the right order, to construct a common namespace with all the messages.

[jandrieu]

Ok. The latest fix didn't quite work, but I submitted a pull request that fixed it:

The following changes since commit c7491ff:

fixed importRoot functionality to actually check for importRoot when handling imports of children (when the filename is a file from the import statement and can't be an object specifying the root). (2013-11-21 21:57:26 -0800)

are available in the git repository at:

https://github.com/dcodeIO/ProtoBuf.js

for you to fetch changes up to c7491ff:

fixed importRoot functionality to actually check for importRoot when handling imports of children (when the filename is a file from the import statement and can't be an object specifying the root). (2013-11-21 21:57:26 -0800)

---