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 streaming service resolution and header passing #291

Merged
merged 9 commits into from
Jun 21, 2021
Merged

Conversation

BuddhiWathsala
Copy link
Contributor

Purpose

Fixes ballerina-platform/ballerina-library#1504
Fixes ballerina-platform/ballerina-library#1458

Examples

Checklist

  • Linked to an issue
  • Updated the changelog
  • Added tests

When there is a context, the previous logic does not check for the stream type and it just return a record.
Thus, it leads to a incorrect service resolution. Here, it added the check.
- Inject headers to the message itself
- Client streaming headers pass to the client through streaming connection
- Bidirectional streaming headers pass to the client through streaming iterator
To easily execute sub-tests.
@BuddhiWathsala BuddhiWathsala requested a review from daneshk June 17, 2021 14:57
@codecov
Copy link

codecov bot commented Jun 17, 2021

Codecov Report

Merging #291 (1057107) into master (ada3b3c) will increase coverage by 0.08%.
The diff coverage is 86.04%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #291      +/-   ##
============================================
+ Coverage     74.52%   74.60%   +0.08%     
- Complexity     1062     1072      +10     
============================================
  Files           121      121              
  Lines          6280     6313      +33     
  Branches       1083     1090       +7     
============================================
+ Hits           4680     4710      +30     
+ Misses         1175     1174       -1     
- Partials        425      429       +4     
Impacted Files Coverage Δ
...java/org/ballerinalang/net/grpc/GrpcConstants.java 100.00% <ø> (ø)
...in/java/org/ballerinalang/net/grpc/ClientCall.java 48.10% <50.00%> (+0.04%) ⬆️
...in/java/org/ballerinalang/net/grpc/stubs/Stub.java 87.14% <50.00%> (-0.54%) ⬇️
...in/java/org/ballerinalang/net/grpc/ServerCall.java 46.08% <66.66%> (+0.55%) ⬆️
...erinalang/net/grpc/listener/ServerCallHandler.java 77.64% <80.00%> (+0.14%) ⬆️
grpc-ballerina/streaming_client.bal 80.00% <100.00%> (+6.00%) ⬆️
.../java/org/ballerinalang/net/grpc/MessageUtils.java 43.61% <100.00%> (ø)
...va/org/ballerinalang/net/grpc/OutboundMessage.java 74.60% <100.00%> (+0.83%) ⬆️
...g/ballerinalang/net/grpc/ServicesBuilderUtils.java 66.45% <100.00%> (+0.66%) ⬆️
...grpc/nativeimpl/serviceendpoint/FunctionUtils.java 58.82% <100.00%> (+1.24%) ⬆️
... and 4 more

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 ada3b3c...1057107. Read the comment docs.

@chamil321
Copy link
Contributor

@BuddhiWathsala There are some commented codes as well. please check

@BuddhiWathsala
Copy link
Contributor Author

@BuddhiWathsala There are some commented codes as well. please check

These commented ones are intentional. gRPC has generated codes that are repeatedly use across multiple tests. We maintain these codes as comment for the sake of completeness.

Copy link
Member

@daneshk daneshk left a comment

Choose a reason for hiding this comment

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

LGTM

@daneshk
Copy link
Member

daneshk commented Jun 21, 2021

Let's add another test case to return ContextMessage on the server-side without using a caller object later

@BuddhiWathsala
Copy link
Contributor Author

We already have a similar test for server streaming. I'll add similar tests for client and bidirectional.
https://github.com/ballerina-platform/module-ballerina-grpc/blob/master/grpc-ballerina/tests/32_return_record_with_headers_server_streaming_server.bal

@BuddhiWathsala BuddhiWathsala merged commit 661a245 into master Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants