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.protoFromFile has odd path construction for imports #70

Closed
jandrieu opened this issue Nov 14, 2013 · 10 comments
Closed

ProtoBuf.protoFromFile has odd path construction for imports #70

jandrieu opened this issue Nov 14, 2013 · 10 comments

Comments

@jandrieu
Copy link

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

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

dcodeIO commented Nov 15, 2013

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

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

dcodeIO commented Nov 15, 2013

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

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

@dcodeIO
Copy link
Member

dcodeIO commented Nov 15, 2013

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

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

dcodeIO commented Nov 16, 2013

@dcodeIO
Copy link
Member

dcodeIO commented Nov 16, 2013

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

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)


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants