support of the protobuf protoset binary format?

[c0b]

from konsumer/grpc-dynamic-gateway#15
and https://groups.google.com/d/msg/grpc-io/dAJz2sPwir8/TlgLny-PBgAJ nobody answered the protoset questions in this discussion thread ,

Is your feature request related to a problem? Please describe.

google searched grpc + nodejs + protoset seems nothing, and searched this issues board as well,
no one has discussed about protoset support before;

I see the compiled protoset binary format is a good solution to bundle many related *.proto files into a single one, it's pretty well supported by protoc compiler, and in Go language pkg github.com/golang/protobuf, and tools like grpcurl
https://github.com/fullstorydev/grpcurl#protoset-files

I hope this base library should include protoset support, for tools like the grpc-dynamic-gateway to be able to load from a single protoset binary file

Describe the solution you'd like

the golang/protobuf package has good support of protoset support, I think this grpc-node should have similar support, to parse one binary protoset file only, instead of multiple plaintext *.proto files
https://github.com/golang/protobuf/tree/master/protoc-gen-go/descriptor

Describe alternatives you've considered

No one discussed grpc + nodejs + protoset before.

Additional

make better documentation about how to use this grpc.load method: does it already support parameter as { file: p, root: include } ?? does it already support multiple proto_path ? does it already support the protoset binary proto definition?

https://grpc.io/grpc/node/grpc.html#.load__anchor

<static> load
Load a gRPC object from a .proto file.

$ protoc --help
Usage: protoc [OPTION] PROTO_FILES
Parse PROTO_FILES and generate output based on the options given:
  -IPATH, --proto_path=PATH   Specify the directory in which to search for
                              imports.  May be specified multiple times;
                              directories will be searched in order.  If not
                              given, the current working directory is used.

  -oFILE,                     Writes a FileDescriptorSet (a protocol buffer,
    --descriptor_set_out=FILE defined in descriptor.proto) containing all of
                              the input files to FILE.

[nicolasnoble]

Sorry, but what you're describing isn't a feature request for grpc-node, but for the third party package protobufjs.

There's two ways of using grpc-node. One is the static codepath, which is using protoc at build time to generate service files, and one is the dynamic codepath, which is using the third-party protobufjs package under the hood, where you load your proto files at runtime.

The static codepath is using protoc, and will therefore support protoset naturally.

Please either file a feature request with protobufjs here: https://github.com/dcodeIO/protobuf.js, or switch to using protoc.

[nicolasnoble]

Also, here's a link to a quick example using the static codepath: https://github.com/grpc/grpc/tree/master/examples/node/static_codegen

[c0b]

About this part below, can I file a request better Documentation ticket?

---

make better documentation about how to use this grpc.load method: does it already support parameter as { file: p, root: include } ?? does it already support multiple proto_path ? does it already support the protoset binary proto definition?

https://grpc.io/grpc/node/grpc.html#.load__anchor

[c0b]

/cc @konsumer as well...

[c0b]

Please either file a feature request with protobufjs here: https://github.com/dcodeIO/protobuf.js, or switch to using protoc.

then should have a better linking to protobuf.js ? I was reading following https://github.com/grpc/grpc/blob/master/examples/node/dynamic_codegen/greeter_client.js#L21-L22 the lines

var grpc = require('grpc');
var protoLoader = require('@grpc/proto-loader');

lead me to its npmjs package description,

• https://www.npmjs.com/package/grpc
• https://www.npmjs.com/package/@grpc/proto-loader

both have same homepage => grpc.io repository => github ==> https://github.com/grpc/grpc-node (this repo)

[nicolasnoble]

This function is flagged as deprecated. Please use the proto loader package instead, with its documentation available in it: https://www.npmjs.com/package/@grpc/proto-loader

[nicolasnoble]

That's fair. The documentation already mentions protobufjs, but we could link to it as well.

[c0b]

This function is flagged as deprecated.

could the Deprecation being documented? https://grpc.io/grpc/node/grpc.html#.load__anchor

[nicolasnoble]

I'm surprised it's not in there. We'll fix this.

[nicolasnoble]

@c0b your documentation concerns should have been addressed in #558. Please take a look.

[c0b]

I see #558 has been merged but I wonder will @deprecated tag show som deprecation warning on its doc page? or takes some time to reflect there?

https://grpc.io/grpc/node/grpc.html#.load__anchor

[nicolasnoble]

The website will be updated for the next minor release.

[c0b]

No, I checked https://grpc.io/grpc/node/grpc.html#.load__anchor ; for 2 months passed it did not happen

Does it mean nobody care the documentation at https://grpc.io/grpc/node/ ? or too few users using grpc with node ?

[c0b]

Documentation generated by JSDoc 3.5.5 on 2018-08-10T10:27:03-07:00 using the DocStrap template.

I got the answer from the site footnotes, it's not updated since August? Would you set up some sort of automated Documentation re-generation? otherwise the online documentation is not useful

[murgatroid99]

The documentation has now been updated for 1.16, including that deprecation notice

[c0b]

still questions: is this <static> grpc.loadObject(value [, options]) usable? or also deprecated?

https://grpc.io/grpc/node/grpc.html#.loadObject__anchor

Name	Type	Argument	Description
value	Object		The ProtoBuf.js reflection object to load

it says the value Object is The ProtoBuf.js reflection object but what is that? do you have some examples how to call this grpc.loadObject ?

the problem still comes from the protoset format support,
from protobufjs/protobuf.js#1117 (comment) I am using the require("protobufjs/ext/descriptor") to load chained *.proto files from one single protoset format binary file instead of multiple chained *.proto plain text files, it returns a single Protobuf.js Root object, but the problem is unable to use the @grpc/proto-loader, reason is it loads plain text files only, in #550 you guys are not willing to export the createPackageDefinition which can work with the already parsed Protobuf.js Root object,

so my solution is using a copy and modified version of the @grpc/proto-loader (to export the createPackageDefinition only)

but your Documentation looks like the grpc.loadObject is able to work with Protobuf.js objects,

https://grpc.io/grpc/node/grpc.html#.loadObject__anchor

but I have tried, it isn't working

so, can only guess it might also be deprecated? if you have gave up your own grpc.load and rely on third party's (the github.com/dcodeIO/Protobuf.js ) parsing; then where do you use your own loaded/parsed types? like grpc.MethodDefinition grpc.PackageDefinition ... ?

1. https://grpc.io/grpc/node/grpc.html#~MethodDefinition__anchor
2. https://grpc.io/grpc/node/grpc.html#~PackageDefinition__anchor
3. ...

[murgatroid99]

grpc.loadObject is not deprecated precisely because it is currently the only viable solution to some problems, like the one you describe. The "ProtoBuf.js reflection object" that documentation refers to is the type with that name from ProtoBuf.js itself. That includes Root, Service, and Message objects (and some others), so you should be able to use that function by passing in the Root object you have. If you provide more details about how this is currently failing, I may be able to help.

One other thing I would like to mention is that it is not really accurate to say that we replaced grpc.load with Protobuf.js. grpc.load also uses Protobuf.js, but it uses an older version in a way that can't be easily upgraded, and it has some other issues that meant that it needed to be replaced.

[c0b]

ok, code is like this below: the Root object is loaded from protobufjs/ext/descriptor from a binary protoset format, because of multiple plain text *.proto files inconvenience;
a modified @grpc/proto-loader (the ./loader.js because I need to export the createPackageDefinition) version can happily load it without problems,

the intention is to not use a modified @grpc/proto-loader; so testing if this grpc.loadObject can work with the pbroot here,

const grpc = require('grpc');
const protobuf   = require("protobufjs"), // requires the full library
      descriptor = require("protobufjs/ext/descriptor");

const pbroot = protobuf.Root.fromDescriptor(
  fs.readFileSync(path.join(__dirname, 'api-protos.bin'))
).resolveAll();

const pkgd = require('./loader.js').createPackageDefinition(
  pbroot,
  { keepCase: true, longs: String, enums: String, defaults: true, oneofs: true, });

const api = grpc.loadPackageDefinition(pkgd);
const api2 = grpc.loadObject(pbroot, { protobufjsVersion: 6 });

  const client = new api.api.Api(
    'grpc-server-address:8443',
    grpc.credentials.createSsl( ...
    ),
  );

I am expecting the grpc.loadObject can load from a Root object without the need to createPackageDefinition,
but it doesn't, if I don't pass in any extra options, I found the guessing logic think it's protobufjsVersion 6 and failed with this error:

/home/drc/tmp/node_modules/grpc/src/protobuf_js_6_common.js:140
    Object.keys(value.nested).forEach(name => {
           ^

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at loadObject (/home/drc/tmp/node_modules/grpc/src/protobuf_js_6_common.js:140:12)
    at Object.keys.forEach.name (/home/drc/tmp/node_modules/grpc/src/protobuf_js_6_common.js:142:22)
    at Array.forEach (<anonymous>)
    at loadObject (/home/drc/tmp/node_modules/grpc/src/protobuf_js_6_common.js:140:31)
    at Object.keys.forEach.name (/home/drc/tmp/node_modules/grpc/src/protobuf_js_6_common.js:142:22)
    at Array.forEach (<anonymous>)
    at loadObject (/home/drc/tmp/node_modules/grpc/src/protobuf_js_6_common.js:140:31)
    at Object.keys.forEach.name (/home/drc/tmp/node_modules/grpc/src/protobuf_js_6_common.js:142:22)
    at Array.forEach (<anonymous>)
Command exited with non-zero status 1

if I pass in by { protobufjsVersion: 5 } then it doesn't fail on the grpc.loadObject line but fail on the following instantiation time new api2.api.Api( ... ) saying no api on the returned api2 object,

then I compared what's the returned api2 object from grpc.loadObject it seems returned a same object as pbroot,

So the question is Could you document the grpc.loadObject better on how does it work? Show by a piece of example code?

https://grpc.io/grpc/node/grpc.html#.loadObject__anchor

[c0b]

Another thing:

the grpc.loadPackageDefinition should be a static method but not documented at all? https://grpc.io/grpc/node/grpc.html

[murgatroid99]

OK, as far as I know, the code you wrote there should work. If you have [email protected] installed, please switch to 1.16.1 for now. We are aware of bugs in that specific code path in 1.17.0-pre1. Based on your specific error, I think that will solve your problem.

Regarding grpc.loadPackageDefinition, thank you for pointing that out. It looks like there are some errors in the JSDoc for it so the generated documentation actually ended up here.

[c0b]

No; I've installed grpc@^1.16.1 and it's still failing for same error,

would you give any working example in the documentation?
https://grpc.io/grpc/node/grpc.html#.loadObject__anchor

is that what you were saying grpc.loadObject(pbroot, options) equivalent to .createPackageDefinition and grpc.loadPackageDefinition ??

const pkgd = require('@grpc/proto-loader').createPackageDefinition(pbroot, options);
const api = grpc.loadPackageDefinition( pkgd );

then a question followed is the supporting options are not exactly same:
as I have given an example of createPackageDefinition's options can be { keepCase: true, longs: String, enums: String, defaults: true, oneofs: true, }
but from above link documentation the grpc.loadObject options are { binaryAsBase64: ... , longsAsStrings: ..., protobufjsVersion: ... } does it support if I supply more options accepted by createPackageDefinition ?

[murgatroid99]

I sincerely doubt that your code is failing with the exact same error in version 1.16, because the referenced line of code did not exist in that form in 1.16. Can you please check what version you actually have installed?

I didn't say anything about createPackageDefinition. It is an internal API, and it is not intended to be used by other packages. In addition, there was never any intention for its API to be the same as loadObject's API. The loadObject function is opinionated and chooses its own settings for most options. Its own options allow some of those choices to be modified in specific ways. The proto-loader library does not do that, and it directly exposes the same options that Protobuf.js itself has. All that being said, createPackageDefinition and loadPackageDefinition together should be functionally very similar to loadObject.