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

datahub-protobuf doesn't parse message comment correctly #7114

Closed
tinolyu opened this issue Jan 23, 2023 · 7 comments
Closed

datahub-protobuf doesn't parse message comment correctly #7114

tinolyu opened this issue Jan 23, 2023 · 7 comments
Assignees
Labels
bug Bug report stale

Comments

@tinolyu
Copy link
Contributor

tinolyu commented Jan 23, 2023

Describe the bug
datahub-protobuf doesn't parse message comment correctly

To Reproduce
Steps to reproduce the behavior:

  1. Using datahub-protobuf 0.9.6
  2. Modify metadata-integration/java/datahub-protobuf/src/test/resources/protobuf/messageB.proto, add message OuterMessage above message MessageB and compile proto.
// Description for OuterMessage
message OuterMessage {
  MessageB messageB = 1;
}
  1. Add the function getTestProtobufGraph in metadata-integration/java/datahub-protobuf/src/test/java/datahub/protobuf/TestFixtures.java and modify metadata-integration/java/datahub-protobuf/src/test/java/datahub/protobuf/visitors/dataset/DescriptionVisitorTest.java to find the root message
public static ProtobufGraph getTestProtobufGraph(String protoPackage, String filename, String m) throws IOException {
    return new ProtobufGraph(getTestProtobufFileSet(protoPackage, filename), m);
}
ProtobufGraph graph = getTestProtobufGraph("protobuf", "messageB", "protobuf.MessageB");
  1. Run the test

Expected behavior
I hope to get the description of MessageB

/**
 * This contains nested types.
 *
 * Owned by TeamB
 */

But it read the description of OuterMessage.

@tinolyu tinolyu added the bug Bug report label Jan 23, 2023
@tinolyu
Copy link
Contributor Author

tinolyu commented Jan 24, 2023

It seems this PR removed the message check messageProto() == fileProto().getMessageType(loc.getPath(1)) in datahub.protobuf.model.ProtobufElement, and it now could not parse the message comment correctly if there are multiple messages in a proto file.

@david-leifker
Copy link
Collaborator

I believe this was fixed here ?

@tinolyu
Copy link
Contributor Author

tinolyu commented Jan 24, 2023

Is it in the v0.9.6.1?

I believe this was fixed here ?

@tinolyu
Copy link
Contributor Author

tinolyu commented Jan 24, 2023

I believe this was fixed here ?

It still couldn't work.

@david-leifker david-leifker self-assigned this Jan 24, 2023
@david-leifker
Copy link
Collaborator

0.9.6.1 doesn't contain the fix, would be included in the next release. Will add test to verify.

@github-actions
Copy link

This issue is stale because it has been open for 30 days with no activity. If you believe this is still an issue on the latest DataHub release please leave a comment with the version that you tested it with. If this is a question/discussion please head to https://slack.datahubproject.io. For feature requests please use https://feature-requests.datahubproject.io

@github-actions github-actions bot added the stale label Feb 24, 2023
@github-actions
Copy link

This issue was closed because it has been inactive for 30 days since being marked as stale.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report stale
Projects
None yet
Development

No branches or pull requests

2 participants