yx9o commented on PR #39604: URL: https://github.com/apache/shardingsphere/pull/39604#issuecomment-5447183391
### Result **Review Result: Mergeable** No code-correctness blocker was found. The PR switches encrypt insert-value parameter rewriting to use the current `ShardingSphereDatabase` as the default-schema owner, aligning it with the insert-values SQL token generation path, and adds focused coverage for the schema-less insert case. ### Evidence - Reviewed PR [#39604](https://github.com/apache/shardingsphere/pull/39604) latest head [`9eee65ef461ad83cd27a4c9e1a806018aac0a9b9`](https://github.com/apache/shardingsphere/commit/9eee65ef461ad83cd27a4c9e1a806018aac0a9b9), base `master` `d67a25dd98361c26d3f2526341eaa46e6e96d2d5`. - `features/encrypt/core/src/main/java/org/apache/shardingsphere/encrypt/rewrite/parameter/EncryptParameterRewritersRegistry.java:53` now passes `sqlRewriteContext.getDatabase()` to `EncryptInsertValueParameterRewriter`; the same PR-head registry passes the database object to the adjacent assignment and on-duplicate rewriters at `:50` and `:54`. - `features/encrypt/core/src/main/java/org/apache/shardingsphere/encrypt/rewrite/parameter/rewriter/EncryptInsertValueParameterRewriter.java:49` stores `ShardingSphereDatabase`, and `:66-67` resolves schema-less inserts with `database::getDefaultSchemaName`. - `features/encrypt/core/src/main/java/org/apache/shardingsphere/encrypt/rewrite/parameter/rewriter/EncryptInsertValueParameterRewriter.java:109`, `:112`, and `:115` still pass `database.getName()` into cipher, assisted-query, and like-query encryption, preserving the encrypt algorithm database-name argument. - `infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/database/ShardingSphereDatabase.java:308-315` owns protocol-aware default schema resolution; `database/connector/core/src/main/java/org/apache/shardingsphere/database/connector/core/type/DatabaseTypeRegistry.java:71-74` marks the old default-schema API as deprecated in favor of it. - `features/encrypt/core/src/main/java/org/apache/shardingsphere/encrypt/rewrite/token/generator/insert/EncryptInsertValuesTokenGenerator.java:108-122` already uses `database::getDefaultSchemaName`, so the parameter rewriter now matches the token generator path. - `features/encrypt/core/src/test/java/org/apache/shardingsphere/encrypt/rewrite/parameter/rewriter/EncryptInsertValueParameterRewriterTest.java:50-60` covers schema-less grouped insert parameter replacement; `:80` makes `TablesContext#getSchemaName()` empty, and the expected cipher call at `:55` verifies `foo_default_schema` is passed to the encrypt algorithm. ### Coverage - Authoritative GitHub changed-file list accounted for 3 files: `EncryptParameterRewritersRegistry.java`, `EncryptInsertValueParameterRewriter.java`, and added `EncryptInsertValueParameterRewriterTest.java`. - Public issue comments contained one previous mergeable review for older head `c03d17b0`; it was treated only as prior public context, and latest head `9eee65ef` was reviewed independently. Review comments and reviews endpoints returned empty. - Behavior clusters reviewed: insert-value parameter default-schema resolution, registry construction compatibility, grouped parameter replacement, encrypt algorithm argument preservation, and focused test validity. - Discovery lenses completed: Root Cause and Behavior; Blast Radius and Contracts; Tests, Runtime, and Operations. Triggered high-risk lenses checked: shared metadata ownership, test validity, high-frequency rewrite path, and user/runtime impact. - Code-scope only; CI was not reviewed. Local Maven tests were not run because the PR head is not checked out locally. -- 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]
