You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "RaigorJiang (via GitHub)" <gi...@apache.org> on 2026/07/21 08:11:02 UTC
[PR] Fix MySQL prepared statement parameter signedness in Proxy [shardingsphere]
RaigorJiang opened a new pull request, #39204:
URL: https://github.com/apache/shardingsphere/pull/39204
Fixes #39203.
Changes proposed in this pull request:
- Decode MySQL binary prepared statement numeric parameters using the unsigned flag carried by `COM_STMT_EXECUTE`, instead of the `UNSIGNED` flag of the target column metadata.
- Prevent a signed `setInt(-1)` parameter from being decoded as `4294967295` before it reaches an `INT UNSIGNED` backend column.
- Remove `parameterColumnDefinitionFlags` from `MySQLServerPreparedStatement`, because target column flags must not determine client parameter wire decoding.
- Keep `parameterColumnTypes` for the existing BLOB handling path, so string-family parameters targeting BLOB columns continue to preserve raw bytes.
- Add signed and unsigned numeric packet coverage using the same `FF FF FF FF` payload:
- client unsigned flag `0x00` decodes to `-1`;
- client unsigned flag `0x80` decodes to `4294967295L`.
- Keep regression coverage for `STRING`, `VAR_STRING`, and `VARCHAR` parameters targeting `TINYBLOB`, `BLOB`, `MEDIUMBLOB`, and `LONGBLOB` columns.
Manual verification:
- Verified `INT UNSIGNED <- setInt(-1)` through Proxy now forwards `-1` and receives the same `SQLState 22001` out-of-range error as a direct MySQL connection.
- Verified valid unsigned numeric INSERT and UPDATE scenarios through Proxy.
- Verified INSERT and UPDATE byte preservation for `TINYBLOB`, `BLOB`, `MEDIUMBLOB`, `LONGBLOB`, `LONG VARBINARY`, `BINARY`, and `VARBINARY`.
---
Before committing this PR, I'm sure that I have checked the following options:
- [x] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [x] I have self-reviewed the commit code.
- [x] I have (or in comment I request) added corresponding labels for the pull request.
- [x] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
- [x] I have made corresponding changes to the documentation. No user documentation change is required for this bug fix.
- [x] I have added corresponding unit tests for my changes.
- [x] I have updated the Release Notes of the current development version. For more details, see [Update Release Note](https://shardingsphere.apache.org/community/en/involved/contribute/contributor/).
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MySQL prepared statement parameter signedness in Proxy [shardingsphere]
Posted by "RaigorJiang (via GitHub)" <gi...@apache.org>.
RaigorJiang commented on PR #39204:
URL: https://github.com/apache/shardingsphere/pull/39204#issuecomment-5032677017
### Summary
**Review Result: Mergeable**
Reason: The change correctly restores MySQL `COM_STMT_EXECUTE` signedness ownership to the client-supplied parameter type flag, rather than deriving it from the target column metadata returned during `COM_STMT_PREPARE`. The implementation remains narrowly scoped, preserves the prior BLOB byte-preservation path, removes only now-invalid cached target-column flag state, and includes focused regression coverage. This result is based on code scope and local verification only; CI not reviewed by request.
### Evidence
- The [MySQL `COM_STMT_EXECUTE` protocol](https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_com_stmt_execute.html) defines a type-and-flag pair for each newly bound parameter, with the flag byte's high bit representing unsignedness.
- `MySQLComStmtExecutePacket` now decodes numeric values from `MySQLPreparedStatementParameterType#getUnsignedFlag()` at `database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacket.java:127`. This prevents an `INT` parameter carrying `0xFFFFFFFF` with client flag `0x00` from being decoded as unsigned merely because the target column is `INT UNSIGNED`.
- `MySQLComStmtExecuteExecutor` stores the full client parameter type object when type information is supplied and reuses that object for subsequent executions at `proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/execute/MySQLComStmtExecuteExecutor.java:108`. Both the new-type and cached-type paths therefore retain the client unsigned bit.
- `MySQLComStmtPrepareExecutor` still calculates and emits target column flags in the `COM_STMT_PREPARE` response, while no longer retaining those flags as execution-decoding state at `proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/prepare/MySQLComStmtPrepareExecutor.java:141`.
- The packet-level regression tests use the same `MYSQL_TYPE_LONG` payload bytes for signed and unsigned cases, differing only in the parameter flag: `0x00` decodes to `-1`; `0x80` decodes to `4294967295L` at `database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java:72`.
- The existing BLOB regression matrix remains covered for `STRING`, `VAR_STRING`, and `VARCHAR` against `TINYBLOB`, `BLOB`, `MEDIUMBLOB`, and `LONGBLOB` target columns at `database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/execute/MySQLComStmtExecutePacketTest.java:110`.
- Public PR discussion and review endpoints contained no prior review feedback to reconcile.
### Review Details
- Review Focus: Code Correctness Review; CI not reviewed by request.
- Reviewed Scope: PR head `7a74fc1bec2fc96d57d89304e0769367cd5fbd07`, merge base `5702a14ade73a3b5eeb34872af672ef5a9e1f264`; the GitHub `/files` inventory matched the local triple-dot diff:
- `RELEASE-NOTES.md`
- `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`
- `proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/MySQLServerPreparedStatement.java`
- `proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/execute/MySQLComStmtExecuteExecutor.java`
- `proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/prepare/MySQLComStmtPrepareExecutor.java`
- `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/MySQLCommandPacketFactoryTest.java`
- `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/admin/MySQLComResetConnectionExecutorTest.java`
- `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/MySQLComStmtSendLongDataExecutorTest.java`
- `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/execute/MySQLComStmtExecuteExecutorTest.java`
- `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/prepare/MySQLPreparedStatementMetadataFactoryTest.java`
- `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/reset/MySQLComStmtResetExecutorTest.java`
- Not Reviewed Scope: CI/workflow status, full distribution packaging, and external client-to-server integration environments were not reviewed.
- Verification:
- `./mvnw -pl database/protocol/dialect/mysql,proxy/frontend/dialect/mysql -DskipITs -DskipIT -DskipE2E -Dtest=MySQLComStmtExecutePacketTest,MySQLComStmtExecuteExecutorTest,MySQLCommandPacketFactoryTest,MySQLComResetConnectionExecutorTest,MySQLComStmtSendLongDataExecutorTest,MySQLPreparedStatementMetadataFactoryTest,MySQLComStmtResetExecutorTest,MySQLComStmtPrepareExecutorTest -DfailIfNoTests=true -Dsurefire.failIfNoSpecifiedTests=false test -Pcheck -T1C` — exit code `0`; protocol packet tests: 26 passed, Proxy targeted tests: 55 passed.
- `./mvnw spotless:apply -Pcheck -T1C` — exit code `0`.
- `./mvnw checkstyle:check -Pcheck -T1C` — exit code `0`.
- Release Note / User Docs: `RELEASE-NOTES.md` includes an accurate Proxy bug-fix entry linked to `#39204`; no separate user documentation change is needed for this internal protocol-decoding correction.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MySQL prepared statement parameter signedness in Proxy [shardingsphere]
Posted by "FlyingZC (via GitHub)" <gi...@apache.org>.
FlyingZC merged PR #39204:
URL: https://github.com/apache/shardingsphere/pull/39204
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org