strongduanmu commented on PR #39694:
URL: https://github.com/apache/shardingsphere/pull/39694#issuecomment-5506871051
• ### Result
**Review Result: Mergeable**
The current code scope satisfies correctness, compatibility, and
regression requirements. The hexadecimal codec replacement preserves existing
letter-case semantics, handles invalid input explicitly, and keeps the MD5
UTF-8 conversion behavior equivalent. No
blocking issues were found.
### Evidence
- The latest public PR head is
[`e3900a50b00`](https://github.com/apache/shardingsphere/pull/39694/commits/e3900a50b00ef14b8ecbcdfbc0dfaa986a59916c).
The GitHub file list matches the local triple-dot scope, covering six changed
files.
- `EncryptAlgorithmEngine` preserves lowercase output for MD5 and
uppercase output for other algorithms, while converting invalid HEX input from
`DecoderException` to `IllegalArgumentException`:
[EncryptAlgorithmEngine.java:130](https://github.com/apache/shardingsphere/blob/e3900a50b00ef14b8ecbcdfbc0dfaa986a59916c/features/encrypt/core/src/main/java/org/apache/shardingsphere/encrypt/algorithm/engine/EncryptAlgorithmEngine.java#L130-L139).
- The official Commons Codec 1.16.0 implementation confirms that `true`
produces lowercase output, `false` produces uppercase output, and odd-length or
illegal input raises `DecoderException`:
[Hex.java:95](https://github.com/apache/commons-codec/blob/rel/commons-codec-1.16.0/src/main/java/org/apache/commons/codec/binary/Hex.java#L95-L130),
[Hex.java:151](https://github.com/apache/commons-codec/blob/rel/commons-codec-1.16.0/src/main/java/org/apache/commons/codec/binary/Hex.java#L151-L156).
- The tests cover `0x00`, nibble boundaries, `0xFF`, mixed-case decoding,
odd-length input, and illegal characters:
[EncryptAlgorithmEngineTest.java:52](https://github.com/apache/shardingsphere/blob/e3900a50b00ef14b8ecbcdfbc0dfaa986a59916c/features/encrypt/core/src/test/java/org/apache/shardingsphere/encrypt/algorithm/engine/EncryptAlgorithmEngineTest.java#L52-L57),
[EncryptAlgorithmEngineTest.java:106](https://github.com/apache/shardingsphere/blob/e3900a50b00ef14b8ecbcdfbc0dfaa986a59916c/features/encrypt/core/src/test/java/org/apache/shardingsphere/encrypt/algorithm/engine/EncryptAlgorithmEngineTest.java#L106-L121).
- `MessageDigestAlgorithmEngine` preserves lowercase HEX output. The root
POM manages the new dependency as `commons-codec:1.16.0`, and the distribution
LICENSE and NOTICE files already cover this library.
- `MD5MessageDigestEngine` uses `convertToBytes(..., null)`, whose
existing contract selects UTF-8. This is equivalent to the previous
`getBytes(StandardCharsets.UTF_8)` behavior.
- Focused verification covered Encrypt, AES, MD5 Assisted Encrypt,
MessageDigest, MD5 Engine, and MD5 Mask, totaling 42 test scenarios with exit
code `0`.
- Spotless passed for all affected modules with exit code `0`; Checkstyle
reported zero violations with exit code `0`.
- The Maven dependency tree confirmed
`commons-codec:commons-codec:1.16.0:compile` with exit code `0`.
### Coverage
- Candidate type: latest public PR head.
- Scope: all six files from GitHub’s authoritative changed-file list,
including three production Java files, two test files, and one POM.
- Behavior clusters: Encrypt HEX encoding and decoding, MessageDigest HEX
encoding and dependency management, and MD5 UTF-8 byte conversion.
- Completed review lenses: Root Cause and Behavior, Blast Radius and
Contracts, Tests Runtime and Operations, followed by a full-scope convergence
pass.
- Authoritative requirement: the Encrypt and MessageDigest refactoring
described by the PR; no valid linked issue was identified.
- Unresolved evidence gaps: none.
- This result covers code correctness only; CI and GitHub Actions were not
reviewed.
--
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]
