Copilot commented on code in PR #16382: URL: https://github.com/apache/dubbo/pull/16382#discussion_r3600173432
########## dubbo-common/src/test/java/org/apache/dubbo/config/context/AbstractConfigManagerTest.java: ########## @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.config.context; + +import org.apache.dubbo.config.RegistryConfig; +import org.apache.dubbo.config.context.ConfigManagerTest.TestPreferSerializationProvider; +import org.apache.dubbo.rpc.model.ApplicationModel; +import org.apache.dubbo.rpc.model.FrameworkModel; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +class AbstractConfigManagerTest { + + private ConfigManager configManager; + private ModuleConfigManager moduleConfigManager; + + @BeforeEach + public void init() { + ApplicationModel.defaultModel().destroy(); + ApplicationModel applicationModel = ApplicationModel.defaultModel(); + configManager = applicationModel.getApplicationConfigManager(); + moduleConfigManager = applicationModel.getDefaultModule().getConfigManager(); + FrameworkModel.defaultModel().getBeanFactory().registerBean(TestPreferSerializationProvider.class); + } Review Comment: This test setup has a few avoidable couplings/side effects: - `moduleConfigManager` is declared and initialized but never used. - The test imports `ConfigManagerTest.TestPreferSerializationProvider` and registers it in the global `FrameworkModel` bean factory, even though this test only exercises `addConfig/getConfig` and doesn't need that provider. Importing from another test class and mutating global state makes the test more brittle. Recommend removing the unused field/assignment and dropping the cross-test import + unnecessary bean registration. ########## dubbo-common/src/main/java/org/apache/dubbo/config/context/AbstractConfigManager.java: ########## @@ -211,10 +213,12 @@ private <C extends AbstractConfig> C addIfAbsent( return config; } - // find by value - Optional<C> prevConfig = findDuplicatedConfig(configsMap, config); - if (prevConfig.isPresent()) { - return prevConfig.get(); + if (!(config instanceof RegistryConfig)) { + // find by value + Optional<C> prevConfig = findDuplicatedConfig(configsMap, config); + if (prevConfig.isPresent()) { + return prevConfig.get(); + } } Review Comment: The new condition skips value-based deduplication for *all* RegistryConfig instances. That fixes the reported case where two registries have different explicit ids but equal content, but it also changes behavior for registries without an explicit id: previously identical registry configs would be deduped (since AbstractConfig.equals ignores getId), now they will both be added and receive different generated ids (registry#1, registry#2). This can unexpectedly introduce multiple equivalent registries and affect default-registry selection. Consider only bypassing the duplicated-config check for RegistryConfig when an explicit id is provided, while keeping the previous dedup behavior when id is null (auto-generated). -- 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]
