Gimini-3 opened a new issue, #39296: URL: https://github.com/apache/shardingsphere/issues/39296
## Bug Report ### Which version of ShardingSphere did you use? Current `master` at commit [c1b22fb063cc6d683502c9bef48e95b2ae6a8d39](https://github.com/apache/shardingsphere/commit/c1b22fb063cc6d683502c9bef48e95b2ae6a8d39). ### Which project did you use? ShardingSphere-JDBC or ShardingSphere-Proxy? ShardingSphere-JDBC with YAML / Java API readwrite-splitting rule configuration. The problem is in the shared rule configuration and load-balancer code. The DistSQL CREATE / ALTER path is not affected because it performs an additional bilateral property check. ### Expected behavior A WEIGHT load balancer should reject the rule configuration during validation when any configured read data source does not have a corresponding weight property. For example, if the configured read data sources are `read_ds_0` and `read_ds_1`, both names should be present in the WEIGHT properties. The following incomplete configuration should fail with an `AlgorithmInitializationException` before the rule can be used: ```yaml dataSources: readwrite_ds: writeDataSourceName: write_ds readDataSourceNames: - read_ds_0 - read_ds_1 loadBalancerName: weight loadBalancers: weight: type: WEIGHT props: read_ds_0: 1 ``` This also matches the documented WEIGHT property contract: the property name is the replica name and the property value is its corresponding weight. https://shardingsphere.apache.org/document/current/en/user-manual/common-config/builtin-algorithm/load-balance/ ### Actual behavior The configuration passes `WeightLoadBalanceAlgorithm.check()`. When a read route is evaluated, the algorithm iterates over both configured read data sources. Looking up the missing weight for `read_ds_1` returns `null`, and assigning that value to primitive `double` triggers a `NullPointerException`. The observed result is: ```text validation=passed Exception in thread "main" java.lang.NullPointerException: Cannot invoke "java.lang.Double.doubleValue()" because the return value of "java.util.Map.get(Object)" is null at org.apache.shardingsphere.infra.algorithm.loadbalancer.weight.WeightLoadBalanceAlgorithm.getWeightValue(WeightLoadBalanceAlgorithm.java:126) at org.apache.shardingsphere.infra.algorithm.loadbalancer.weight.WeightLoadBalanceAlgorithm.getWeights(WeightLoadBalanceAlgorithm.java:100) at org.apache.shardingsphere.infra.algorithm.loadbalancer.weight.WeightLoadBalanceAlgorithm.initWeight(WeightLoadBalanceAlgorithm.java:88) at org.apache.shardingsphere.infra.algorithm.loadbalancer.weight.WeightLoadBalanceAlgorithm.getTargetName(WeightLoadBalanceAlgorithm.java:70) ``` No physical database connection is required to reproduce the failure because it happens while selecting the read target. ### Reason analyze (If you can) `WeightLoadBalanceAlgorithm.check()` currently validates only one direction: ```java @Override public void check(final String databaseName, final Collection<String> configuredTargetNames) { weightConfigMap.keySet().forEach(each -> ShardingSpherePreconditions.checkContains(configuredTargetNames, each, () -> new AlgorithmInitializationException(this, "Target `%s` is required in database `%s`.", each, databaseName))); } ``` Source: https://github.com/apache/shardingsphere/blob/c1b22fb063cc6d683502c9bef48e95b2ae6a8d39/infra/algorithm/type/load-balancer/type/weight/src/main/java/org/apache/shardingsphere/infra/algorithm/loadbalancer/weight/WeightLoadBalanceAlgorithm.java#L61-L65 It verifies: ```text weight property keys ⊆ configured read target names ``` It does not verify the reverse requirement: ```text configured read target names ⊆ weight property keys ``` Therefore, `{read_ds_0=1}` is accepted for configured targets `[read_ds_0, read_ds_1]`, because the only property key, `read_ds_0`, is a valid target. The normal rule configuration checker delegates to this incomplete check: ```java loadBalancer.check(databaseName, each.getReadDataSourceNames()); ``` Source: https://github.com/apache/shardingsphere/blob/c1b22fb063cc6d683502c9bef48e95b2ae6a8d39/features/readwrite-splitting/core/src/main/java/org/apache/shardingsphere/readwritesplitting/checker/ReadwriteSplittingRuleConfigurationChecker.java#L58-L69 Later, read routing calls `getTargetName()` with all available read targets. `getWeights()` invokes `getWeightValue()` for each one: ```java private double getWeightValue(final String readDataSourceName) { double result = weightConfigMap.get(readDataSourceName); // ... } ``` Source: https://github.com/apache/shardingsphere/blob/c1b22fb063cc6d683502c9bef48e95b2ae6a8d39/infra/algorithm/type/load-balancer/type/weight/src/main/java/org/apache/shardingsphere/infra/algorithm/loadbalancer/weight/WeightLoadBalanceAlgorithm.java#L96-L135 The DistSQL checker already validates both directions: ```java weightKeys.forEach(each -> ShardingSpherePreconditions.checkContains(ruleSegment.getReadDataSources(), each, ...)); ruleSegment.getReadDataSources().forEach(each -> ShardingSpherePreconditions.checkContains(weightKeys, each, ...)); ``` Source: https://github.com/apache/shardingsphere/blob/c1b22fb063cc6d683502c9bef48e95b2ae6a8d39/features/readwrite-splitting/distsql/handler/src/main/java/org/apache/shardingsphere/readwritesplitting/distsql/handler/checker/ReadwriteSplittingRuleStatementChecker.java#L225-L237 This creates inconsistent validation behavior between YAML / Java rule configuration and DistSQL. Related issues and PRs were checked: - #33248 / #33249 validate the opposite case in the DistSQL path: a WEIGHT property references a read data source that is not configured. - #34139 / #34140 improve DistSQL validation and add the reverse missing-weight check there, but do not update `WeightLoadBalanceAlgorithm.check()`. - #21602 fixes reuse of a cached weight array after the available target list size changes; it does not validate missing WEIGHT properties. - #33189 adds tests for an extra invalid property key, but does not cover a configured target whose weight is missing. ### Steps to reproduce the behavior, such as: SQL to execute, sharding rule configuration, when exception occur etc. 1. Build the current WEIGHT load-balancer module. 2. Instantiate the public WEIGHT SPI with only the weight for `read_ds_0`. 3. Validate it against two configured targets. 4. Request a target name using the same two targets. ```java import java.util.Arrays; import java.util.Properties; import org.apache.shardingsphere.infra.algorithm.loadbalancer.spi.LoadBalanceAlgorithm; import org.apache.shardingsphere.infra.spi.type.typed.TypedSPILoader; public final class WeightLoadBalanceReproducer { public static void main(final String[] args) { Properties props = new Properties(); props.setProperty("read_ds_0", "1"); LoadBalanceAlgorithm algorithm = TypedSPILoader.getService(LoadBalanceAlgorithm.class, "WEIGHT", props); algorithm.check("test_db", Arrays.asList("read_ds_0", "read_ds_1")); System.out.println("validation=passed"); algorithm.getTargetName("readwrite_ds", Arrays.asList("read_ds_0", "read_ds_1")); } } ``` `algorithm.check(...)` returns normally, then `algorithm.getTargetName(...)` deterministically throws the NPE shown above. ### Example codes for reproduce this issue (such as a github link). The reproducer uses only public APIs and the current module implementation. The existing unit test also shows the coverage gap: https://github.com/apache/shardingsphere/blob/c1b22fb063cc6d683502c9bef48e95b2ae6a8d39/infra/algorithm/type/load-balancer/type/weight/src/test/java/org/apache/shardingsphere/infra/algorithm/loadbalancer/weight/WeightLoadBalanceAlgorithmTest.java#L44-L47 The existing test verifies that an extra WEIGHT key is rejected, but there is no test for a missing weight. A focused regression test should configure `read_ds_0=1`, pass `[read_ds_0, read_ds_1]` to `check()`, and assert `AlgorithmInitializationException`. The smallest fix should add the reverse membership validation to `WeightLoadBalanceAlgorithm.check()`, so invalid configuration fails during rule validation rather than adding a fallback value or handling `null` in the high-frequency routing path. -- 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]
