Skip to content

Commit

Permalink
[Dubbo-4525] fix Clusterutils.mergeurl, make sure specific keys from …
Browse files Browse the repository at this point in the history 
…provider side not override by that from consumer. (#4533)

Fixes #4525
  • Loading branch information
@chickenlj @cvictory
chickenlj authored and cvictory committed Jul 12, 2019
1 parent 4676d22 commit a5f6090
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 49 deletions.
53 changes: 16 additions & 37 deletions dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/support/ClusterUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,11 @@
package org.apache.dubbo.rpc.cluster.support;

import org.apache.dubbo.common.URL;
import org.apache.dubbo.common.utils.StringUtils;
import org.apache.dubbo.remoting.Constants;

import java.util.HashMap;
import java.util.Map;

import static org.apache.dubbo.rpc.cluster.Constants.TAG_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.ALIVE_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.APPLICATION_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.CORE_THREADS_KEY;
Expand All @@ -41,6 +39,7 @@
import static org.apache.dubbo.remoting.Constants.DUBBO_VERSION_KEY;
import static org.apache.dubbo.rpc.Constants.INVOKER_LISTENER_KEY;
import static org.apache.dubbo.rpc.Constants.REFERENCE_FILTER_KEY;
import static org.apache.dubbo.rpc.cluster.Constants.TAG_KEY;

/**
* ClusterUtils
Expand Down Expand Up @@ -81,55 +80,35 @@ public static URL mergeUrl(URL remoteUrl, Map<String, String> localMap) {
}

if (localMap != null && localMap.size() > 0) {
// All providers come to here have been filtered by group, which means only those providers that have the exact same group value with the consumer could come to here.
// So, generally, we don't need to care about the group value here.
// But when comes to group merger, there is an exception, the consumer group may be '*' while the provider group can be empty or any other values.
String remoteGroup = map.get(GROUP_KEY);
String remoteRelease = map.get(RELEASE_KEY);
map.putAll(localMap);
if (StringUtils.isNotEmpty(remoteGroup)) {
map.put(GROUP_KEY, remoteGroup);
}
// we should always keep the Provider RELEASE_KEY not overrode by the the value on Consumer side.
map.remove(RELEASE_KEY);
if (StringUtils.isNotEmpty(remoteRelease)) {
map.put(RELEASE_KEY, remoteRelease);
}
}
if (remoteMap != null && remoteMap.size() > 0) {
// Use version passed from provider side
reserveRemoteValue(DUBBO_VERSION_KEY, map, remoteMap);
reserveRemoteValue(VERSION_KEY, map, remoteMap);
reserveRemoteValue(METHODS_KEY, map, remoteMap);
reserveRemoteValue(TIMESTAMP_KEY, map, remoteMap);
reserveRemoteValue(TAG_KEY, map, remoteMap);
// TODO, for compatibility consideration, we cannot simply change the value behind APPLICATION_KEY from Consumer to Provider. So just add an extra key here.
// Reserve application name from provider.
Map<String, String> copyOfLocalMap = new HashMap<>(localMap);
copyOfLocalMap.remove(GROUP_KEY);
copyOfLocalMap.remove(RELEASE_KEY);
copyOfLocalMap.remove(DUBBO_VERSION_KEY);
copyOfLocalMap.remove(VERSION_KEY);
copyOfLocalMap.remove(METHODS_KEY);
copyOfLocalMap.remove(TIMESTAMP_KEY);
copyOfLocalMap.remove(TAG_KEY);

map.putAll(copyOfLocalMap);

map.put(REMOTE_APPLICATION_KEY, remoteMap.get(APPLICATION_KEY));

// Combine filters and listeners on Provider and Consumer
String remoteFilter = remoteMap.get(REFERENCE_FILTER_KEY);
String localFilter = localMap.get(REFERENCE_FILTER_KEY);
String localFilter = copyOfLocalMap.get(REFERENCE_FILTER_KEY);
if (remoteFilter != null && remoteFilter.length() > 0
&& localFilter != null && localFilter.length() > 0) {
localMap.put(REFERENCE_FILTER_KEY, remoteFilter + "," + localFilter);
map.put(REFERENCE_FILTER_KEY, remoteFilter + "," + localFilter);
}
String remoteListener = remoteMap.get(INVOKER_LISTENER_KEY);
String localListener = localMap.get(INVOKER_LISTENER_KEY);
String localListener = copyOfLocalMap.get(INVOKER_LISTENER_KEY);
if (remoteListener != null && remoteListener.length() > 0
&& localListener != null && localListener.length() > 0) {
localMap.put(INVOKER_LISTENER_KEY, remoteListener + "," + localListener);
map.put(INVOKER_LISTENER_KEY, remoteListener + "," + localListener);
}
}

return remoteUrl.clearParameters().addParameters(map);
}

private static void reserveRemoteValue(String key, Map<String, String> map, Map<String, String> remoteMap) {
String remoteValue = remoteMap.get(key);
if (StringUtils.isNotEmpty(remoteValue)) {
map.put(key, remoteValue);
}
}

}
74 changes: 73 additions & 1 deletion dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/support/ClusterUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,27 @@
import org.junit.jupiter.api.Test;

import static org.apache.dubbo.common.constants.CommonConstants.ALIVE_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.APPLICATION_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.CLUSTER_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.CORE_THREADS_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.DEFAULT_KEY_PREFIX;
import static org.apache.dubbo.common.constants.CommonConstants.DUBBO_PROTOCOL;
import static org.apache.dubbo.common.constants.CommonConstants.GROUP_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.METHODS_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.PID_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.QUEUES_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.RELEASE_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.REMOTE_APPLICATION_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.THREADPOOL_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.THREADS_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.THREAD_NAME_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.TIMEOUT_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.TIMESTAMP_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.VERSION_KEY;
import static org.apache.dubbo.common.constants.CommonConstants.DUBBO_PROTOCOL;
import static org.apache.dubbo.remoting.Constants.DUBBO_VERSION_KEY;
import static org.apache.dubbo.rpc.Constants.REFERENCE_FILTER_KEY;
import static org.apache.dubbo.rpc.cluster.Constants.LOADBALANCE_KEY;
import static org.apache.dubbo.rpc.cluster.Constants.TAG_KEY;

public class ClusterUtilsTest {

Expand All @@ -60,11 +70,15 @@ public void testMergeUrl() throws Exception {
.addParameter(DEFAULT_KEY_PREFIX + QUEUES_KEY, Integer.MAX_VALUE)
.addParameter(DEFAULT_KEY_PREFIX + ALIVE_KEY, Integer.MAX_VALUE)
.addParameter(DEFAULT_KEY_PREFIX + THREAD_NAME_KEY, "test")
.addParameter(APPLICATION_KEY, "provider")
.addParameter(REFERENCE_FILTER_KEY, "filter1,filter2")
.build();

URL consumerURL = new URLBuilder(DUBBO_PROTOCOL, "localhost", 55555)
.addParameter(PID_KEY, "1234")
.addParameter(THREADPOOL_KEY, "foo")
.addParameter(APPLICATION_KEY, "consumer")
.addParameter(REFERENCE_FILTER_KEY, "filter3")
.build();

URL url = ClusterUtils.mergeUrl(providerURL, consumerURL.getParameters());
Expand All @@ -91,6 +105,64 @@ public void testMergeUrl() throws Exception {
Assertions.assertEquals(url.getPassword(), "password");
Assertions.assertEquals(url.getParameter(PID_KEY), "1234");
Assertions.assertEquals(url.getParameter(THREADPOOL_KEY), "foo");
Assertions.assertEquals(url.getParameter(APPLICATION_KEY), "consumer");
Assertions.assertEquals(url.getParameter(REMOTE_APPLICATION_KEY), "provider");
Assertions.assertEquals(url.getParameter(REFERENCE_FILTER_KEY), "filter1,filter2,filter3");
}

@Test
public void testUseProviderParams() {
// present in both local and remote, but uses remote value.
URL localURL = URL.valueOf("dubbo://localhost:20880/DemoService?version=local&group=local&dubbo=local&release=local" +
"&methods=local&tag=local&timestamp=local");
URL remoteURL = URL.valueOf("dubbo://localhost:20880/DemoService?version=remote&group=remote&dubbo=remote&release=remote" +
"&methods=remote&tag=remote&timestamp=remote");
URL mergedUrl = ClusterUtils.mergeUrl(remoteURL, localURL.getParameters());

Assertions.assertEquals(remoteURL.getParameter(VERSION_KEY), mergedUrl.getParameter(VERSION_KEY));
Assertions.assertEquals(remoteURL.getParameter(GROUP_KEY), mergedUrl.getParameter(GROUP_KEY));
Assertions.assertEquals(remoteURL.getParameter(DUBBO_VERSION_KEY), mergedUrl.getParameter(DUBBO_VERSION_KEY));
Assertions.assertEquals(remoteURL.getParameter(RELEASE_KEY), mergedUrl.getParameter(RELEASE_KEY));
Assertions.assertEquals(remoteURL.getParameter(METHODS_KEY), mergedUrl.getParameter(METHODS_KEY));
Assertions.assertEquals(remoteURL.getParameter(TIMESTAMP_KEY), mergedUrl.getParameter(TIMESTAMP_KEY));
Assertions.assertEquals(remoteURL.getParameter(TAG_KEY), mergedUrl.getParameter(TAG_KEY));

// present in local url but not in remote url, parameters of remote url is empty
localURL = URL.valueOf("dubbo://localhost:20880/DemoService?version=local&group=local&dubbo=local&release=local" +
"&methods=local&tag=local&timestamp=local");
remoteURL = URL.valueOf("dubbo://localhost:20880/DemoService");
mergedUrl = ClusterUtils.mergeUrl(remoteURL, localURL.getParameters());

Assertions.assertNull(mergedUrl.getParameter(VERSION_KEY));
Assertions.assertNull(mergedUrl.getParameter(GROUP_KEY));
Assertions.assertNull(mergedUrl.getParameter(DUBBO_VERSION_KEY));
Assertions.assertNull(mergedUrl.getParameter(RELEASE_KEY));
Assertions.assertNull(mergedUrl.getParameter(METHODS_KEY));
Assertions.assertNull(mergedUrl.getParameter(TIMESTAMP_KEY));
Assertions.assertNull(mergedUrl.getParameter(TAG_KEY));

// present in local url but not in remote url
localURL = URL.valueOf("dubbo://localhost:20880/DemoService?version=local&group=local&dubbo=local&release=local" +
"&methods=local&tag=local&timestamp=local");
remoteURL = URL.valueOf("dubbo://localhost:20880/DemoService?key=value");
mergedUrl = ClusterUtils.mergeUrl(remoteURL, localURL.getParameters());

Assertions.assertNull(mergedUrl.getParameter(VERSION_KEY));
Assertions.assertNull(mergedUrl.getParameter(GROUP_KEY));
Assertions.assertNull(mergedUrl.getParameter(DUBBO_VERSION_KEY));
Assertions.assertNull(mergedUrl.getParameter(RELEASE_KEY));
Assertions.assertNull(mergedUrl.getParameter(METHODS_KEY));
Assertions.assertNull(mergedUrl.getParameter(TIMESTAMP_KEY));
Assertions.assertNull(mergedUrl.getParameter(TAG_KEY));

// present in both local and remote, uses local url params
localURL = URL.valueOf("dubbo://localhost:20880/DemoService?loadbalance=local&timeout=1000&cluster=local");
remoteURL = URL.valueOf("dubbo://localhost:20880/DemoService?loadbalance=remote&timeout=2000&cluster=remote");
mergedUrl = ClusterUtils.mergeUrl(remoteURL, localURL.getParameters());

Assertions.assertEquals(localURL.getParameter(CLUSTER_KEY), mergedUrl.getParameter(CLUSTER_KEY));
Assertions.assertEquals(localURL.getParameter(TIMEOUT_KEY), mergedUrl.getParameter(TIMEOUT_KEY));
Assertions.assertEquals(localURL.getParameter(LOADBALANCE_KEY), mergedUrl.getParameter(LOADBALANCE_KEY));
}

}
6 changes: 3 additions & 3 deletions ...ubbo/config/spring/beans/factory/annotation/ReferenceAnnotationBeanPostProcessorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
@TestPropertySource(properties = {
"packagesToScan = org.apache.dubbo.config.spring.context.annotation.provider",
"consumer.version = ${demo.service.version}",
"consumer.url = dubbo://127.0.0.1:12345",
"consumer.url = dubbo://127.0.0.1:12345?version=2.5.7",
})
public class ReferenceAnnotationBeanPostProcessorTest {

Expand Down Expand Up @@ -221,7 +221,7 @@ public DemoService getDemoServiceFromAncestor() {
return demoServiceFromAncestor;
}

@Reference(id = "my-reference-bean", version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@Reference(id = "my-reference-bean", version = "2.5.7", url = "dubbo://127.0.0.1:12345?version=2.5.7")
public void setDemoServiceFromAncestor(DemoService demoServiceFromAncestor) {
this.demoServiceFromAncestor = demoServiceFromAncestor;
}
Expand Down Expand Up @@ -259,7 +259,7 @@ public DemoService getDemoService() {
return demoService;
}

@com.alibaba.dubbo.config.annotation.Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@com.alibaba.dubbo.config.annotation.Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345?version=2.5.7")
public void setDemoService(DemoService demoService) {
this.demoService = demoService;
}
Expand Down
10 changes: 6 additions & 4 deletions ...ava/org/apache/dubbo/config/spring/context/annotation/consumer/ConsumerConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
@PropertySource("META-INF/default.properties")
public class ConsumerConfiguration {

private static final String remoteURL = "dubbo://127.0.0.1:12345?version=2.5.7";

/**
* Current application configuration, to replace XML config:
* <prev>
Expand Down Expand Up @@ -67,7 +69,7 @@ public RegistryConfig registryConfig() {
@Autowired
private DemoService autowiredDemoService;

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@Reference(version = "2.5.7", url = remoteURL)
private DemoService demoService;

public DemoService getDemoService() {
Expand All @@ -86,7 +88,7 @@ public Child c() {

public static abstract class Ancestor {

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@Reference(version = "2.5.7", url = remoteURL)
private DemoService demoServiceFromAncestor;

public DemoService getDemoServiceFromAncestor() {
Expand All @@ -106,7 +108,7 @@ public DemoService getDemoServiceFromParent() {
return demoServiceFromParent;
}

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@Reference(version = "2.5.7", url = remoteURL)
public void setDemoServiceFromParent(DemoService demoServiceFromParent) {
this.demoServiceFromParent = demoServiceFromParent;
}
Expand All @@ -118,7 +120,7 @@ public static class Child extends Parent {
@Autowired
private DemoService demoService;

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345")
@Reference(version = "2.5.7", url = remoteURL)
private DemoService demoServiceFromChild;


Expand Down
10 changes: 6 additions & 4 deletions ...pache/dubbo/config/spring/context/annotation/consumer/test/TestConsumerConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@
@EnableTransactionManagement
public class TestConsumerConfiguration {

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345", application = "dubbo-demo-application")
private static final String remoteURL = "dubbo://127.0.0.1:12345?version=2.5.7";

@Reference(version = "2.5.7", url = remoteURL, application = "dubbo-demo-application")
private DemoService demoService;

@Autowired
Expand All @@ -61,7 +63,7 @@ public TestConsumerConfiguration.Child c() {

public static abstract class Ancestor {

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345", application = "dubbo-demo-application")
@Reference(version = "2.5.7", url = remoteURL, application = "dubbo-demo-application")
private DemoService demoServiceFromAncestor;

public DemoService getDemoServiceFromAncestor() {
Expand All @@ -81,7 +83,7 @@ public DemoService getDemoServiceFromParent() {
return demoServiceFromParent;
}

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345", application = "dubbo-demo-application")
@Reference(version = "2.5.7", url = remoteURL, application = "dubbo-demo-application")
public void setDemoServiceFromParent(DemoService demoServiceFromParent) {
this.demoServiceFromParent = demoServiceFromParent;
}
Expand All @@ -90,7 +92,7 @@ public void setDemoServiceFromParent(DemoService demoServiceFromParent) {

public static class Child extends TestConsumerConfiguration.Parent {

@Reference(version = "2.5.7", url = "dubbo://127.0.0.1:12345", application = "dubbo-demo-application")
@Reference(version = "2.5.7", url = remoteURL, application = "dubbo-demo-application")
private DemoService demoServiceFromChild;

public DemoService getDemoServiceFromChild() {
Expand Down

0 comments on commit a5f6090

Please sign in to comment.