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

Use Class.getDeclaredField() instead of getField() when fetching Thrift metadata. #1729

Merged
merged 4 commits into from Apr 22, 2019

Conversation

edgao
Copy link
Contributor

@edgao edgao commented Apr 19, 2019

Related: #1728

Motivation:

When a user builds a .thrift file with the private-members option enabled,
Armeria fails to find some fields in the generated code.

Modifications:

  • Use Class.getDeclaredField() instead of getField().

Result:

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2019

CLA assistant check
All committers have signed the CLA.

@edgao edgao changed the title [WIP] use getDeclaredField use getDeclaredField Apr 19, 2019
@edgao edgao marked this pull request as ready for review April 19, 2019 07:33
@edgao
Copy link
Contributor Author

edgao commented Apr 19, 2019

Update: Turns out this is because we use the java:private-members option when generating thrift code, so the unit test I added doesn't actually do anything. Specifically, without this option set, the TestService class contains this internal class:

public static class ping_result implements org.apache.thrift.TBase<ping_result, ping_result._Fields>, java.io.Serializable, Cloneable, Comparable<ping_result>   {
    // <snip>
    public TestException te; // required
    // <snip>
}

But with private-members enabled, te is a private field, and so getFields does not return it.

Copy link
Collaborator

@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 for the detailed analysis. Could you remove the new test method since we now know it does not add anything to the test?

@trustin trustin added the defect label Apr 19, 2019
@trustin trustin added this to the 0.84.0 milestone Apr 19, 2019
This reverts commit 2c5d13d.
@codecov
Copy link

codecov bot commented Apr 20, 2019

Codecov Report

Merging #1729 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1729      +/-   ##
============================================
+ Coverage     72.86%   72.89%   +0.03%     
- Complexity     8121     8128       +7     
============================================
  Files           734      734              
  Lines         32374    32374              
  Branches       3980     3980              
============================================
+ Hits          23589    23600      +11     
+ Misses         6746     6741       -5     
+ Partials       2039     2033       -6
Impacted Files Coverage Δ Complexity Δ
...necorp/armeria/internal/thrift/ThriftFunction.java 78.86% <100%> (ø) 45 <0> (ø) ⬇️
...rp/armeria/common/stream/DefaultStreamMessage.java 89.28% <0%> (-1.43%) 53% <0%> (-1%)
...inecorp/armeria/server/grpc/ArmeriaServerCall.java 86.87% <0%> (+0.38%) 84% <0%> (+1%) ⬆️
...inecorp/armeria/server/HttpResponseSubscriber.java 81.86% <0%> (+1.39%) 59% <0%> (+3%) ⬆️
...a/common/grpc/protocol/ArmeriaMessageDeframer.java 70.81% <0%> (+1.43%) 47% <0%> (+2%) ⬆️
...om/linecorp/armeria/client/HttpSessionHandler.java 61.2% <0%> (+1.72%) 29% <0%> (+1%) ⬆️
...spring/web/reactive/ArmeriaServerHttpResponse.java 77.22% <0%> (+1.98%) 29% <0%> (+1%) ⬆️

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 8d0e58e...6d2694d. Read the comment docs.

@trustin trustin changed the title use getDeclaredField Use Class.getDeclaredField() instead of getField() when fetching Thrift metadata. Apr 22, 2019
@trustin
Copy link
Collaborator

trustin commented Apr 22, 2019

@hyangtack Please review. :-)

Copy link
Contributor

@hyangtack hyangtack left a comment

Choose a reason for hiding this comment

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

Nice catch! 👍

@trustin trustin removed the request for review from minwoox April 22, 2019 02:06
@trustin trustin merged commit 666155d into line:master Apr 22, 2019
@trustin
Copy link
Collaborator

trustin commented Apr 22, 2019

Thanks again, @edgao !

@edgao
Copy link
Contributor Author

edgao commented Apr 22, 2019

thanks for the super quick turnaround! just curious - do you know when the next official release will be? (not a blocker for us since we've built a patched jar)

@edgao edgao deleted the handle_thrift_exception branch April 22, 2019 02:35
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
…Thrift metadata. (line#1729)

Related: line#1728

Motivation:

When a user builds a `.thrift` file with the `private-members` option enabled,
Armeria fails to find some fields in the generated code.

Modifications:

- Use `Class.getDeclaredField()` instead of `getField()`.

Result:

- Fixes line#1728
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.

armeria-thrift does not handle service method throwing exception
4 participants