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

Judge null for key and value in attachment in RpcContextFilter. #2171

Merged
merged 2 commits into from
Aug 9, 2018

Conversation

carryxyh
Copy link
Member

@carryxyh carryxyh commented Aug 2, 2018

Judge null for key and value in attachment in RpcContextFilter.
If not, NullPointerException will be thrown in RpcContextFilter in some case.

Fix #2127 : #2127 (comment)

If not, NullPointerException will be thrown in RpcContextFilter in some case.
@codecov-io
Copy link

codecov-io commented Aug 2, 2018

Codecov Report

Merging #2171 into master will increase coverage by 0.28%.
The diff coverage is 44.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2171      +/-   ##
============================================
+ Coverage      54.3%   54.58%   +0.28%     
- Complexity     5111     5150      +39     
============================================
  Files           559      569      +10     
  Lines         24981    25036      +55     
  Branches       4453     4457       +4     
============================================
+ Hits          13566    13666     +100     
+ Misses         9380     9326      -54     
- Partials       2035     2044       +9
Impacted Files Coverage Δ Complexity Δ
...ache/dubbo/rpc/protocol/rest/RpcContextFilter.java 63.63% <44.44%> (-4.23%) 8 <1> (+1)
...n/java/org/apache/dubbo/common/utils/NetUtils.java 53.59% <0%> (-8.53%) 44% <0%> (ø)
...apache/dubbo/rpc/protocol/dubbo/FutureAdapter.java 58.06% <0%> (-6.46%) 3% <0%> (ø)
...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java 51.89% <0%> (-3.8%) 10% <0%> (-3%)
...he/dubbo/remoting/transport/netty/NettyServer.java 67.85% <0%> (-3.58%) 8% <0%> (-1%)
...bo/remoting/transport/netty/NettyCodecAdapter.java 53.12% <0%> (-1.57%) 3% <0%> (ø)
...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java 77.2% <0%> (-1.48%) 29% <0%> (ø)
.../dubbo/remoting/transport/netty4/NettyChannel.java 65% <0%> (-1.25%) 22% <0%> (-1%)
...apache/dubbo/rpc/protocol/dubbo/DubboProtocol.java 58.33% <0%> (-0.84%) 30% <0%> (-1%)
...he/dubbo/registry/multicast/MulticastRegistry.java 57.32% <0%> (-0.42%) 38% <0%> (-3%)
... and 16 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 c904589...74d61a2. Read the comment docs.

if (StringUtils.isNotEmpty(value)) {
if (illegalForRest(value)) {
throw new IllegalArgumentException("The attachments of " + RpcContext.class.getSimpleName() + " must not contain ',' or '=' when using rest protocol");
}
Copy link

@diecui1202 diecui1202 Aug 9, 2018

Choose a reason for hiding this comment

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

if ( (StringUtils.isNotEmpty(key) && illegalForRest(key)) || (StringUtils.isNotEmpty(value) && illegalForRest(value)) ) {
    throw new IllegalArgumentException("The attachments of " + RpcContext.class.getSimpleName() + " must not contain ',' or '=' when using rest protocol");
}

Is the above code better ?

* @return true for illegal
*/
private boolean illegalForRest(String v) {
return v.contains(",") || v.contains("=");

Choose a reason for hiding this comment

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

Or judge null place here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Prefer judge null in 'illegalForRest'.
The meaning of this method is that if a value is not null, it should not contains ',' or '='.
I will fix this soon.
:)

@diecui1202
Copy link

diecui1202 commented Aug 9, 2018

LGTM.

@diecui1202 diecui1202 merged commit 895a4dd into apache:master Aug 9, 2018
@carryxyh carryxyh deleted the fix/judgenull branch August 9, 2018 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants