RaigorJiang commented on PR #39072:
URL: https://github.com/apache/shardingsphere/pull/39072#issuecomment-4935767549
### Summary
**Review Result: Mergeable**
**Reason:** For code correctness scope, the PR directly fixes the root-cause
path described in #39071 by preserving bytes before string decoding when MySQL
`COM_STMT_EXECUTE` sends a string-like parameter for a BLOB-family target
column. The added tests cover the important protocol boundaries, and focused
local verification passed on the latest PR head. This result is code-scope
only; CI was not reviewed by request.
### Evidence
- The fix is applied at the correct trigger point, before
`MySQLStringLenencBinaryProtocolValue` can decode invalid binary bytes as text.
`MySQLComStmtExecutePacket#readParameters` now delegates non-null values
through `readParameterValue(...)`, and `readParameterValue(...)` uses
`payload.readStringLenencByBytes()` when the client parameter type is `STRING`,
`VAR_STRING`, or `VARCHAR` and the prepared target column type is `TINY_BLOB`,
`BLOB`, `MEDIUM_BLOB`, or `LONG_BLOB`:
`database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacket.java:112`,
`database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacket.java:121`,
`database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacket.java:133`,
`datab
ase/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacket.java:137`.
- The compatibility boundary is preserved. Normal character-column behavior
still flows through `decodeStringParameterValue(...)`, and the existing string
decoding test continues to assert `STRING` bound to `STRING`, `VAR_STRING`, or
`VARCHAR` target columns becomes `String`:
`database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java:142`,
`database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java:161`.
- The regression test covers the full intended matrix for the bug boundary:
`STRING`, `VAR_STRING`, and `VARCHAR` client parameter types against
`TINY_BLOB`, `BLOB`, `MEDIUM_BLOB`, and `LONG_BLOB` target column types, using
invalid UTF-8 bytes `AC ED FF` and asserting the result remains the original
`byte[]`:
`database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java:113`,
`database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java:126`.
- The PR scope is narrow and aligned with #39071: one protocol
implementation file and one focused protocol test file. I did not find
unrelated behavior changes or scope expansion in the reviewed diff.
### Review Details
- **Review Focus:** Code Correctness Review. CI not reviewed by request.
- **Reviewed Scope:** Latest PR head
`a0f4d925eed2bc2580cc52b01d9c0fb4123b4a12`; base `master` at
`94f02c4b8ab3815ad58cb8d046f0c096b835b18c`; local merge-base
`94f02c4b8ab3815ad58cb8d046f0c096b835b18c`. Reviewed files:
-
`database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacket.java`
-
`database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java`
The local triple-dot changed-file list matched GitHub `/pulls/39072/files`.
- **Not Reviewed Scope:** GitHub Actions / CI status and logs were not
reviewed. I also did not run a live MySQL Proxy integration smoke test; the
review is based on public issue evidence, PR diff, protocol-level code tracing,
and focused unit verification.
- **Verification:**
- `./mvnw -pl database/protocol/dialect/mysql -DskipITs -DskipIT -DskipE2E
-DskipDocker -Dtest=MySQLComStmtExecutePacketTest test -Pcheck -T1C` exited `0`
with `BUILD SUCCESS` on the latest PR head.
- `./mvnw -pl database/protocol/dialect/mysql checkstyle:check -Pcheck
-T1C` exited `0` with `BUILD SUCCESS` on the latest PR head.
- **Release Note / User Docs:** Not required for this narrow protocol bugfix
in the PR itself. The linked issue contains the reproduction and user-visible
behavior; release-note aggregation can be handled by the normal release process.
--
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]
