jiangML commented on PR #38713: URL: https://github.com/apache/shardingsphere/pull/38713#issuecomment-4519584772
1. Decision Block Merge Verdict: Mergeable Reviewed Scope: features/encrypt/distsql/handler/src/main/java/.../DropEncryptRuleExecutor.java features/encrypt/distsql/handler/src/test/java/.../DropEncryptRuleExecutorTest.java Full latest-head diff of master...pr-38713 (only 2 files changed, +24 lines) Not Reviewed Scope: Runtime behavior of other untouched encrypt modules (only impact analysis was done, not deep file-by-file review) Need Expert Review: No (no concurrency protocol changes, no SPI design changes, no SQL parser semantic changes, no security-sensitive logic) 2. Basis Root cause is fixed correctly: The original issue was that buildToBeDroppedRuleConfiguration directly used the SQL input name each to construct EncryptTableRuleConfiguration, which could lose the original case from existing config and cause mismatch when deleting persisted nodes by name. The new logic first locates the real table config by equalsIgnoreCase, then uses tableRuleConfig.get().getName() (original casing) to build the to-be-dropped config, and uses the same name for dropRule. This fixes the trigger point directly, not via fallback behavior. Regression validation is sufficient and one-to-one: Added test assertBuildToBeDroppedRuleConfigurationUsesOriginalCaseTableName, with t_Encrypt + DROP ... T_ENCRYPT, asserting output keeps t_Encrypt. This is a counterexample validation for case-insensitive input vs case-preserving output, exactly covering the semantic compatibility risk. Adversarial risk scan: Cross-feature/cross-dialect regression path: change is limited to encrypt DistSQL drop executor; no shared binder/router/parser path is touched; blast radius is small. Config-disabled / if exists path: if exists semantics are unchanged; when table is not found, nothing is added to to-be-dropped, which is expected. “Original symptom only partially covered” risk: new test covers the core case issue; existing tests still cover normal drop and in-use encryptor scenarios. No unresolved branch risk found. 3. Pre-merge checks Latest PR head was verified locally: pr-38713 (same commit as current fix-master: 4ef1a3afc28). Local verification (including dependency chain): ./mvnw -pl features/encrypt/distsql/handler spotless:apply -Pcheck -T1C ✅ ./mvnw -pl features/encrypt/distsql/handler checkstyle:check -Pcheck -T1C ✅ ./mvnw -pl features/encrypt/distsql/handler -am -DskipITs -Dspotless.skip=true -Dmockito.mock-maker=subclass -Dtest=DropEncryptRuleExecutorTest#assertBuildToBeDroppedRuleConfigurationUsesOriginalCaseTableName -Dsurefire.failIfNoSpecifiedTests=false test ✅ (89-module dependency-chain build/test succeeded) Pre-merge recommendation: merge after GitHub CI is green. -- 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]
