Fix incorrect generated key handling for explicit auto-increment values

[somiljain2006]

Fixes #38717

Changes proposed in this pull request:

When an INSERT specifies an explicit value for an auto-increment column, Proxy should not populate lastInsertId from that value. This aligns the behavior with MySQL and prevents incorrectly generated key responses for explicit values. Add regression test covering explicit negative auto-increment values.

---

Before committing this PR, I'm sure that I have checked the following options:

• My code follows the code of conduct of this project.
• I have self-reviewed the commit code.
• I have (or in the comment I request) added corresponding labels for the pull request.
• I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven. javadoc.skip -Dmaven. jacoco.skip -e.
• I have made corresponding changes to the documentation.
• I have added corresponding unit tests for my changes.
• I have updated the Release Notes of the current development version. For more details, see Update Release Note

[somiljain2006]

@terrymanu Can you review this pr?

[terrymanu]

Please resolve conflicts first.

[terrymanu]

Summary

• Merge Decision: Not Mergeable
• Reason: The patch filters generated values only after execution, while explicit auto-increment inserts can still request and read backend generated keys before that filter.

Positive Feedback

• Using GeneratedKeyContext.isGenerated() is the right direction for separating generated values from client-supplied values, and the PR includes a release-note entry.

Issues

• P1 Root-cause path still requests backend generated keys for explicit values (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java:189)
  • Problem: Normal Proxy execution still sets isReturnGeneratedKeys for every InsertStatement when the dialect has a generated-key option, without checking InsertStatementContext.getGeneratedKeyContext().isGenerated(). That flag is passed into StatementOption and can still reach statement.getGeneratedKeys() / resultSet.getLong(1) at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/executor/callback/ProxyJDBCExecutorCallback.java:75 and proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/executor/callback/ProxyJDBCExecutorCallback.java:86. The same unconditional flag exists on the federation path at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:264. The new .filter(GeneratedKeyContext::isGenerated) at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:334 is too late to prevent the linked issue's backend generated-key read failure, and it also cannot suppress a positive explicit value already carried in UpdateResult.lastInsertId, which UpdateResponseHeader keeps at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/response/header/update/UpdateResponseHeader.java:67.
  • Impact: The reported Proxy/MySQL rule-managed insert with explicit -3 can still fail before UpdateResponseHeader is built, and an adjacent explicit positive auto-increment value can still be returned as lastInsertId. This leaves issue #38717 only partially fixed.
  • Required Change: Please gate RETURN_GENERATED_KEYS and the generated-key callback path with the same explicit/generated distinction, for both normal and federation execution paths if both are reachable. Please also add regression coverage that does not mock away ProxyJDBCExecutorCallback: verify explicit auto-increment values use NO_GENERATED_KEYS or otherwise never call getGeneratedKeys(), and keep a generated-value case to prove real generated keys still work.

Review Details

• Reviewed Scope: Latest PR head 97cca573cb323987908b6620cd155acf0731c9fc; GitHub base SHA 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; local merge-base 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; local triple-dot file list matched GitHub /pulls/38810/files. Reviewed RELEASE-NOTES.md, proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java, proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnectorTest.java, and the related generated-key execution / OK-packet path. Shared-path blast radius checked for MySQL and MariaDB generated-key metadata; other dialect metadata defaults to no generated-key option.
• Not Reviewed Scope: CI/check statuses and full live Proxy + MySQL reproduction were not reviewed; no code changes were made.
• Verification: git fetch apache master:refs/remotes/apache/master pull/38810/head:refs/remotes/apache/pr/38810 exit 0; git diff --name-status refs/remotes/apache/master...refs/remotes/apache/pr/38810 exit 0 and matched GitHub files; static git show / git grep inspection exit 0. Maven tests were not run because the code-path blocker is visible and the added unit test bypasses the failing callback path.
• Release Note / User Docs: Release note is present in RELEASE-NOTES.md:29; user docs are not required for this internal Proxy bug fix, but the release-note claim should be kept only after the root-cause fix is complete.

[somiljain2006]

Thanks for the detailed review.

I've updated the fix to address the generated-key retrieval path before execution rather than only filtering values in UpdateResponseHeader.

Changes made:

• Gated RETURN_GENERATED_KEYS using GeneratedKeyContext.isGenerated() in ProxySQLExecutor, so explicit auto-increment values no longer request generated keys from the backend.
• Applied the same generated/explicit distinction to the federation execution path.
• Kept the response-side filtering as a safeguard to avoid propagating explicit values as generated keys.
• Added regression coverage in ProxySQLExecutorTest to verify the generated-key decision is propagated through StatementOption.
• Added callback-level regression tests in ProxyJDBCExecutorCallbackTest to verify:
  • explicit auto-increment values do not invoke Statement#getGeneratedKeys()
  • genuine generated-key inserts still invoke Statement#getGeneratedKeys()

All related tests are passing locally, including:

• StandardDatabaseProxyConnectorTest
• ProxySQLExecutorTest
• ProxyJDBCExecutorCallbackTest

[terrymanu]

Summary

• Merge Decision: Not Mergeable
• Reason: The latest patch fixes the original explicit -3 path too broadly and now suppresses generated keys for MySQL AUTO_INCREMENT values that are still generated when the key column is explicitly present as NULL or 0.

Positive Feedback

• The latest head is in the right direction compared with the previous round: it now gates StatementOption, the normal Proxy callback path, the federation path, and the final UpdateResponseHeader value propagation instead of only filtering after execution.

Issues

• P1 Explicit NULL/0 auto-increment inserts lose generated-key handling (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java:266, proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:431)
  • Problem: The new gate returns generated keys only when GeneratedKeyContext.isGenerated() is true, but GeneratedKeyContextEngine sets generated=false whenever the auto-increment column is present in the INSERT list (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/insert/keygen/engine/GeneratedKeyContextEngine.java:59 and infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/insert/keygen/engine/GeneratedKeyContextEngine.java:103). That correctly covers explicit nonspecial values such as -3, but it also covers INSERT INTO t(id, ...) VALUES (NULL, ...) and 0 under normal MySQL mode, where MySQL still generates an AUTO_INCREMENT value. The MySQL 8.4 C API documentation distinguishes explicit nonspecial values from NULL/0 values that generate auto-increment IDs.
  • Impact: Valid Proxy MySQL/MariaDB inserts that intentionally request database-side generation with an explicit NULL or 0 key column will now use Statement.NO_GENERATED_KEYS / prepareStatement(sql) and ProxyJDBCExecutorCallback will skip getGeneratedKeys() at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/executor/callback/ProxyJDBCExecutorCallback.java:75. That regresses last_insert_id / generated-key response behavior for an adjacent valid input while fixing issue #38717.
  • Required Change: Please distinguish explicit nonspecial client-supplied values from explicit special values that still ask MySQL/MariaDB to generate the auto-increment value. Keep suppressing generated-key reads for values like -3, but preserve backend generated-key handling for NULL and 0 where the storage engine generates the value. Please add regression tests for both sides: explicit nonspecial values return no generated key, while explicit NULL/0 generated cases still return the generated key.

Multi-Round Comparison

• Previous blocker: The prior review requested moving the fix earlier than UpdateResponseHeader so Proxy would not request/read backend generated keys for explicit nonspecial values. This is partially fixed in the latest head by gating both normal and federation execution paths.
• New blocker: The new gate uses GeneratedKeyContext.isGenerated() as a broader “should ask backend for generated keys” signal, but that flag does not model MySQL’s explicit NULL/0 generation semantics. The adjacent generated-key case remains untested.

Review Details

• Reviewed Scope: Latest PR head d0d9c09c91a48602130176bebfb6ba4e373eac00; GitHub base SHA 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; local merge-base 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; local triple-dot file list matched GitHub /pulls/38810/files. Reviewed RELEASE-NOTES.md, proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java, proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java, and the three changed test files, plus related generated-key binder/callback/OK-packet paths. Shared-path blast radius checked for MySQL and MariaDB, which expose DialectGeneratedKeyOption; dialects such as PostgreSQL/openGauss/SQL92 default to no generated-key option on this path. Review baseline included CODE_OF_CONDUCT.md:8, CODE_OF_CONDUCT.md:11, CODE_OF_CONDUCT.md:17, and CODE_OF_CONDUCT.md:18.
• Not Reviewed Scope: CI/check statuses, full live Proxy + MySQL reproduction, and unrelated dirty local workspace files were not reviewed or used for conclusions.
• Verification: git fetch apache master:refs/remotes/apache/master pull/38810/head:refs/remotes/apache/pr/38810 exit 0; local git diff --name-status 8162bdf1377764c7cf4a52edd32c35f4d0a2796c..refs/remotes/apache/pr/38810 matched GitHub /pulls/38810/files exit 0; static git show / git grep inspection exit 0; git grep computeIfAbsent on changed production files returned no matches. Maven was not run because the merge blocker is visible in the control flow and the current tests do not cover the adjacent NULL/0 generated-key case.
• Release Note / User Docs: Release note is present at RELEASE-NOTES.md:29; user docs are not required for this internal Proxy bug fix, but the release-note claim should remain only after the regression above is fixed.

[somiljain2006]

Thanks for the detailed review.

I've updated the implementation to distinguish explicit non-special values from explicit values that still trigger AUTO_INCREMENT generation in MySQL/MariaDB.

Changes made:

• Kept generated-key suppression for explicit nonspecial values such as -3.
• Preserved generated-key handling for explicit NULL and 0, which continue to request database-side AUTO_INCREMENT generation.
• Applied the same logic consistently to both the normal Proxy execution path and the federation execution path.
• Added regression coverage for:
  • Explicit nonspecial value -3 returns no generated key
  • Explicit NULL returns a generated key
  • Parameter-bound nonspecial value -3 returns no generated key
  • Parameter-bound 0 returns a generated key
• Existing generated-key behavior remains covered by the callback-level tests.

All related tests are passing locally.