terrymanu commented on PR #40304: URL: https://github.com/apache/shardingsphere/pull/40304#issuecomment-6292801245
### Result Review Result: Mergeable No blocking issues requiring changes were found. The current PR contains only **1 commit and 17 changed files**, consistent with the scope of this Bean Validation fix. This conclusion covers code correctness only; CI was neither queried nor assessed. ### Evidence - The 7 affected validators no longer insert configuration names into message templates. They use fixed messages, default constraint templates, or formatting with fixed property names only. Existing validation conditions, property nodes, and exception wrapping are preserved. [Fix corresponding to alert #63](https://github.com/apache/shardingsphere/blob/6abec8296b6263c5d912a416239cf01420701618/features/sharding/api/src/main/java/org/apache/shardingsphere/sharding/api/config/validator/ShardingTableRuleConfigurationValidator.java#L47) - The diagnostic compatibility changes are explicit: concrete names are omitted, `{reference}` is no longer substituted, and the static `{pool}` attribute still interpolates correctly. MCP diagnostics in the same category may be consolidated by the existing `distinct()` operation. [Reference annotation](https://github.com/apache/shardingsphere/blob/6abec8296b6263c5d912a416239cf01420701618/infra/common/src/main/java/org/apache/shardingsphere/infra/config/rule/validator/constraint/reference/ConfigurationReferenceExists.java#L60) - **All 255 tests across 9 test classes passed**, with no failures, errors, or skipped tests. Coverage includes the actual BVal provider, complete-rule validation, rule-item validation, and YAML entry points. Assertions for SQLState `44000` and error code `10200` are retained. - Results from the official `java/insecure-bean-validation` query decreased from **6 to 0**, including the path corresponding to alert #63. Verification used CodeQL CLI 2.27.2 and java-queries 1.11.12, without custom models or suppressions. - Existing test and CodeQL results were reused after verifying that 535 relevant inputs and 507 extracted source files matched the current candidate. Style checks were executed against the current head during this review. The observed BVal parameter/resource interpolation does not establish exploitable RCE with the default provider. ### Coverage - Public head: `6abec8296b6263c5d912a416239cf01420701618` - Base: `a726e41a5744cdc7a0cc1bc0bea90651722b39e1` - Merge base: `4a291dbab6a974824272b45eb04c13c66b73355e` - GitHub’s complete changed-file list matches the local triple-dot comparison: 17 files and 64 changed blocks. The public head and file contents were checked again before completion and remained unchanged. The worktree is clean, with no additional local changes. - The review assessed the public PR’s message-template security issue, existing configuration-validation contracts, and the established diagnostic-message boundaries. Responsibility remains with the existing validators; no new API/SPI, dependency, or project support commitment is introduced. - Coverage includes sharding schemas, sharding references, encryption-column conflicts, shadow references, shared reference/duplicate-name validation and its mask/readwrite-splitting consumers, and MCP configuration. Root-cause, impact, test, and runtime-behavior checks and a full-scope convergence pass were completed, with no unresolved evidence gaps. - Two rounds, each involving two independent reviewers, were completed. The reports underwent substantive review and counter-evidence checks, and the complete conclusions agreed across both rounds. Session validation exited with code 0. - CodeQL coverage includes `infra/common`, `features/sharding/api`, `features/encrypt/api`, `features/shadow/api`, `mcp/support`, and `mcp/bootstrap`, comprising 507 main Java source files. This was not a repository-wide scan. - This review did not modify source code, create commits, or push changes. Both Checkstyle and Spotless covered all 17 files listed below. Both goals ran in the same invocation, which exited with code **0**. Spotless selected 17 files and skipped none through caching. Checkstyle reported 0 error-level violations and 3 non-blocking warnings, consistent with the repository’s configured failure threshold. The sanitized command summary is shown below. Both filter parameters correspond exactly to the files listed: ./mvnw -o -Dspotless.upToDateChecking.enabled=false -pl infra/common,features/sharding/api,features/encrypt/api,features/shadow/api,features/mask/api,features/readwrite-splitting/api,mcp/bootstrap -DspotlessFiles="<17 exact path regular expressions>" -Dcheckstyle.includes="<17 source-root-relative paths>" -Dcheckstyle.includeResources=false -Dcheckstyle.includeTestResources=false spotless:check checkstyle:check -Pcheck -T1C Files covered by the complete review and both style checks: - `features/encrypt/api/src/main/java/org/apache/shardingsphere/encrypt/config/validator/EncryptTableRuleConfigurationValidator.java` - `features/encrypt/api/src/test/java/org/apache/shardingsphere/encrypt/config/EncryptRuleConfigurationTest.java` - `features/mask/api/src/test/java/org/apache/shardingsphere/mask/config/MaskRuleConfigurationTest.java` - `features/readwrite-splitting/api/src/test/java/org/apache/shardingsphere/readwritesplitting/config/ReadwriteSplittingRuleConfigurationTest.java` - `features/shadow/api/src/main/java/org/apache/shardingsphere/shadow/config/validator/ShadowRuleConfigurationValidator.java` - `features/shadow/api/src/test/java/org/apache/shardingsphere/shadow/config/ShadowRuleConfigurationTest.java` - `features/sharding/api/src/main/java/org/apache/shardingsphere/sharding/api/config/validator/ShardingRuleConfigurationValidator.java` - `features/sharding/api/src/main/java/org/apache/shardingsphere/sharding/api/config/validator/ShardingTableRuleConfigurationValidator.java` - `features/sharding/api/src/test/java/org/apache/shardingsphere/sharding/api/config/ShardingRuleConfigurationTest.java` - `features/sharding/api/src/test/java/org/apache/shardingsphere/sharding/api/config/validator/ShardingTableRuleConfigurationValidatorTest.java` - `infra/common/src/main/java/org/apache/shardingsphere/infra/config/rule/validator/constraint/reference/ConfigurationReferenceExists.java` - `infra/common/src/main/java/org/apache/shardingsphere/infra/config/rule/validator/constraint/reference/ConfigurationReferenceExistsValidator.java` - `infra/common/src/main/java/org/apache/shardingsphere/infra/config/rule/validator/constraint/unique/UniqueTableNamesValidator.java` - `infra/common/src/test/java/org/apache/shardingsphere/infra/config/rule/validator/constraint/reference/ConfigurationReferenceExistsValidatorTest.java` - `infra/common/src/test/java/org/apache/shardingsphere/infra/config/rule/validator/constraint/unique/UniqueTableNamesValidatorTest.java` - `mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/config/yaml/validator/YamlRuntimeDatabaseConfigurationsValidator.java` - `mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/config/yaml/swapper/YamlMCPLaunchConfigurationSwapperTest.java` -- 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]
