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

fix(grpc-reflection): [#2671] handle references to root-level message types in default package #2673

Merged
merged 1 commit into from Feb 23, 2024

Conversation

jtimmons
Copy link
Contributor

Bugfix to handle references to root-level message types in the default (unspecified) package from issue #2671. This was causing warnings about failed lookups to any message type that:

  1. Was in the default package (eg. no package directive in the proto file), and
  2. Was at the top-level of the package, not nested in another message

The root cause of this was a bug in the scope utility where, when we were seeking up the package scope for where the symbol reference lived, we skipped over the root (.) package. Relevant snippet from bug report:

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 change includes:

  1. A fix to the scope utility to prevent it from skipping over the root (.) package
  2. A fix to how we reference message types to avoid cases like . + .TopLevelMessage
  3. A reproduction of the issue from @grpc/reflection writes wanings to console for .proto files with no package declaration #2671 in unscoped.proto as well as a regression test in test-reflection-v1-implementation.ts

Copy link
Member

@murgatroid99 murgatroid99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix.

@murgatroid99 murgatroid99 merged commit 9886ee2 into grpc:master Feb 23, 2024
9 of 10 checks passed
@murgatroid99
Copy link
Member

This has been published in version 1.0.2.

@murgatroid99
Copy link
Member

This change is not entirely correct, because when a file has no package, its entries in the symbol table have a leading ., so removing the period causes the failure to find the symbol. Also, this test doesn't seem to correspond to the warnings that were the originally reported problem in #2671: I can trigger new warnings by adding a service to unscoped.proto, but I can't see how to make the test fail even with that change.

@murgatroid99
Copy link
Member

I think the main issue with the test is that when there are no cross-file references, addReference doesn't actually do anything. You get the warning about a failure to find a symbol, but the actual outcome is no different than if it did find the symbol.

@murgatroid99
Copy link
Member

I think I have a fix. I'll send a PR shortly.

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

Successfully merging this pull request may close these issues.

None yet

3 participants