Repository navigation
Add support for ALTER COLUMN with SET DEFAULT and DROP DEFAULT - #555
Conversation
The plugin exposes MySQL on SQLite through a symlink. Apply the existing driver exemption there as well, since this code implements the database layer and cannot use wpdb.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 55.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 5 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely identifies the primary change: support for ALTER COLUMN with SET DEFAULT and DROP DEFAULT. It matches the main implementation and test coverage. Linked Issues check The PR satisfies the coding requirements in directly linked issue #553.record_alter_table()now recordsSET DEFAULTandDROP DEFAULTchanges in information-schema metadata. Table rebuilds there…Out of Scope Changes check The changes remain within the stated PR scope. Trigger detection, missing-table and missing-column errors, exception conversion, and BIT default handling support the described compatibility and error … Full details: Linked Issues check
-
Explanation
The PR satisfies the coding requirements in directly linked issue
#553.record_alter_table()now recordsSET DEFAULTandDROP DEFAULTchanges in information-schema metadata. Table rebuilds therefore use the new defaults for later inserts and preserve existing values. The tests cover permanent and temporary tables, metadata, literals, expressions, quoted names, ordered actions, invalid defaults, missing objects, rollback, and automatic ID generation.
Full details: Out of Scope Changes check
-
Explanation
The changes remain within the stated PR scope. Trigger detection, missing-table and missing-column errors, exception conversion, and BIT default handling support the described compatibility and error cases. The added tests verify the requested behavior. The PDO exclusion covers the symlinked driver named in the PR description. No unrelated change is demonstrated.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 PHPStan (2.3.0)
-
PHPStan reported 1 non-file analysis error(s); analysis is incomplete
🔧 PHPMD (3.0.0)
-
packages/mysql-on-sqlite/tests/WP_MySQL_On_SQLite_Tests.php
-
PHPMD could not process this file (exit code 255): 0/1 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░] 0%
1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 20971520 bytes) in phar:///usr/bin/phpmd/vendor/pdepend/pdepend/src/Util/Cache/Driver/FileCacheDriver.php on line 183
-
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
44be48b to
0e17bbd
Compare
JanJakes
changed the title
ALTER COLUMN with SET DEFAULT and DROP DEFAULT
0e17bbd to
f17b3bf
Compare
f17b3bf to
0ce3c18
Compare
0ce3c18 to
874ffb4
Compare
Record SET DEFAULT and DROP DEFAULT using shared default handling, in order with the other ALTER TABLE actions. Preserve unrelated column metadata and reject missing columns. Reject NULL defaults on NOT NULL columns and keep accepted AUTO_INCREMENT defaults unset. Cover inserted values, schema metadata, temporary tables, quoted names, expressions, and rollback on failure. Fixes #553
874ffb4 to
b585b07
Compare
brandonpayton
left a comment
brandonpayton
left a comment
There was a problem hiding this comment.
I'm Codex, posting this local review feedback at @brandonpayton's request through their authenticated GitHub account.
I have two correctness concerns and one compatibility concern, detailed inline. The existing MySQL-on-SQLite suite passes (1,135 tests; 15 skipped, 2 incomplete). The timestamp regression needs a behavioral assertion beyond checking metadata and inserted defaults. The default-validation gap predates this PR and could be addressed in a separate follow-up.
| if ( str_contains( $extra, 'auto_increment' ) ) { | ||
| throw WP_SQLite_Information_Schema_Exception::invalid_default_value( $column_name ); | ||
| } | ||
| $extra = trim( 'DEFAULT_GENERATED ' . $extra ); |
There was a problem hiding this comment.
Codex review feedback, requested by @brandonpayton.
Expression defaults can disable ON UPDATE CURRENT_TIMESTAMP. The rebuild checks whether EXTRA exactly equals on update CURRENT_TIMESTAMP. Adding DEFAULT_GENERATED causes it to skip recreating the trigger, leaving timestamps stale even though the reported schema retains ON UPDATE.
Suggested fix: Use str_contains() for that attribute check in get_sqlite_create_table_statement(). Add a regression test that updates an existing row while the expression default is active, covering permanent and temporary tables.
There was a problem hiding this comment.
Thanks, I found this during self-review but decided to extract it to #554. The reason is that doing only the str_contains fix amplifies another issue with the timestamp trigger (it always overrides explicit values), and so I think it's better to fix both together as a follow-up.
| $default = $literal | ||
| ? $this->get_literal_default( $literal, $column['DATA_TYPE'], $column_name ) | ||
| : $this->get_default_value( $node, $column['DATA_TYPE'], $column_name ); |
There was a problem hiding this comment.
Codex review feedback, requested by @brandonpayton.
Invalid literal defaults can be accepted and break later inserts. For example, SET DEFAULT 'invalid' succeeds on an INT column, then inserts omitting that column fail. This is an existing validation gap exposed by the new ALTER support.
Suggested fix: Add shared validation and normalization using the literal AST and complete column metadata. Reuse it across CREATE and ALTER operations, raise the existing invalid-default exception, and test rollback. This broader work could be a separate follow-up.
There was a problem hiding this comment.
Agreed. This is fairly complex because it requires extensive type conversion and validation in PHP. It’s already tracked in #551 (and pre-existing for all existing ways to set column defaults). I guess I should address it soon in a follow-up, because particular instances of this issue keep coming up.
| if ( str_contains( $extra, 'auto_increment' ) ) { | ||
| $default = null; |
There was a problem hiding this comment.
Codex review feedback, requested by @brandonpayton.
Defaults on AUTO_INCREMENT columns are accepted but discarded. The comments and tests acknowledge this divergence, but successful execution gives callers the impression that the requested behavior was applied.
Suggested fix: Either preserve the additional metadata needed to emulate the behavior or explicitly reject unsupported changes. I'd avoid locking silent acceptance into the tests.
There was a problem hiding this comment.
This looks like a MySQL bug that I don’t think is worth reproducing, but I like the idea of explicitly rejecting these cases. MySQL silently applies the default without exposing it in SHOW CREATE TABLE or INFORMATION_SCHEMA, so even a schema export/import loses it (looks like a bug indeed).
|
I had Codex do an initial review and discuss its findings. They seemed worth discussing so I had them posted here. |
Reject ALTER COLUMN SET DEFAULT and DROP DEFAULT instead of silently discarding them. MySQL hides these defaults from schema metadata, so matching its behavior would require separate hidden state. Verify rejection, rollback, and subsequent inserts for regular and temporary tables. #555 (comment)
|
@brandonpayton Thanks! I replied to all the findings. Two are tracked as follow-up work, and I made the suggested fix for one of them. I'll go ahead and merge this to continue working on some related issues. |
## Summary A numeric zero default on a temporal column was stored verbatim and emitted as a quoted string: ```sql CREATE TABLE t (created_at TIMESTAMP NOT NULL DEFAULT 0); SHOW CREATE TABLE t; -- `created_at` timestamp NOT NULL DEFAULT '0' (MySQL: '0000-00-00 00:00:00') ``` MySQL rejects `DEFAULT '0'` in strict SQL mode (`ERROR 1067: Invalid default value`), so dumps of these tables don't import. The H5P plugin creates its tables with this pattern, so exports of sites that use it fail. ## Changes Zero defaults of temporal columns now get MySQL's zero values, both in new tables and in existing databases: - **Normalize on write.** `CREATE TABLE` and `ALTER TABLE` store zero defaults of `DATE`, `TIME`, `DATETIME`, `TIMESTAMP`, and `YEAR` columns as MySQL zero values, including fractional seconds precision (`DATETIME(3)` → `'0000-00-00 00:00:00.000'`). This covers all numeric forms, such as `0`, `-0`, `0.0`, `0e0`, and `FALSE`, and it fixes `SHOW CREATE TABLE`, `INFORMATION_SCHEMA.COLUMNS`, and the default that new rows receive. - **Normalize `ALTER COLUMN ... SET DEFAULT`.** Defaults set with `SET DEFAULT`, added in #555, are normalized the same way. This includes a parenthesized literal, such as `SET DEFAULT (0)`, which MySQL also stores as a literal. - **Normalize quoted zeros, except for `YEAR`.** Quoted zeros, such as `DEFAULT '0'`, are normalized the same way, as older driver versions stored them the same as numbers. For `YEAR` columns, MySQL reads the strings `'0'` and `'00'` as the year 2000, so they stay as written. - **Fix `DATETIME_PRECISION`.** `INFORMATION_SCHEMA.COLUMNS` reported `0` for all `TIME(n)`, `DATETIME(n)`, and `TIMESTAMP(n)` columns. It now reports the precision of the column type. - **Repair existing databases.** For databases last configured by driver 3.1.1 or older, the configuration corrects the stored `DATETIME_PRECISION` values, and it changes zero defaults of temporal columns, such as `0`, to the zero values with `ALTER TABLE ... ALTER COLUMN ... SET DEFAULT`. This also recreates the tables, so that new rows get the zero value. This includes tables created by the legacy SQLite driver. The repair runs once, within the configuration transaction, so it either fully succeeds or changes nothing. - **Repair existing rows.** Rows that got such a zero default from older driver versions still store it, such as `'0'`. The repair replaces it with the zero value, so that dumps of these rows import into MySQL in strict SQL mode. The update doesn't run the table's triggers, such as the emulation of `ON UPDATE CURRENT_TIMESTAMP`. - **Report why a table rebuild fails.** `ANALYZE`, `CHECK`, `OPTIMIZE`, and `REPAIR TABLE` reported every general SQLite error as a missing table, so a failed table rebuild could say `Table '…' doesn't exist` for a table that exists. They now report only SQLite's "no such table" error that way, and other errors as SQLite returns them. Values match MySQL 8 output for the same default in `CREATE TABLE` or `ALTER COLUMN ... SET DEFAULT`, in `STRICT_TRANS_TABLES` mode: | Column | `COLUMN_DEFAULT` | | --- | --- | | `TIMESTAMP` / `DATETIME DEFAULT 0` | `0000-00-00 00:00:00` | | `DATE DEFAULT 0` | `0000-00-00` | | `TIME DEFAULT 0` | `00:00:00` | | `YEAR DEFAULT 0` | `0000` | | `DATETIME(3) DEFAULT 0` | `0000-00-00 00:00:00.000` | | `TIMESTAMP(6) DEFAULT 0` | `0000-00-00 00:00:00.000000` | | `TIME(2) DEFAULT 0` | `00:00:00.00` | ## Scope - **Non-zero temporal defaults**, such as `DEFAULT 20200102`, are still stored verbatim. The existing `@TODO` covers them. - **`YEAR` string zeros**, such as `'0'`, stay as written. MySQL reads them as the year 2000, but the driver also stores an inserted `'0'` as `0000`, so both need a separate fix, tracked in #545. Older driver versions stored `YEAR DEFAULT 0` and `YEAR DEFAULT '0'` the same way, so the repair can't tell them apart and leaves them unchanged. - **Recreated tables reset their `AUTO_INCREMENT` counter** to the highest existing ID. This is an existing problem of `ALTER TABLE` and `OPTIMIZE TABLE`, tracked in #544. - **Recreated legacy tables lose `ON UPDATE CURRENT_TIMESTAMP`.** The legacy SQLite driver implemented it with triggers, which a table rebuild drops without replacing them. This is an existing problem of `ALTER TABLE` and `OPTIMIZE TABLE`, tracked in #547. The repair also rebuilds legacy tables that have a temporal zero default, so they lose it as well. ## Testing End to end, the `SHOW CREATE TABLE` output for H5P's `wp_h5p_contents` and `wp_h5p_contents_user_data` imports into MySQL with `sql_mode='STRICT_TRANS_TABLES'`. Before the change, it fails with `ERROR 1067`. See the [verification on a WordPress Studio site](#542 (comment)). --- AI assistance: Claude (Anthropic) via the Pi coding agent, with an initial candidate from a Homeboy cook. Claude traced the failure, reworked and tested the implementation, verified it against MySQL, and drafted this PR at Chris Huber's direction. Fixes #541. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Temporal columns with numeric-zero defaults now use MySQL-compatible zero dates or times, including fractional seconds. * Temporal defaults are consistently represented in table definitions, metadata, and inserted or altered rows. * Existing databases are normalized when reopened, including temporal precision corrections for older driver versions. * Non-numeric defaults and string `YEAR` zero values remain unchanged. * Foreign-key relationships and active transactions remain intact during reconfiguration. * Failed table repairs now roll back cleanly and preserve the underlying error details. * Administrative errors distinguish missing tables from other database errors. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Jan Jakeš <jan@jakes.pro>
Previously,
ALTER TABLE ... ALTER COLUMN ... SET DEFAULTandDROP DEFAULTsucceeded but left the old defaults in place. This PR makes new inserts and schema metadata use the changed defaults, while preserving existing row values and unrelated column attributes.For example, after setting an integer column's default from
1to5and dropping a string column's default of'x', an insert that omits both columns now uses5andNULL.The change also handles the related error and compatibility cases:
NULLdefaults onNOT NULLcolumns and preserves automatic ID generation.ALTER COLUMNchanges in order with the otherALTER TABLEactions, reports missing tables and columns correctly, and rolls back failed changes.SET DEFAULT (expr)on anON UPDATE CURRENT_TIMESTAMPcolumn stops its automatic update, because the driver doesn't create the update trigger for columns with an expression default. Fixing that detection alone would let the trigger overwrite explicitly written timestamps on many more tables, so it's fixed together with the trigger in #554.Keeping a nullable column without a default after
DROP DEFAULTis tracked in #556.The existing PDO coding-standard exemption also covers the plugin's symlinked copy of the driver.
Fixes #553.
Summary by CodeRabbit
ALTER TABLE, including literal and expression defaults. Updated defaults appear in column metadata and apply to subsequent inserts.BITdefaults.ON UPDATE CURRENT_TIMESTAMPcolumn settings.