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

@herbyderby herbyderby commented Mar 23, 2020

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 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 Mar 24, 2020
@zhangkun83
Copy link
Contributor

@zhangkun83 zhangkun83 commented Mar 24, 2020

LGTM

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

@zhangkun83 zhangkun83 Mar 24, 2020

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

@ejona86 ejona86 Mar 24, 2020

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

@herbyderby herbyderby Mar 24, 2020

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

@zhangkun83 zhangkun83 Mar 30, 2020

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 label Mar 30, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:force-run label Mar 30, 2020
@zhangkun83 zhangkun83 merged commit e081f41 into grpc:master Mar 30, 2020
13 checks passed
dfawley pushed a commit to dfawley/grpc-java that referenced this issue 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