nacos-registry:serviceName split error #4974
Conversation
|
dubbo admin service query doesn't work before I fix it |
Codecov Report
@@ Coverage Diff @@
## master #4974 +/- ##
============================================
+ Coverage 63.9% 63.91% +<.01%
- Complexity 449 451 +2
============================================
Files 769 769
Lines 33163 33160 -3
Branches 5229 5227 -2
============================================
+ Hits 21194 21195 +1
+ Misses 9545 9544 -1
+ Partials 2424 2421 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@charish00 would you mind to try if org.apache.dubbo.common.utils.StringUtils#split works? and it would be nice if you can explain why the original implementation doesn't work.
one more thing, it is not a good practice to depend on org.apache.commons.lang3, it would be very nice if you can throughly remove it from the current implementation.
StringUtils.split("category:serviceInterface::", ":").length return 2 then length will be the only condition(length==4) to judge the serviceName format. "category:serviceInterface::" is a correct serviceName, we can't use the original implementation |
| @@ -252,7 +251,7 @@ private void scheduleServiceNamesLookup(final URL url, final NotifyListener list | |||
| boolean accepted = false; | |||
| for (String category : ALL_SUPPORTED_CATEGORIES) { | |||
| String prefix = category + SERVICE_NAME_SEPARATOR; | |||
| if (StringUtils.startsWith(serviceName, prefix)) { | |||
| if (serviceName != null && serviceName.startsWith(prefix)) { | |||
| import com.alibaba.nacos.api.naming.pojo.Instance; | ||
| import com.alibaba.nacos.api.naming.pojo.ListView; | ||
| import org.apache.commons.lang3.ArrayUtils; | ||
| import org.apache.commons.lang3.StringUtils; |
There was a problem hiding this comment.
Why remove StringUtils usage? It somehow makes our code more elegant.
There was a problem hiding this comment.
I think we should not rely on common-lang3 directly. It is a traverse dependency from nacos. We've already had StringUtils in dubbo. Instead of use another StringUtils, I would prefer to enhance the existing one.
| int length = segments.length; | ||
| if (length != 4) { // must present 4 segments | ||
| return false; | ||
| } | ||
|
|
||
| String category = segments[CATEGORY_INDEX]; | ||
| if (!ArrayUtils.contains(categories, category)) { // no match category | ||
| if (!categories.contains(category)) { // no match category |
There was a problem hiding this comment.
getCategories guarentees it cannot be null.
final List<String> categories = getCategories(url); | return false; | ||
| } | ||
|
|
||
| String serviceInterface = segments[SERVICE_INTERFACE_INDEX]; | ||
| if (!WILDCARD.equals(targetServiceInterface) && | ||
| !StringUtils.equals(targetServiceInterface, serviceInterface)) { // no match service interface | ||
| !targetServiceInterface.equals(serviceInterface)) { // no match service interface |
| return false; | ||
| } | ||
|
|
||
| String group = segments[SERVICE_GROUP_INDEX]; | ||
| return group == null || WILDCARD.equals(targetGroup) | ||
| || StringUtils.equals(targetGroup, group); | ||
| || targetGroup.equals(group); |
There was a problem hiding this comment.
targetGroup cannot be null, but I will use dubbo's StringUtils anyway.
| return false; | ||
| } | ||
|
|
||
| String version = segments[SERVICE_VERSION_INDEX]; | ||
| if (!WILDCARD.equals(targetVersion) && | ||
| !StringUtils.equals(targetVersion, version)) { // no match service version | ||
| !targetVersion.equals(version)) { // no match service version |
There was a problem hiding this comment.
targetVersion cannot be null, but I will use dubbo's StringUtils anyway.
| @@ -113,12 +107,12 @@ public boolean isCompatible(NacosServiceName concreteServiceName) { | |||
| } | |||
|
|
|||
| // Not match comparison | |||
| if (!StringUtils.equals(this.category, concreteServiceName.category) && | |||
| !ArrayUtils.contains(splitPreserveAllTokens(this.category, VALUE_SEPARATOR), concreteServiceName.category)) { | |||
| if (!this.category.equals(concreteServiceName.category) | |||
There was a problem hiding this comment.
will change to use dubbo's stringutils.
| return false; | ||
| } | ||
|
|
||
| if (!StringUtils.equals(this.serviceInterface, concreteServiceName.serviceInterface)) { | ||
| if (!this.serviceInterface.equals(concreteServiceName.serviceInterface)) { |
There was a problem hiding this comment.
will change to use dubbo's stringutils.
| @@ -132,13 +126,12 @@ public boolean isCompatible(NacosServiceName concreteServiceName) { | |||
| } | |||
|
|
|||
| // range condition | |||
| if (!StringUtils.equals(this.version, concreteServiceName.version) && | |||
| !matchRange(this.version, concreteServiceName.version)) { | |||
| if (!this.version.equals(concreteServiceName.version) | |||
There was a problem hiding this comment.
will change to use dubbo's stringutils.
|
@beiwei30 |
|
since I will clean up imports, I will double check. |
|
Okay sir. |
|
sorry, I try to search the method “equals” in dubbo stringUtils, but miss the method “isEquals”. |
What is the purpose of the change
bugfix: serviceName split return the wrong size of segments when group or version is empty
Brief changelog
bugfix: serviceName split return the wrong size of segments
Verifying this change
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[Dubbo-XXX] Fix UnknownException when host config not exist #XXX. Each commit in the pull request should have a meaningful subject line and body.mvn clean install -DskipTests=false&mvn clean test-compile failsafe:integration-testto make sure unit-test and integration-test pass.