@grpc/reflection writes wanings to console for .proto files with no package declaration

[paulish]

Problem description

If proto file has not package declaration then call to ReflectionService writes unwanted warnings to console.

Reproduction steps

Use attached .proto file from archive dbdata.zip and the following code

    const loadOptions = {
        keepCase: true
    };
    const packageDefinition = protoLoader.loadSync(PROTO_PATH, loadOptions);
    const dbDataProto = grpc.loadPackageDefinition(packageDefinition);
    const reflection = new ReflectionService(packageDefinition, loadOptions);

Environment

"@grpc/reflection": "^1.0.1"

Additional context

The problem is in the reflection-v1.js addReference function.

            if (ref.startsWith('.')) {
                // absolute reference -- just remove the leading '.' and use the ref directly
                referencedFile = this.symbols[ref.replace(/^\./, '')];
            }

It expects to remove the prefix before the package but regular messages has no package prefix and dot should not be removed for them.

[paulish]

I have not tried your tests but perhaps a possible solution could be to replace that piece of code with

            if (ref.startsWith('.') && ref.includes('.', 1)) {
                // absolute reference -- just remove the leading '.' and use the ref directly
                referencedFile = this.symbols[ref.slice(1)];
            }

I also changed ref.replace(/^\./, '') with ref.slice(1) since removing a first dot with a regular expression looks like overkill :)

[murgatroid99]

I think that looks reasonable, though I'm guessing that ref.includes('.', 1) could just be ref.length > 1, and I think ref.substring(1) would be preferred over ref.slice(1). I would also think that the part where the relevant entry in this.symbols is added might have the same problem, so a similar change would probably be needed there.

@jtimmons You wrote this code, does this change look reasonable to you, or is there something we're missing here?

[jtimmons]

does this change look reasonable to you, or is there something we're missing here?

I think the problem here is actually in the "seek upwards in package scope to find symbol" logic in the second if statement. I managed a pretty minimal reproduction of @paulish's issue by paring his example down to:

syntax = "proto3";
message TopLevelMessage {
  bool value = 1;
}
message ProcessRequest {
  string key = 1;
  TopLevelMessage message = 2;
}

in testing this, we end up with TopLevelMessage being indexed as .TopLevelMessage and the reference to it from ProcessRequest being sourced from the scope .ProcessRequest. When checking the index the search goes like this in order:

1. Check current scope (.ProcessRequest.TopLevelMessage) which does not exist
2. Check parent scope (TopLevelMessage) which does not exist
3. Bail out because we're at the root already

this seems like a bug because I'd expect it to look for .TopLevelMessage (note the leading .) first before trying a global lookup. Probably just a tweak to the scope utility in utils.ts, I'll take a pass at it and try and put up a PR today or tomorrow

[murgatroid99]

A fix for this is out in @grpc/reflection version 1.0.2. Please try it out.

[paulish]

@murgatroid99 tried 1.0.2 and the problem is still exists. At least I still have warnings in the console output.

[murgatroid99]

What warnings are you seeing, exactly?

[paulish]

Could not find file associated with reference .GetActiveReply
Could not find file associated with reference .ProcessRequest
Could not find file associated with reference .ValueReply
Could not find file associated with reference .GetInfoRequest
Could not find file associated with reference .ValueReply
Could not find file associated with reference .Empty
Could not find file associated with reference .Empty
Could not find file associated with reference .Empty
Could not find file associated with reference .ExecuteCommandRequest
Could not find file associated with reference .ValueReply
Could not find file associated with reference .Empty
Could not find file associated with reference .GetActiveReply
Could not find file associated with reference .ProcessRequest
Could not find file associated with reference .ValueReply
Could not find file associated with reference .GetInfoRequest
Could not find file associated with reference .ValueReply
Could not find file associated with reference .Empty
Could not find file associated with reference .ExecuteCommandRequest
Could not find file associated with reference .ValueReply```

[murgatroid99]

I just published version 1.0.3 with more fixes. According to my manual testing with your proto file, that fixes all of the warnings that you were seeing.

[paulish]

Thank you! Now I can confirm that the problem is fixed.