Copilot commented on code in PR #15925:
URL: https://github.com/apache/dubbo/pull/15925#discussion_r2650361916
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryProtocol.java:
##########
@@ -282,7 +257,8 @@ public <T> Exporter<T> export(final Invoker<T>
originInvoker) throws RpcExceptio
final OverrideListener overrideSubscribeListener = new
OverrideListener(overrideSubscribeUrl, originInvoker);
ConcurrentHashMap<URL, Set<NotifyListener>> overrideListeners =
getProviderConfigurationListener(overrideSubscribeUrl).getOverrideListeners();
- ConcurrentHashMapUtils.computeIfAbsent(overrideListeners,
overrideSubscribeUrl, k -> new ConcurrentHashSet<>())
+ Objects.requireNonNull(ConcurrentHashMapUtils.computeIfAbsent(
+ overrideListeners, overrideSubscribeUrl, k -> new
ConcurrentHashSet<>()))
Review Comment:
The Objects.requireNonNull wrapper is unnecessary here.
ConcurrentHashMapUtils.computeIfAbsent will never return null when the mapping
function returns a non-null value (new ConcurrentHashSet<>()). This defensive
check adds unnecessary complexity without providing any benefit.
```suggestion
ConcurrentHashMapUtils.computeIfAbsent(
overrideListeners, overrideSubscribeUrl, k -> new
ConcurrentHashSet<>())
```
##########
dubbo-registry/dubbo-registry-api/src/test/java/org/apache/dubbo/registry/integration/RegistryProtocolTest.java:
##########
@@ -544,4 +550,104 @@ void testRegisterConsumerUrl() {
verify(registry, times(1)).register(registeredConsumerUrl);
}
+
+ /**
+ * Verifies that multiple ServiceConfigurationListeners registered for the
same
+ * service are preserved and that their configurators are applied
cumulatively.
+ */
+ @Test
+ void testServiceConfigurationListenersAggregation() throws Exception {
+
ApplicationModel.defaultModel().getApplicationConfigManager().setApplication(new
ApplicationConfig("test-app"));
+
+ RegistryProtocol registryProtocol = new RegistryProtocol();
+
+ ModuleModel moduleModel =
ApplicationModel.defaultModel().getDefaultModule();
+
+ Map<String, String> params = new HashMap<>();
+ params.put(INTERFACE_KEY, DemoService.class.getName());
+
+ ServiceConfigURL providerUrl =
+ new ServiceConfigURL("dubbo", "127.0.0.1", 20880,
DemoService.class.getName(), params);
+
+ URL url = providerUrl.setScopeModel(moduleModel);
+
+ Invoker<?> invoker = mock(Invoker.class);
+ when(invoker.getUrl()).thenReturn(url);
+
+ Class<?> overrideListenerClass = null;
+ for (Class<?> c : RegistryProtocol.class.getDeclaredClasses()) {
+ if ("OverrideListener".equals(c.getSimpleName())) {
+ overrideListenerClass = c;
+ break;
+ }
+ }
+ Assertions.assertNotNull(overrideListenerClass);
+
+ Constructor<?> ctor =
+
overrideListenerClass.getDeclaredConstructor(RegistryProtocol.class, URL.class,
Invoker.class);
+ ctor.setAccessible(true);
+
+ Object listener1 = ctor.newInstance(registryProtocol, url, invoker);
+ Object listener2 = ctor.newInstance(registryProtocol, url, invoker);
+
+ Method method =
+
RegistryProtocol.class.getDeclaredMethod("overrideUrlWithConfig", URL.class,
overrideListenerClass);
+ method.setAccessible(true);
+
+ method.invoke(registryProtocol, url, listener1);
+ method.invoke(registryProtocol, url, listener2);
+
+ Field field =
RegistryProtocol.class.getDeclaredField("serviceConfigurationListeners");
+ field.setAccessible(true);
+
+ @SuppressWarnings("unchecked")
+ Map<String, CopyOnWriteArrayList<?>> map = (Map<String,
CopyOnWriteArrayList<?>>) field.get(registryProtocol);
+
+ CopyOnWriteArrayList<?> listeners = map.get(url.getServiceKey());
+ Assertions.assertEquals(2, listeners.size());
+
+ Field cfgField =
listeners.get(0).getClass().getSuperclass().getDeclaredField("configurators");
+ cfgField.setAccessible(true);
+
+ Configurator c1 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("a", "1");
+ }
+
+ public URL getUrl() {
+ return URL.valueOf("override://0.0.0.0");
+ }
+ };
+ Configurator c2 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("b", "2");
+ }
+
+ public URL getUrl() {
+ return URL.valueOf("override://0.0.0.0");
+ }
+ };
+
+ List<Configurator> l1 = new ArrayList<>();
+ l1.add(c1);
+
+ List<Configurator> l2 = new ArrayList<>();
+ l2.add(c2);
+
+ cfgField.set(listeners.get(0), l1);
+ cfgField.set(listeners.get(1), l2);
+
+ Method agg =
RegistryProtocol.class.getDeclaredMethod("getConfiguredInvokerUrl", List.class,
URL.class);
+ agg.setAccessible(true);
+
+ URL result = url;
+ for (Object l : listeners) {
+ @SuppressWarnings("unchecked")
+ List<Configurator> cs = (List<Configurator>) cfgField.get(l);
+ result = (URL) agg.invoke(null, cs, result);
Review Comment:
The test uses reflection extensively to access private members and inner
classes, which makes it brittle and tightly coupled to implementation details.
Consider refactoring the code to provide package-private test methods or test
utilities that allow testing the aggregation behavior without relying on
reflection. This would make the test more maintainable and less likely to break
with internal refactorings.
```suggestion
URL result = url;
for (Object l : listeners) {
@SuppressWarnings("unchecked")
List<Configurator> cs = (List<Configurator>) cfgField.get(l);
for (Configurator c : cs) {
result = c.configure(result);
}
```
##########
dubbo-registry/dubbo-registry-api/src/test/java/org/apache/dubbo/registry/integration/RegistryProtocolTest.java:
##########
@@ -544,4 +550,104 @@ void testRegisterConsumerUrl() {
verify(registry, times(1)).register(registeredConsumerUrl);
}
+
+ /**
+ * Verifies that multiple ServiceConfigurationListeners registered for the
same
+ * service are preserved and that their configurators are applied
cumulatively.
+ */
+ @Test
+ void testServiceConfigurationListenersAggregation() throws Exception {
+
ApplicationModel.defaultModel().getApplicationConfigManager().setApplication(new
ApplicationConfig("test-app"));
+
+ RegistryProtocol registryProtocol = new RegistryProtocol();
+
+ ModuleModel moduleModel =
ApplicationModel.defaultModel().getDefaultModule();
+
+ Map<String, String> params = new HashMap<>();
+ params.put(INTERFACE_KEY, DemoService.class.getName());
+
+ ServiceConfigURL providerUrl =
+ new ServiceConfigURL("dubbo", "127.0.0.1", 20880,
DemoService.class.getName(), params);
+
+ URL url = providerUrl.setScopeModel(moduleModel);
+
+ Invoker<?> invoker = mock(Invoker.class);
+ when(invoker.getUrl()).thenReturn(url);
+
+ Class<?> overrideListenerClass = null;
+ for (Class<?> c : RegistryProtocol.class.getDeclaredClasses()) {
+ if ("OverrideListener".equals(c.getSimpleName())) {
+ overrideListenerClass = c;
+ break;
+ }
+ }
+ Assertions.assertNotNull(overrideListenerClass);
+
+ Constructor<?> ctor =
+
overrideListenerClass.getDeclaredConstructor(RegistryProtocol.class, URL.class,
Invoker.class);
+ ctor.setAccessible(true);
+
+ Object listener1 = ctor.newInstance(registryProtocol, url, invoker);
+ Object listener2 = ctor.newInstance(registryProtocol, url, invoker);
+
+ Method method =
+
RegistryProtocol.class.getDeclaredMethod("overrideUrlWithConfig", URL.class,
overrideListenerClass);
+ method.setAccessible(true);
+
+ method.invoke(registryProtocol, url, listener1);
+ method.invoke(registryProtocol, url, listener2);
+
+ Field field =
RegistryProtocol.class.getDeclaredField("serviceConfigurationListeners");
+ field.setAccessible(true);
+
+ @SuppressWarnings("unchecked")
+ Map<String, CopyOnWriteArrayList<?>> map = (Map<String,
CopyOnWriteArrayList<?>>) field.get(registryProtocol);
+
+ CopyOnWriteArrayList<?> listeners = map.get(url.getServiceKey());
+ Assertions.assertEquals(2, listeners.size());
+
+ Field cfgField =
listeners.get(0).getClass().getSuperclass().getDeclaredField("configurators");
+ cfgField.setAccessible(true);
+
+ Configurator c1 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("a", "1");
+ }
+
+ public URL getUrl() {
+ return URL.valueOf("override://0.0.0.0");
+ }
+ };
+ Configurator c2 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("b", "2");
+ }
+
+ public URL getUrl() {
Review Comment:
This method overrides [Configurator.getUrl](1); it is advisable to add an
Override annotation.
##########
dubbo-registry/dubbo-registry-api/src/test/java/org/apache/dubbo/registry/integration/RegistryProtocolTest.java:
##########
@@ -544,4 +550,104 @@ void testRegisterConsumerUrl() {
verify(registry, times(1)).register(registeredConsumerUrl);
}
+
+ /**
+ * Verifies that multiple ServiceConfigurationListeners registered for the
same
+ * service are preserved and that their configurators are applied
cumulatively.
+ */
+ @Test
+ void testServiceConfigurationListenersAggregation() throws Exception {
+
ApplicationModel.defaultModel().getApplicationConfigManager().setApplication(new
ApplicationConfig("test-app"));
+
+ RegistryProtocol registryProtocol = new RegistryProtocol();
+
+ ModuleModel moduleModel =
ApplicationModel.defaultModel().getDefaultModule();
+
+ Map<String, String> params = new HashMap<>();
+ params.put(INTERFACE_KEY, DemoService.class.getName());
+
+ ServiceConfigURL providerUrl =
+ new ServiceConfigURL("dubbo", "127.0.0.1", 20880,
DemoService.class.getName(), params);
+
+ URL url = providerUrl.setScopeModel(moduleModel);
+
+ Invoker<?> invoker = mock(Invoker.class);
+ when(invoker.getUrl()).thenReturn(url);
+
+ Class<?> overrideListenerClass = null;
+ for (Class<?> c : RegistryProtocol.class.getDeclaredClasses()) {
+ if ("OverrideListener".equals(c.getSimpleName())) {
+ overrideListenerClass = c;
+ break;
+ }
+ }
+ Assertions.assertNotNull(overrideListenerClass);
+
+ Constructor<?> ctor =
+
overrideListenerClass.getDeclaredConstructor(RegistryProtocol.class, URL.class,
Invoker.class);
+ ctor.setAccessible(true);
+
+ Object listener1 = ctor.newInstance(registryProtocol, url, invoker);
+ Object listener2 = ctor.newInstance(registryProtocol, url, invoker);
+
+ Method method =
+
RegistryProtocol.class.getDeclaredMethod("overrideUrlWithConfig", URL.class,
overrideListenerClass);
+ method.setAccessible(true);
+
+ method.invoke(registryProtocol, url, listener1);
+ method.invoke(registryProtocol, url, listener2);
+
+ Field field =
RegistryProtocol.class.getDeclaredField("serviceConfigurationListeners");
+ field.setAccessible(true);
+
+ @SuppressWarnings("unchecked")
+ Map<String, CopyOnWriteArrayList<?>> map = (Map<String,
CopyOnWriteArrayList<?>>) field.get(registryProtocol);
+
+ CopyOnWriteArrayList<?> listeners = map.get(url.getServiceKey());
+ Assertions.assertEquals(2, listeners.size());
+
+ Field cfgField =
listeners.get(0).getClass().getSuperclass().getDeclaredField("configurators");
+ cfgField.setAccessible(true);
+
+ Configurator c1 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("a", "1");
+ }
+
+ public URL getUrl() {
+ return URL.valueOf("override://0.0.0.0");
+ }
+ };
+ Configurator c2 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("b", "2");
+ }
+
Review Comment:
This method overrides [Configurator.configure](1); it is advisable to add an
Override annotation.
```suggestion
Configurator c1 = new Configurator() {
@Override
public URL configure(URL u) {
return u.addParameter("a", "1");
}
@Override
public URL getUrl() {
return URL.valueOf("override://0.0.0.0");
}
};
Configurator c2 = new Configurator() {
@Override
public URL configure(URL u) {
return u.addParameter("b", "2");
}
@Override
```
##########
dubbo-registry/dubbo-registry-api/src/test/java/org/apache/dubbo/registry/integration/RegistryProtocolTest.java:
##########
@@ -544,4 +550,104 @@ void testRegisterConsumerUrl() {
verify(registry, times(1)).register(registeredConsumerUrl);
}
+
+ /**
+ * Verifies that multiple ServiceConfigurationListeners registered for the
same
+ * service are preserved and that their configurators are applied
cumulatively.
+ */
+ @Test
+ void testServiceConfigurationListenersAggregation() throws Exception {
+
ApplicationModel.defaultModel().getApplicationConfigManager().setApplication(new
ApplicationConfig("test-app"));
+
+ RegistryProtocol registryProtocol = new RegistryProtocol();
+
+ ModuleModel moduleModel =
ApplicationModel.defaultModel().getDefaultModule();
+
+ Map<String, String> params = new HashMap<>();
+ params.put(INTERFACE_KEY, DemoService.class.getName());
+
+ ServiceConfigURL providerUrl =
+ new ServiceConfigURL("dubbo", "127.0.0.1", 20880,
DemoService.class.getName(), params);
+
+ URL url = providerUrl.setScopeModel(moduleModel);
+
+ Invoker<?> invoker = mock(Invoker.class);
+ when(invoker.getUrl()).thenReturn(url);
+
+ Class<?> overrideListenerClass = null;
+ for (Class<?> c : RegistryProtocol.class.getDeclaredClasses()) {
+ if ("OverrideListener".equals(c.getSimpleName())) {
+ overrideListenerClass = c;
+ break;
+ }
+ }
+ Assertions.assertNotNull(overrideListenerClass);
+
+ Constructor<?> ctor =
+
overrideListenerClass.getDeclaredConstructor(RegistryProtocol.class, URL.class,
Invoker.class);
+ ctor.setAccessible(true);
+
+ Object listener1 = ctor.newInstance(registryProtocol, url, invoker);
+ Object listener2 = ctor.newInstance(registryProtocol, url, invoker);
+
+ Method method =
+
RegistryProtocol.class.getDeclaredMethod("overrideUrlWithConfig", URL.class,
overrideListenerClass);
+ method.setAccessible(true);
+
+ method.invoke(registryProtocol, url, listener1);
+ method.invoke(registryProtocol, url, listener2);
+
+ Field field =
RegistryProtocol.class.getDeclaredField("serviceConfigurationListeners");
+ field.setAccessible(true);
+
+ @SuppressWarnings("unchecked")
+ Map<String, CopyOnWriteArrayList<?>> map = (Map<String,
CopyOnWriteArrayList<?>>) field.get(registryProtocol);
+
+ CopyOnWriteArrayList<?> listeners = map.get(url.getServiceKey());
+ Assertions.assertEquals(2, listeners.size());
+
+ Field cfgField =
listeners.get(0).getClass().getSuperclass().getDeclaredField("configurators");
+ cfgField.setAccessible(true);
+
+ Configurator c1 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("a", "1");
+ }
+
+ public URL getUrl() {
+ return URL.valueOf("override://0.0.0.0");
+ }
+ };
+ Configurator c2 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("b", "2");
+ }
+
Review Comment:
This method overrides [Configurator.getUrl](1); it is advisable to add an
Override annotation.
```suggestion
Configurator c1 = new Configurator() {
@Override
public URL configure(URL u) {
return u.addParameter("a", "1");
}
@Override
public URL getUrl() {
return URL.valueOf("override://0.0.0.0");
}
};
Configurator c2 = new Configurator() {
@Override
public URL configure(URL u) {
return u.addParameter("b", "2");
}
@Override
```
##########
dubbo-registry/dubbo-registry-api/src/test/java/org/apache/dubbo/registry/integration/RegistryProtocolTest.java:
##########
@@ -544,4 +550,104 @@ void testRegisterConsumerUrl() {
verify(registry, times(1)).register(registeredConsumerUrl);
}
+
+ /**
+ * Verifies that multiple ServiceConfigurationListeners registered for the
same
+ * service are preserved and that their configurators are applied
cumulatively.
+ */
+ @Test
+ void testServiceConfigurationListenersAggregation() throws Exception {
+
ApplicationModel.defaultModel().getApplicationConfigManager().setApplication(new
ApplicationConfig("test-app"));
+
+ RegistryProtocol registryProtocol = new RegistryProtocol();
+
+ ModuleModel moduleModel =
ApplicationModel.defaultModel().getDefaultModule();
+
+ Map<String, String> params = new HashMap<>();
+ params.put(INTERFACE_KEY, DemoService.class.getName());
+
+ ServiceConfigURL providerUrl =
+ new ServiceConfigURL("dubbo", "127.0.0.1", 20880,
DemoService.class.getName(), params);
+
+ URL url = providerUrl.setScopeModel(moduleModel);
+
+ Invoker<?> invoker = mock(Invoker.class);
+ when(invoker.getUrl()).thenReturn(url);
+
+ Class<?> overrideListenerClass = null;
+ for (Class<?> c : RegistryProtocol.class.getDeclaredClasses()) {
+ if ("OverrideListener".equals(c.getSimpleName())) {
+ overrideListenerClass = c;
+ break;
+ }
+ }
+ Assertions.assertNotNull(overrideListenerClass);
+
+ Constructor<?> ctor =
+
overrideListenerClass.getDeclaredConstructor(RegistryProtocol.class, URL.class,
Invoker.class);
+ ctor.setAccessible(true);
+
+ Object listener1 = ctor.newInstance(registryProtocol, url, invoker);
+ Object listener2 = ctor.newInstance(registryProtocol, url, invoker);
+
+ Method method =
+
RegistryProtocol.class.getDeclaredMethod("overrideUrlWithConfig", URL.class,
overrideListenerClass);
+ method.setAccessible(true);
+
+ method.invoke(registryProtocol, url, listener1);
+ method.invoke(registryProtocol, url, listener2);
+
+ Field field =
RegistryProtocol.class.getDeclaredField("serviceConfigurationListeners");
+ field.setAccessible(true);
+
+ @SuppressWarnings("unchecked")
+ Map<String, CopyOnWriteArrayList<?>> map = (Map<String,
CopyOnWriteArrayList<?>>) field.get(registryProtocol);
+
+ CopyOnWriteArrayList<?> listeners = map.get(url.getServiceKey());
+ Assertions.assertEquals(2, listeners.size());
+
+ Field cfgField =
listeners.get(0).getClass().getSuperclass().getDeclaredField("configurators");
+ cfgField.setAccessible(true);
+
+ Configurator c1 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("a", "1");
+ }
+
+ public URL getUrl() {
+ return URL.valueOf("override://0.0.0.0");
+ }
+ };
+ Configurator c2 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("b", "2");
+ }
+
Review Comment:
This method overrides [Configurator.configure](1); it is advisable to add an
Override annotation.
```suggestion
Configurator c1 = new Configurator() {
@Override
public URL configure(URL u) {
return u.addParameter("a", "1");
}
@Override
public URL getUrl() {
return URL.valueOf("override://0.0.0.0");
}
};
Configurator c2 = new Configurator() {
@Override
public URL configure(URL u) {
return u.addParameter("b", "2");
}
@Override
```
##########
dubbo-registry/dubbo-registry-api/src/test/java/org/apache/dubbo/registry/integration/RegistryProtocolTest.java:
##########
@@ -544,4 +550,104 @@ void testRegisterConsumerUrl() {
verify(registry, times(1)).register(registeredConsumerUrl);
}
+
+ /**
+ * Verifies that multiple ServiceConfigurationListeners registered for the
same
+ * service are preserved and that their configurators are applied
cumulatively.
+ */
+ @Test
+ void testServiceConfigurationListenersAggregation() throws Exception {
+
ApplicationModel.defaultModel().getApplicationConfigManager().setApplication(new
ApplicationConfig("test-app"));
+
+ RegistryProtocol registryProtocol = new RegistryProtocol();
+
+ ModuleModel moduleModel =
ApplicationModel.defaultModel().getDefaultModule();
+
+ Map<String, String> params = new HashMap<>();
+ params.put(INTERFACE_KEY, DemoService.class.getName());
+
+ ServiceConfigURL providerUrl =
+ new ServiceConfigURL("dubbo", "127.0.0.1", 20880,
DemoService.class.getName(), params);
+
+ URL url = providerUrl.setScopeModel(moduleModel);
+
+ Invoker<?> invoker = mock(Invoker.class);
+ when(invoker.getUrl()).thenReturn(url);
+
+ Class<?> overrideListenerClass = null;
+ for (Class<?> c : RegistryProtocol.class.getDeclaredClasses()) {
+ if ("OverrideListener".equals(c.getSimpleName())) {
+ overrideListenerClass = c;
+ break;
+ }
+ }
+ Assertions.assertNotNull(overrideListenerClass);
+
+ Constructor<?> ctor =
+
overrideListenerClass.getDeclaredConstructor(RegistryProtocol.class, URL.class,
Invoker.class);
+ ctor.setAccessible(true);
+
+ Object listener1 = ctor.newInstance(registryProtocol, url, invoker);
+ Object listener2 = ctor.newInstance(registryProtocol, url, invoker);
+
+ Method method =
+
RegistryProtocol.class.getDeclaredMethod("overrideUrlWithConfig", URL.class,
overrideListenerClass);
+ method.setAccessible(true);
+
+ method.invoke(registryProtocol, url, listener1);
+ method.invoke(registryProtocol, url, listener2);
+
+ Field field =
RegistryProtocol.class.getDeclaredField("serviceConfigurationListeners");
+ field.setAccessible(true);
+
+ @SuppressWarnings("unchecked")
+ Map<String, CopyOnWriteArrayList<?>> map = (Map<String,
CopyOnWriteArrayList<?>>) field.get(registryProtocol);
+
+ CopyOnWriteArrayList<?> listeners = map.get(url.getServiceKey());
+ Assertions.assertEquals(2, listeners.size());
+
+ Field cfgField =
listeners.get(0).getClass().getSuperclass().getDeclaredField("configurators");
+ cfgField.setAccessible(true);
+
+ Configurator c1 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("a", "1");
+ }
+
+ public URL getUrl() {
+ return URL.valueOf("override://0.0.0.0");
+ }
+ };
+ Configurator c2 = new Configurator() {
+ public URL configure(URL u) {
+ return u.addParameter("b", "2");
+ }
+
+ public URL getUrl() {
+ return URL.valueOf("override://0.0.0.0");
+ }
+ };
Review Comment:
The test creates anonymous Configurator implementations without properly
implementing the toComparators method that may be required by the Configurator
interface. While this may work for the current test scenario, it's recommended
to either use existing Configurator implementations or create proper test
implementations that fully implement the interface contract to avoid potential
issues if the interface evolves.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
