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 the scalapb json marshaller for sealed-oneof messages #3342

Merged
merged 8 commits into from Mar 9, 2021

Conversation

ateirney
Copy link
Contributor

@ateirney ateirney commented Feb 11, 2021

When returning a ScalaPB message of the sealed-oneof flavour,
the JSON marshaller does not marshal the wrapping message.

This changeset corrects that by detecting the case where the value passed in
is a GeneratedSealedOneof, and then marshalls the container message.

@CLAassistant
Copy link

CLAassistant commented Feb 11, 2021

CLA assistant check
All committers have signed the CLA.

@ikhoon ikhoon added the defect label Feb 11, 2021
@ikhoon ikhoon added this to the 1.5.0 milestone Feb 11, 2021
@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #3342 (6fded71) into master (d2d6207) will increase coverage by 0.02%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3342      +/-   ##
============================================
+ Coverage     74.16%   74.18%   +0.02%     
- Complexity    13276    13290      +14     
============================================
  Files          1158     1159       +1     
  Lines         50626    50740     +114     
  Branches       6491     6504      +13     
============================================
+ Hits          37548    37643      +95     
- Misses         9765     9778      +13     
- Partials       3313     3319       +6     
Impacted Files Coverage Δ Complexity Δ
...rp/armeria/client/WebClientRequestPreparation.java 30.00% <0.00%> (-1.04%) 7.00 <0.00> (ø)
...in/example/armeria/grpc/kotlin/HelloServiceImpl.kt 75.86% <ø> (+3.31%) 7.00 <0.00> (-3.00) ⬆️
...meria/spring/AbstractArmeriaAutoConfiguration.java 64.70% <0.00%> (ø) 6.00 <0.00> (ø)
.../spring/ArmeriaBeanPostProcessorConfiguration.java 100.00% <ø> (ø) 2.00 <0.00> (ø)
...meria/internal/common/grpc/HttpStreamDeframer.java 67.34% <50.00%> (-1.55%) 11.00 <1.00> (ø)
...inecorp/armeria/server/grpc/FramedGrpcService.java 82.17% <50.00%> (-0.51%) 30.00 <0.00> (ø)
.../linecorp/armeria/common/stream/StreamMessage.java 80.76% <57.14%> (-3.68%) 22.00 <2.00> (+2.00) ⬇️
...armeria/internal/common/thrift/ThriftFunction.java 86.15% <78.57%> (-1.25%) 51.00 <3.00> (+2.00) ⬇️
...p/armeria/common/stream/FuseableStreamMessage.java 87.64% <87.64%> (ø) 10.00 <10.00> (?)
...orp/armeria/common/AbstractHttpRequestBuilder.java 87.21% <90.90%> (+0.33%) 50.00 <1.00> (+3.00)
... and 33 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 d2d6207...a4c77c2. Read the comment docs.

@ikhoon ikhoon modified the milestones: 1.5.0, 1.6.0 Feb 15, 2021
@trustin
Copy link
Member

trustin commented Feb 26, 2021

@andrew-teirney Gentle ping

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks, @andrew-teirney and @ikhoon !

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks! @andrew-teirney

.appveyor.build.sh Outdated Show resolved Hide resolved
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks, @andrew-teirney and @ikhoon !!!

@trustin trustin merged commit 9e17885 into line:master Mar 9, 2021
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

5 participants