alavrik / piqi

Piqi – universal schema language: JSON, XML, Protocol Buffers data validation and conversion
http://piqi.org
Apache License 2.0
246 stars 36 forks source link

Relative vs. Global module imports (... or to-proto(of-proto(<proto file>)) fails) #43

Closed joell closed 10 years ago

joell commented 10 years ago

There appears to be an issue with piqi deciding between treating a module import as relative to the directory the current .piqi file is in vs. a global import. It is possible to write a correct set of .proto files that will compile to .piqi files via of-proto without issue but will then fail to compile back to .proto files with to-proto (or will fail when another piqi command like convert attempts to load them).

To reproduce this issue, consider the following three files:

A.proto

import "Apkg/B.proto";

message A {
    required B foo = 1;
}

Apkg/B.proto

import "C.proto";

message B {
    optional C cool_thing = 1;
}

C.proto

message C {
    required int32 counter = 1;
}

Notice that the B message is defined in a .proto file inside a subdirectory named Apkg. I was able to compile these .proto files with protoc and use them in code, so I'm not aware of anything that would make them invalid Protocol Buffer code.

Now, when you convert these .proto files to .piqi files, everything goes smoothly:

$ piqi of-proto -I . A.proto
$ piqi of-proto -I . Apkg/B.proto
$ piqi of-proto -I . C.proto

No errors. Good so far. But when you try to convert them from .piqi files back to .proto files (say in a new sub directory called new), you run into an error:

$ piqi to-proto --debug 1 -I . -o new/A.proto A.proto.piqi
loading piqi file: A.proto.piqi
    piqi_db: caching piqi module "A"
    processing module A
        import: Apkg/B
        trying to locate module directory at "./Apkg"
        trying to locate module at "./Apkg/B.piqi"
        trying to locate module at "./Apkg/B.proto.piqi"
        file: ./Apkg/B.proto.piqi
            piqi_db: caching piqi module "Apkg/B"
            processing module Apkg/B
Warning: internal error:
Piqloc.addref: 70159063683368 at 70159063683632 -- NOT FOUND
--
--
                import: Apkg/C
                trying to locate module directory at "./Apkg"
                trying to locate module at "./Apkg/C.piqi"
                trying to locate module at "./Apkg/C.proto.piqi"
                trying to locate module directory at "./Apkg"
                trying to locate module at "./Apkg/C.piqi"
                trying to locate module at "./Apkg/C.proto.piqi"
                piqi module is not found in path: "Apkg/C"
unknown:0:0: piqi module is not found: "Apkg/C"

My interpretation of these logs is that once piqi loads the Apkg/B.proto.piqi file, it presumes that all imports will be relative to the Apkg subdirectory ... which contradicts with Protocol Buffer's view that all module imports remain global.

This example is the simplest one I could generate for a much more complex production Protocol Buffer module set that I am working with. My real issue is that I cannot use the .piqi files generated from the Protocol Buffer files with piqi convert because of this module import problem. But I'm pretty sure that if we can solve this simple version of the problem my larger case will also be solved.

alavrik commented 10 years ago

Thank you for the detailed write up. The actual problem is in piqi of-proto that doesn't account for the fact that .proto and .piqi module systems are different.

In short, Piqi recognizes two types of module names -- local and scoped and has stricter rules of interpreting them. For instance, an imported local module (i.e. the one without a / in its name) is expected to be local relative to the module that imports it. A scoped module name, on the other hand, is always expected to be a path suffix that forms a full file path by appending it to one of the search paths (specified via -I).

And protoc uses a mechanism similar to C++ namespaces for locating modules.

So, to fix that, during .proto -> .piqi conversion, we'd need to properly handle this and form Piqi module names based on a) the actual location of the .proto file and b) the provided list of search paths. Unfortunately, protoc doesn't help us here, because it doesn't give us a resolved file name. This means we have to follow protoc's logic for .proto module lookup and figure it out ourselves.

Anyway, I'll think of it a bit more and try to fix it in the next couple of days.

In the meantime, I fixed the internal error that gives unkown:0:0 instead of a proper source code location.

alavrik commented 10 years ago

Should be fixed now -- see the linked commit for details. The actual solution turned out to be different, but it makes a lot more sense this way. Basically, the new lookup scheme for imported modules matches the one used by Protocol buffers. This, in turn, makes Piqi imports fully compatible with Protobuf imports. This way, no extra transformation of imported module names is needed when converting proto -> piqi and the other way around.

Can you test the latest version and let me know if it works on your full set of Protobuf modules?

joell commented 10 years ago

I've checked the code with the fix against my full specs, and the fix works flawlessly. I no longer have module import problems. Thanks, Anton! :-)