You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "zhaixiaoxiang (GitHub)" <gi...@apache.org> on 2018/12/27 05:46:40 UTC
[GitHub] [incubator-dubbo] zhaixiaoxiang opened pull request #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Fix #3069
## What is the purpose of the change
Fix NumberFormatException for input string "" or " " or null in convertPrimitive(Class<?> type, String value)
## Brief changelog
Update files:
ClassHelper
StringUtils
ClassHelperTest
StringUtilsTest
## Verifying this change
Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] Make sure there is a [GITHUB_issue](https://github.com/apache/incubator-dubbo/issues) field for the change (usually before you start working on it). Trivial changes like typos do not require a GITHUB issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
- [x] Format the pull request title like `[Dubbo-XXX] Fix UnknownException when host config not exist #XXX`. Each commit in the pull request should have a meaningful subject line and body.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/incubator-dubbo/tree/master/dubbo-test).
- [x] Run `mvn clean install -DskipTests` & `mvn clean test-compile failsafe:integration-test` to make sure unit-test and integration-test pass.
- [ ] If this contribution is large, please follow the [Software Donation Guide](https://github.com/apache/incubator-dubbo/wiki/Software-donation-guide).
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
duplicate: https://github.com/apache/incubator-dubbo/pull/3075
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] zhaixiaoxiang commented on issue #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "zhaixiaoxiang (GitHub)" <gi...@apache.org>.
> Hi, @zhaixiaoxiang
>
> ```
> if (type == char.class || type == Character.class) {
> return value.length() > 0 ? value.charAt(0) : '\0';
> }
> ```
> Return '\0' may be confusing.
>
> How about put the `if StringUtils.isRealBlank(value)` in the first line of this method?
> But be careful if the '\0' is used in other place.
Emm, I'm not sure whether '\0' is used in other place.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3076: [Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
existing **isRealBlank** can be change with given one. It is not exact replacement, I have paste the whole code of isRealBlank with modified one so that you don't have to spend time modifying line by line. In the modified one I have remove the extra **return** statement because the **return** at the end of method is good enough for the purpose.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] zhaixiaoxiang commented on pull request
#3076: [Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "zhaixiaoxiang (GitHub)" <gi...@apache.org>.
Hi, what can be replaced by what?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] zhaixiaoxiang commented on issue #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "zhaixiaoxiang (GitHub)" <gi...@apache.org>.
> Hi, @zhaixiaoxiang
>
> ```
> if (type == char.class || type == Character.class) {
> return value.length() > 0 ? value.charAt(0) : '\0';
> }
> ```
> Return '\0' may be confusing.
>
> How about put the `if StringUtils.isRealBlank(value)` in the first line of this method?
> But be careful if the '\0' is used in other place.
Yes, ClassHelper.convertPrimitive() is only used in one place, and '\0' is not used, so I remove '\0'.
```
if (StringUtils.isNotEmpty(value) && ClassHelper.isTypeMatch(method.getParameterTypes()[0], value)) {
method.invoke(this, ClassHelper.convertPrimitive(method.getParameterTypes()[0], value));
}
```
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] cdfive commented on issue #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "cdfive (GitHub)" <gi...@apache.org>.
Hi, @zhaixiaoxiang
```
if (type == char.class || type == Character.class) {
return value.length() > 0 ? value.charAt(0) : '\0';
}
```
Return '\0' may be confusing.
How about put the `if StringUtils.isRealBlank(value)` in the first line of this method?
But be careful if the '\0' is used in other place.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3076?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@0282a42`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `84.21%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3076?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3076 +/- ##
=========================================
Coverage ? 63.56%
Complexity ? 75
=========================================
Files ? 653
Lines ? 28228
Branches ? 4807
=========================================
Hits ? 17943
Misses ? 8010
Partials ? 2275
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3076?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...java/org/apache/dubbo/rpc/cluster/RouterChain.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3076/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvUm91dGVyQ2hhaW4uamF2YQ==) | `96.87% <ø> (ø)` | `0 <0> (?)` | |
| [...ster/router/condition/config/AppRouterFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3076/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvcm91dGVyL2NvbmRpdGlvbi9jb25maWcvQXBwUm91dGVyRmFjdG9yeS5qYXZh) | `88.88% <ø> (ø)` | `0 <0> (?)` | |
| [.../dubbo/registry/integration/RegistryDirectory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3076/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9pbnRlZ3JhdGlvbi9SZWdpc3RyeURpcmVjdG9yeS5qYXZh) | `80% <0%> (ø)` | `0 <0> (?)` | |
| [...ava/org/apache/dubbo/common/utils/StringUtils.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3076/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vdXRpbHMvU3RyaW5nVXRpbHMuamF2YQ==) | `86.85% <100%> (ø)` | `0 <0> (?)` | |
| [...ava/org/apache/dubbo/common/utils/ClassHelper.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3076/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vdXRpbHMvQ2xhc3NIZWxwZXIuamF2YQ==) | `69.72% <83.33%> (ø)` | `0 <0> (?)` | |
| [...ster/router/condition/config/ListenableRouter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3076/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvcm91dGVyL2NvbmRpdGlvbi9jb25maWcvTGlzdGVuYWJsZVJvdXRlci5qYXZh) | `28.57% <83.33%> (ø)` | `0 <0> (?)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3076?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3076?src=pr&el=footer). Last update [0282a42...925ccfb](https://codecov.io/gh/apache/incubator-dubbo/pull/3076?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3076: [Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
I think this return (after for loop)is not requires and remove the else and keep just `return true;`
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] ralf0131 commented on pull request #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "ralf0131 (GitHub)" <gi...@apache.org>.
Hi,
I think we don't need to call `isRealBlank`, because there is too much to check. If we really want to check, we need to make sure the input `value` is numeric. I have left comment on #3093
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] zhaixiaoxiang commented on issue #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "zhaixiaoxiang (GitHub)" <gi...@apache.org>.
> Hi, @zhaixiaoxiang
>
> ```
> if (type == char.class || type == Character.class) {
> return value.length() > 0 ? value.charAt(0) : '\0';
> }
> ```
> Return '\0' may be confusing.
>
> How about put the `if StringUtils.isRealBlank(value)` in the first line of this method?
> But be careful if the '\0' is used in other place.
ClassHelper.convertPrimitive() is only used in exactly one place, and '\0' is not used, so I remove '\0'.
```
if (StringUtils.isNotEmpty(value) && ClassHelper.isTypeMatch(method.getParameterTypes()[0], value)) {
method.invoke(this, ClassHelper.convertPrimitive(method.getParameterTypes()[0], value));
}
```
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3076: [Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Can be replaced by 'StringUtils.isNotEmpty(cs)'
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] zhaixiaoxiang closed pull request #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "zhaixiaoxiang (GitHub)" <gi...@apache.org>.
[ pull request closed by zhaixiaoxiang ]
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] zhaixiaoxiang commented on issue #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "zhaixiaoxiang (GitHub)" <gi...@apache.org>.
> Hi, @zhaixiaoxiang
>
> ```
> if (type == char.class || type == Character.class) {
> return value.length() > 0 ? value.charAt(0) : '\0';
> }
> ```
> Return '\0' may be confusing.
>
> How about put the `if StringUtils.isRealBlank(value)` in the first line of this method?
> But be careful if the '\0' is used in other place.
Emm, I'm not sure whether '\0' us used in other place.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3076: [Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
can be replace is
```
public static boolean isRealBlank(CharSequence cs) {
int strLen;
if (cs != null && (strLen = cs.length()) != 0) {
for (int i = 0; i < strLen; ++i) {
if (!Character.isWhitespace(cs.charAt(i))) {
return false;
}
}
}
return true;
}
```
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] cdfive commented on issue #3076:
[Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "cdfive (GitHub)" <gi...@apache.org>.
Hi, @zhaixiaoxiang
```
if (type == char.class || type == Character.class) {
return value.length() > 0 ? value.charAt(0) : '\0';
}
```
Return '\0' may be confusing.
How about put the `if StringUtils.isRealBlank(value)` in the first line of this method?
But be careful if the '\0' is used.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3076: [Dubbo-3069] Fix NumberFormatException for input string ""
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
plz ignore my comment.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3076 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org