Skip to content

Commit f4212a3

Browse files
JanJakesJanJakes
authored
Fix & improve ON UPDATE trigger behavior (#156)
This just fixes and improves #150. With some further testing, I realized there were some issues in my original implementation: 1. The `ON UPDATE` trigger uses `id` instead of `rowid`. This is a regression — a query can fail now for anyone who uses `ON UPDATE` and doesn't have an `id` column. I somehow missed that 🤦‍♂️ 2. The trigger is deleted with every column operation, not only for column `CHANGE`s, and it is actually missed by the `CHANGE` for creation as well, as that branch uses `continue`/`break` and skips the trigger creation below. I fixed both and added test coverage.
1 parent e88a4e5 commit f4212a3

2 files changed

Lines changed: 141 additions & 7 deletions

File tree

‎tests/WP_SQLite_Translator_Tests.php‎

Lines changed: 125 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1102,14 +1102,14 @@ public function testColumnWithOnUpdate() {
11021102
'name' => '___tmp_table_created_at_on_update__',
11031103
'tbl_name' => '_tmp_table',
11041104
'rootpage' => '0',
1105-
'sql' => "CREATE TRIGGER \"___tmp_table_created_at_on_update__\"\n\t\t\tAFTER UPDATE ON \"_tmp_table\"\n\t\t\tFOR EACH ROW\n\t\t\tBEGIN\n\t\t\t UPDATE \"_tmp_table\" SET \"created_at\" = CURRENT_TIMESTAMP WHERE id = NEW.id;\n\t\t\tEND",
1105+
'sql' => "CREATE TRIGGER \"___tmp_table_created_at_on_update__\"\n\t\t\tAFTER UPDATE ON \"_tmp_table\"\n\t\t\tFOR EACH ROW\n\t\t\tBEGIN\n\t\t\t UPDATE \"_tmp_table\" SET \"created_at\" = CURRENT_TIMESTAMP WHERE rowid = NEW.rowid;\n\t\t\tEND",
11061106
),
11071107
(object) array(
11081108
'type' => 'trigger',
11091109
'name' => '___tmp_table_updated_at_on_update__',
11101110
'tbl_name' => '_tmp_table',
11111111
'rootpage' => '0',
1112-
'sql' => "CREATE TRIGGER \"___tmp_table_updated_at_on_update__\"\n\t\t\tAFTER UPDATE ON \"_tmp_table\"\n\t\t\tFOR EACH ROW\n\t\t\tBEGIN\n\t\t\t UPDATE \"_tmp_table\" SET \"updated_at\" = CURRENT_TIMESTAMP WHERE id = NEW.id;\n\t\t\tEND",
1112+
'sql' => "CREATE TRIGGER \"___tmp_table_updated_at_on_update__\"\n\t\t\tAFTER UPDATE ON \"_tmp_table\"\n\t\t\tFOR EACH ROW\n\t\t\tBEGIN\n\t\t\t UPDATE \"_tmp_table\" SET \"updated_at\" = CURRENT_TIMESTAMP WHERE rowid = NEW.rowid;\n\t\t\tEND",
11131113
),
11141114
),
11151115
$results
@@ -1176,6 +1176,129 @@ public function testColumnWithOnUpdate() {
11761176
$this->assertNull( $result[0]->updated_at );
11771177
}
11781178

1179+
public function testColumnWithOnUpdateAndNoIdField() {
1180+
// CREATE TABLE with ON UPDATE
1181+
$this->assertQuery(
1182+
'CREATE TABLE _tmp_table (
1183+
name varchar(20) NOT NULL,
1184+
created_at timestamp NULL ON UPDATE CURRENT_TIMESTAMP
1185+
);'
1186+
);
1187+
1188+
// on INSERT, no timestamps are expected
1189+
$this->assertQuery( "INSERT INTO _tmp_table (name) VALUES ('aaa')" );
1190+
$result = $this->assertQuery( "SELECT * FROM _tmp_table WHERE name = 'aaa'" );
1191+
$this->assertNull( $result[0]->created_at );
1192+
1193+
// on UPDATE, we expect timestamps in form YYYY-MM-DD HH:MM:SS
1194+
$this->assertQuery( "UPDATE _tmp_table SET name = 'bbb' WHERE name = 'aaa'" );
1195+
$result = $this->assertQuery( "SELECT * FROM _tmp_table WHERE name = 'bbb'" );
1196+
$this->assertRegExp( '/\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d/', $result[0]->created_at );
1197+
}
1198+
1199+
public function testChangeColumnWithOnUpdate() {
1200+
// CREATE TABLE with ON UPDATE
1201+
$this->assertQuery(
1202+
'CREATE TABLE _tmp_table (
1203+
id int(11) NOT NULL,
1204+
created_at timestamp NULL
1205+
);'
1206+
);
1207+
$results = $this->assertQuery( 'DESCRIBE _tmp_table;' );
1208+
$this->assertEquals(
1209+
array(
1210+
(object) array(
1211+
'Field' => 'id',
1212+
'Type' => 'int(11)',
1213+
'Null' => 'NO',
1214+
'Key' => '',
1215+
'Default' => '0',
1216+
'Extra' => '',
1217+
),
1218+
(object) array(
1219+
'Field' => 'created_at',
1220+
'Type' => 'timestamp',
1221+
'Null' => 'YES',
1222+
'Key' => '',
1223+
'Default' => null,
1224+
'Extra' => '',
1225+
),
1226+
),
1227+
$results
1228+
);
1229+
1230+
// no ON UPDATE is set
1231+
$this->assertQuery( 'INSERT INTO _tmp_table (id) VALUES (1)' );
1232+
$this->assertQuery( 'UPDATE _tmp_table SET id = 1 WHERE id = 1' );
1233+
$result = $this->assertQuery( 'SELECT * FROM _tmp_table WHERE id = 1' );
1234+
$this->assertNull( $result[0]->created_at );
1235+
1236+
// CHANGE COLUMN to add ON UPDATE
1237+
$this->assertQuery(
1238+
'ALTER TABLE _tmp_table CHANGE COLUMN created_at created_at timestamp NULL ON UPDATE CURRENT_TIMESTAMP'
1239+
);
1240+
$results = $this->assertQuery( 'DESCRIBE _tmp_table;' );
1241+
$this->assertEquals(
1242+
array(
1243+
(object) array(
1244+
'Field' => 'id',
1245+
'Type' => 'int(11)',
1246+
'Null' => 'NO',
1247+
'Key' => '',
1248+
'Default' => '0',
1249+
'Extra' => '',
1250+
),
1251+
(object) array(
1252+
'Field' => 'created_at',
1253+
'Type' => 'timestamp',
1254+
'Null' => 'YES',
1255+
'Key' => '',
1256+
'Default' => null,
1257+
'Extra' => '',
1258+
),
1259+
),
1260+
$results
1261+
);
1262+
1263+
// now, ON UPDATE SHOULD BE SET
1264+
$this->assertQuery( 'UPDATE _tmp_table SET id = 1 WHERE id = 1' );
1265+
$result = $this->assertQuery( 'SELECT * FROM _tmp_table WHERE id = 1' );
1266+
$this->assertRegExp( '/\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d/', $result[0]->created_at );
1267+
1268+
// change column to remove ON UPDATE
1269+
$this->assertQuery(
1270+
'ALTER TABLE _tmp_table CHANGE COLUMN created_at created_at timestamp NULL'
1271+
);
1272+
$results = $this->assertQuery( 'DESCRIBE _tmp_table;' );
1273+
$this->assertEquals(
1274+
array(
1275+
(object) array(
1276+
'Field' => 'id',
1277+
'Type' => 'int(11)',
1278+
'Null' => 'NO',
1279+
'Key' => '',
1280+
'Default' => '0',
1281+
'Extra' => '',
1282+
),
1283+
(object) array(
1284+
'Field' => 'created_at',
1285+
'Type' => 'timestamp',
1286+
'Null' => 'YES',
1287+
'Key' => '',
1288+
'Default' => null,
1289+
'Extra' => '',
1290+
),
1291+
),
1292+
$results
1293+
);
1294+
1295+
// now, no timestamp is expected
1296+
$this->assertQuery( 'INSERT INTO _tmp_table (id) VALUES (2)' );
1297+
$this->assertQuery( 'UPDATE _tmp_table SET id = 2 WHERE id = 2' );
1298+
$result = $this->assertQuery( 'SELECT * FROM _tmp_table WHERE id = 2' );
1299+
$this->assertNull( $result[0]->created_at );
1300+
}
1301+
11791302
public function testAlterTableWithColumnFirstAndAfter() {
11801303
$this->assertQuery(
11811304
"CREATE TABLE _tmp_table (

‎wp-includes/sqlite/class-wp-sqlite-translator.php‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1145,7 +1145,8 @@ private function parse_mysql_create_table_field() {
11451145
array( 'CURRENT_TIMESTAMP' )
11461146
)
11471147
) {
1148-
$this->rewriter->skip();
1148+
$this->rewriter->skip(); // ON UPDATE
1149+
$this->rewriter->skip(); // CURRENT_TIMESTAMP
11491150
$result->on_update = true;
11501151
continue;
11511152
}
@@ -3118,6 +3119,10 @@ private function execute_alter() {
31183119
$new_field->mysql_data_type
31193120
);
31203121

3122+
// Drop ON UPDATE trigger by the old column name.
3123+
$on_update_trigger_name = $this->get_column_on_update_current_timestamp_trigger_name( $this->table_name, $from_name );
3124+
$this->execute_sqlite_query( "DROP TRIGGER IF EXISTS \"$on_update_trigger_name\"" );
3125+
31213126
/*
31223127
* In SQLite, there is no direct equivalent to the CHANGE COLUMN
31233128
* statement from MySQL. We need to do a bit of work to emulate it.
@@ -3238,6 +3243,11 @@ private function execute_alter() {
32383243
);
32393244
}
32403245

3246+
// Add the ON UPDATE trigger if needed.
3247+
if ( $new_field->on_update ) {
3248+
$this->add_column_on_update_current_timestamp( $this->table_name, $new_field->name );
3249+
}
3250+
32413251
if ( ',' === $alter_terminator->token ) {
32423252
/*
32433253
* If the terminator was a comma,
@@ -3330,9 +3340,6 @@ private function execute_alter() {
33303340
);
33313341
$this->rewriter->drop_last();
33323342

3333-
$on_update_trigger_name = $this->get_column_on_update_current_timestamp_trigger_name( $this->table_name, $op_subject );
3334-
$this->execute_sqlite_query( "DROP TRIGGER IF EXISTS \"$on_update_trigger_name\"" );
3335-
33363343
$this->execute_sqlite_query(
33373344
$this->rewriter->get_updated_query()
33383345
);
@@ -4397,12 +4404,16 @@ private function generate_index_name( $table, $original_index_name ) {
43974404
*/
43984405
private function add_column_on_update_current_timestamp( $table, $column ) {
43994406
$trigger_name = $this->get_column_on_update_current_timestamp_trigger_name( $table, $column );
4407+
4408+
// The trigger wouldn't work for virtual and "WITHOUT ROWID" tables,
4409+
// but currently that can't happen as we're not creating such tables.
4410+
// See: https://www.sqlite.org/rowidtable.html
44004411
$this->execute_sqlite_query(
44014412
"CREATE TRIGGER \"$trigger_name\"
44024413
AFTER UPDATE ON \"$table\"
44034414
FOR EACH ROW
44044415
BEGIN
4405-
UPDATE \"$table\" SET \"$column\" = CURRENT_TIMESTAMP WHERE id = NEW.id;
4416+
UPDATE \"$table\" SET \"$column\" = CURRENT_TIMESTAMP WHERE rowid = NEW.rowid;
44064417
END"
44074418
);
44084419
}

0 commit comments

Comments
 (0)