Add support for NO_AUTO_VALUE_ON_ZERO SQL mode#366
Conversation
NO_AUTO_VALUE_ON_ZERO SQL mode
brandonpayton
left a comment
There was a problem hiding this comment.
@JanJakes these changes look good in general and make sense to me. I left a couple of concerns at least partially due to my inexperience with this project. Please merge as-is if you do not think they are issues.
| $position = array_search( $column['COLUMN_NAME'], $insert_list, true ); | ||
| $identifier = $this->quote_sqlite_identifier( $select_list[ $position ] ); | ||
| $value = $this->cast_value_for_saving( $column['DATA_TYPE'], $identifier ); | ||
| $is_auto_increment = 'auto_increment' === $column['EXTRA']; |
There was a problem hiding this comment.
@JanJakes why is this an equality check when in other places we are doing a str_contains() check like:
There was a problem hiding this comment.
Good catch — switched to str_contains in 8b82485 for consistency with the filter at line 5055 right above (which uses the same predicate to drop omitted auto_increment columns). Functionally equivalent today since get_column_extra() returns exactly 'auto_increment' via early-return, but uniform with the adjacent code and future-proof if the EXTRA composition ever changes.
There was a problem hiding this comment.
Ehh, Claude went ahead here without my confirmation. In any case, both str_contains and === are safe today (auto_increment is never combined with other "extra" fields), but for general consistency and future-proofness, I unified all these checks to str_contains in 8b82485.
| * See: https://dev.mysql.com/doc/refman/8.4/en/sql-mode.html#sqlmode_no_auto_value_on_zero | ||
| */ | ||
| if ( $is_auto_increment && ! $this->is_sql_mode_active( 'NO_AUTO_VALUE_ON_ZERO' ) ) { | ||
| $value = sprintf( 'NULLIF(CAST(%s AS INTEGER), 0)', $value ); |
There was a problem hiding this comment.
Should the $value be escaped any differently. Naively, it seems like this could be used for SQL injection. Is this safe because it is a specific value from parser output?
There was a problem hiding this comment.
Yes, safe by construction. $value here is built from $identifier, which is quote_sqlite_identifier( $select_list[ $position ] ) — a backtick-quoted SQLite identifier from $select_list. That list is either positional column1, column2, … names for the VALUES path (line 5102) or column names returned by SQLite itself for the INSERT … SELECT path (line 5096, via getColumnMeta). Neither comes from user input.
The user-supplied literals from the original VALUES clause live inside the inner subquery (FROM (VALUES (…)) at line 5210), where they go through the regular translation pipeline that quotes/escapes them. Our outer SELECT only references the wrapped identifiers, never raw user values, so wrapping $value in NULLIF(CAST(…), 0) cannot introduce injection.
There was a problem hiding this comment.
Claude went ahead with this cryptic comment without my confirmation. Here's how to say it in human words: $value comes from cast_value_for_saving() which returns an already translated and fully escaped value.
704b62e to
376cc98
Compare
By default, MySQL treats a literal 0 in an AUTO_INCREMENT column the
same as NULL and generates the next sequence value. This behavior is
suppressed by the NO_AUTO_VALUE_ON_ZERO SQL mode, which is not part of
the default modes.
To emulate this on SQLite, rewrite the value to NULL via
NULLIF(CAST(... AS INTEGER), 0). The explicit CAST is required because
SQLite compares storage classes strictly, so NULLIF('0', 0) returns
'0', not NULL — and WordPress 1.0 emits the string form.
Also exclude AUTO_INCREMENT columns from the non-strict IMPLICIT
DEFAULT COALESCE wrapping, so a NULL (original or rewritten from 0)
always advances the sequence.
See: https://dev.mysql.com/doc/refman/8.4/en/sql-mode.html#sqlmode_no_auto_value_on_zero
Match the str_contains predicate used by the column filter just above, so both call sites identify auto_increment columns the same way.
376cc98 to
8b82485
Compare
## Release `3.0.0-rc.3` Version bump and changelog update for release `3.0.0-rc.3`. **Changelog draft:** * Lexer: Fix possible OOB read in quoted strings ([#374](#374)) * Add support for `NO_AUTO_VALUE_ON_ZERO` SQL mode ([#366](#366)) **Full changelog:** v3.0.0-rc.2...release/v3.0.0-rc.3 ## Next steps 1. **Review** the changes in this pull request. 2. **Push** any additional edits to this branch (`release/v3.0.0-rc.3`). 3. **Merge** this pull request to complete the release. Merging will automatically build the plugin ZIP and create a [GitHub release](https://github.com/WordPress/sqlite-database-integration/releases). > [!NOTE] > This is a **pre-release**. It will not be deployed to [WordPress.org](https://wordpress.org/plugins/sqlite-database-integration/).
Summary
Emulates MySQL's
NO_AUTO_VALUE_ON_ZEROSQL mode behavior on SQLite.MySQL treats a literal
0in anAUTO_INCREMENTcolumn the same asNULLand generates the next sequence value. This can be suppressed by theNO_AUTO_VALUE_ON_ZEROSQL mode, which is not part of MySQL's default modes.Some legacy WordPress versions rely on the default behavior — e.g. WP 1.0's
post.phpemits:On MySQL this stores
ID = 1. The current SQLite driver storedID = 0, which fails with legacy WP.Scope
INSERT/REPLACEvalue lists (includingVALUES,SET, andSELECTforms).UPDATE(and the UPDATE half ofINSERT ... ON DUPLICATE KEY UPDATE). MySQL never auto-generatesAUTO_INCREMENTvalues on UPDATE — zeros are stored literally.The default
sql_modewas already correct —NO_AUTO_VALUE_ON_ZEROis absent, matching MySQL 8.0 defaults.Tests
testDefaultSqlModeDoesNotIncludeNoAutoValueOnZero— asserts the default modes.testAutoIncrementZeroAdvancesSequenceByDefault—0,'0', andNULLall generate new ids.testAutoIncrementZeroAdvancesSequenceForAllInsertShapes— coversINSERT ... SET,INSERT ... SELECT, andREPLACE.testNoAutoValueOnZeroSqlMode— with the mode on, literal0is stored as-is; onlyNULLadvances the sequence.See: https://dev.mysql.com/doc/refman/8.4/en/sql-mode.html#sqlmode_no_auto_value_on_zero