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 a bug where a GrpcDocServicePlugin does not handle exact path map… #1138

Merged
merged 1 commit into from Apr 11, 2018

Conversation

trustin
Copy link
Member

@trustin trustin commented Apr 10, 2018

…pings

Motivation:

When a user adds a GrpcService with ServerBuilder.service(ServiceWithPathMappings),
GrpcDocServicePlugin fails to add the service endpoints to ServiceSpecification.

To work around this issue, a user has to fall back to the old usage:

ServerBuilder sb = ...;
sb.serviceUnder("/", grpcService); // Works
sb.service(grpcService); // Does not work

Modifications:

  • Update the endpoint detection logic in GrpcDocServicePlugin

Result:

…pings

Motivation:

When a user adds a GrpcService with ServerBuilder.service(ServiceWithPathMappings),
GrpcDocServicePlugin fails to add the service endpoints to ServiceSpecification.

To work around this issue, a user has to fall back to the old usage:

    ServerBuilder sb = ...;
    sb.serviceUnder("/", grpcService); // Works
    sb.service(grpcService); // Does not work

Modifications:

- Update the endpoint detection logic in GrpcDocServicePlugin

Result:

- Fixes line#1136
@trustin trustin added the defect label Apr 10, 2018
@trustin trustin added this to the 0.63.0 milestone Apr 10, 2018
@trustin trustin requested a review from anuraaga April 10, 2018 03:28
@codecov
Copy link

codecov bot commented Apr 10, 2018

Codecov Report

Merging #1138 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
- Coverage   72.48%   72.47%   -0.02%     
==========================================
  Files         509      509              
  Lines       22789    22790       +1     
  Branches     2825     2827       +2     
==========================================
- Hits        16519    16516       -3     
- Misses       4768     4770       +2     
- Partials     1502     1504       +2
Impacted Files Coverage Δ
...corp/armeria/server/grpc/GrpcDocServicePlugin.java 90.47% <100%> (+0.05%) ⬆️
...linecorp/armeria/internal/Http2GoAwayListener.java 76% <0%> (-4%) ⬇️
...om/linecorp/armeria/client/HttpClientDelegate.java 77.77% <0%> (-2.23%) ⬇️
...linecorp/armeria/client/HttpRequestSubscriber.java 66.42% <0%> (-0.73%) ⬇️
...corp/armeria/common/logging/DefaultRequestLog.java 79.94% <0%> (-0.51%) ⬇️
...inecorp/armeria/server/HttpResponseSubscriber.java 78.37% <0%> (+0.54%) ⬆️
...corp/armeria/common/stream/FixedStreamMessage.java 92.98% <0%> (+1.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2c1b98...2b0d2af. Read the comment docs.

@trustin trustin merged commit 68cbfc4 into line:master Apr 11, 2018
@trustin trustin deleted the fix_grpc_docservice_plugin branch April 11, 2018 06:15
@trustin
Copy link
Member Author

trustin commented Apr 11, 2018

Thanks, reviewers!

fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
line#1138)

Motivation:

When a user adds a GrpcService with ServerBuilder.service(ServiceWithPathMappings),
GrpcDocServicePlugin fails to add the service endpoints to ServiceSpecification.

To work around this issue, a user has to fall back to the old usage:

    ServerBuilder sb = ...;
    sb.serviceUnder("/", grpcService); // Works
    sb.service(grpcService); // Does not work

Modifications:

- Update the endpoint detection logic in GrpcDocServicePlugin

Result:

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

Successfully merging this pull request may close these issues.

None yet

3 participants