Skip to content

fix: fix when package equal service name #1501

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Gcaufy
Copy link

@Gcaufy Gcaufy commented Oct 28, 2020

In the proto file, if the service name === package name, then lookupService does not work.

Check here: https://github.com/protobufjs/protobuf.js/blob/master/src/namespace.js#L341-L358

When it found a matched name type, it will stop fetching the nestedArray.

Reproduce Code

// test.proto
package greeter;

syntax = "proto2";

service greeter {
    rpc SayHello (HelloRequest) returns (HelloReply) {}
}
message HelloRequest {
    optional string name = 1;
}
message HelloReply {
    optional string message = 1;
}

// test.js
const protobuf = require('protobufjs');
const root = protobuf.loadSync('./test.proto');
const svr = root.lookupService('greeter'); // This line will throw an Error
Error: no such Service 'greeter' in Root
    at Root.lookupService (/private/tmp/node_modules/protobufjs/src/namespace.js:426:15)
    at Object.<anonymous> (/private/tmp/test-proto-service/index.js:7:18)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:279:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:752:3)

@alexander-fenster
Copy link
Contributor

Hi @Gcaufy,

Would you be willing to add a test for this scenario, and make sure the tests are green?

Thank you!

@alexander-fenster
Copy link
Contributor

@Gcaufy Apparently one of the test fails with this change, the code is here:

test.throws(function() {
ns.lookupType("Enm");
}, Error, "should throw when looking up an enum as a type");

@murgatroid99 and I believe that the failing test might be wrong (it should've found the message Enm). Could you please make the tests pass, and probably add one more test for the case you're fixing?

@Gcaufy
Copy link
Author

Gcaufy commented Dec 13, 2020

@alexander-fenster Tests updated. please take your time have a look.

@Gcaufy
Copy link
Author

Gcaufy commented Dec 18, 2020

@alexander-fenster

Can you have a look the pull request?

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

Successfully merging this pull request may close these issues.

2 participants