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

core: copy the SchemaDescriptor when rebuilding descriptor #6851

Merged
merged 2 commits into from Mar 30, 2020

Conversation

herbyderby
Copy link
Contributor

useMarshalledMessages works by duplicating a ServerServiceDefinition while replacing just the marshallers. It currently does not copy over the SchemaDescriptors, which breaks at least the ProtoReflectionService.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 23, 2020

CLA Check
The committers are authorized under a signed CLA.

@zhangkun83 zhangkun83 requested a review from ejona86 March 24, 2020 01:20
@zhangkun83
Copy link
Contributor

LGTM

@@ -187,13 +187,15 @@ public InputStream parse(final InputStream stream) {
wrappedMethods.add(wrapMethod(definition, wrappedMethodDescriptor));
Copy link
Contributor

Choose a reason for hiding this comment

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

wrappedMethods is never used, and this is causing the CI error.

Copy link
Member

Choose a reason for hiding this comment

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

Look like the earlier for loop should not have been deleted and should occur after serviceBuilder is created (just like before).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not sure what happened--some patching problem on my end. Should look something like this:

    // Build the new service descriptor
    final ServiceDescriptor.Builder serviceDescriptorBuilder =
        ServiceDescriptor.newBuilder(serviceDef.getServiceDescriptor().getName())
            .setSchemaDescriptor(serviceDef.getServiceDescriptor().getSchemaDescriptor());
    // Create the new service definition.
    final ServerServiceDefinition.Builder serviceBuilder =
        ServerServiceDefinition.builder(serviceDescriptorBuilder.build());
    for (ServerMethodDefinition<?, ?> definition : wrappedMethods) {
      serviceBuilder.addMethod(definition);
    }
    return serviceBuilder.build();

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied fix.

useMarshalledMessages works by duplicating a ServerServiceDefinition while replacing just the marshallers. It currently does not copy over the SchemaDescriptors, which breaks at least the ProtoReflectionService.
@zhangkun83 zhangkun83 added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Mar 30, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Mar 30, 2020
@zhangkun83 zhangkun83 merged commit e081f41 into grpc:master Mar 30, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
useMarshalledMessages works by duplicating a ServerServiceDefinition while replacing just the marshallers. It currently does not copy over the SchemaDescriptors, which breaks at least the ProtoReflectionService.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants