terrymanu commented on PR #38991:
URL: https://github.com/apache/shardingsphere/pull/38991#issuecomment-4881524231
### Summary
**Review Result: Mergeable**
Reason: This is a code-scope review only; CI was not reviewed by request.
The latest PR head only reuses already-resolved `actualRule` values and masked
rule payloads in encrypt and mask validation paths, without changing branch
conditions, mismatch construction semantics, or returned validation statuses. I
found no code-level blocker in the reviewed scope.
### Evidence
-
`mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptWorkflowValidationService.java:171-205`
now reuses `maskedActualRule` and `actualRuleValue` after `actualRule` has
already been proven present. The previous repeated `actualRule.get()` and
`createMaskedRules(...).getFirst()` calls are replaced with equivalent local
values.
-
`mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/service/MaskWorkflowValidationService.java:138-157`
applies the same local reuse pattern for the drop, mismatch, and passed
validation paths.
- `createMaskedRules(...)` in both classes constructs new `LinkedHashMap`
values from the supplied rule and masks properties before returning the result,
so reusing the single masked map for the same validation branch preserves the
same visible payload while avoiding duplicate transformation work.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR `apache/shardingsphere#38991`, latest head
`e29655dfb43d969cf92a69a7973731ca1e032d6a`, base `master` at
`7a03f99acdc277dd809665699e296072e5e9176d`, merge-base
`7a03f99acdc277dd809665699e296072e5e9176d`. Local triple-dot file list matched
GitHub `/pulls/38991/files`.
- Reviewed Files:
-
`mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptWorkflowValidationService.java`
-
`mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/service/MaskWorkflowValidationService.java`
- Not Reviewed Scope: GitHub Actions/check-runs, full repository test suite,
runtime MCP E2E behavior, and modules outside the two changed files.
- Verification:
- `./mvnw -pl mcp/features/encrypt,mcp/features/mask
-Dtest=EncryptWorkflowValidationServiceTest,MaskWorkflowValidationServiceTest
-DfailIfNoTests=false -Dsurefire.failIfNoSpecifiedTests=false test -Pcheck
-T1C` passed with exit code 0.
- `./mvnw checkstyle:check -Pcheck -T1C` passed with exit code 0.
- `./mvnw spotless:check -Pcheck -T1C` passed with exit code 0.
- Release Note / User Docs: Not required. The PR is an internal
behavior-preserving validation readability refactor and does not change
user-facing MCP behavior, configuration, diagnostics, or compatibility.
--
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]
