From 0e27552d8d3050cee8b7f18ec95db4cabe897353 Mon Sep 17 00:00:00 2001 From: "Joseph P. White" Date: Wed, 27 Nov 2019 15:41:24 -0500 Subject: [PATCH 1/3] Fix foreign key constraint on moddb report_template_acls Also fix the ETLv2 code to correctly handle modifications to foreign keys. --- classes/ETL/DbModel/ForeignKeyConstraint.php | 8 ++++---- classes/ETL/DbModel/Table.php | 11 ++++++++--- .../etl/etl_tables.d/acls/report_template_acls.json | 3 ++- tests/ci/validate.sh | 6 ++++++ 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/classes/ETL/DbModel/ForeignKeyConstraint.php b/classes/ETL/DbModel/ForeignKeyConstraint.php index 861a2e9cbb..82e5dbaee0 100644 --- a/classes/ETL/DbModel/ForeignKeyConstraint.php +++ b/classes/ETL/DbModel/ForeignKeyConstraint.php @@ -188,15 +188,15 @@ public function compare(iEntity $cmp) } if ($this->on_delete != $cmp->on_delete - && !in_array($this->on_delete, array(null, 'RESTRICT', 'NO ACTION')) - && !in_array($cmp->on_delete, array(null, 'RESTRICT', 'NO ACTION')) + && !(in_array($this->on_delete, array(null, 'RESTRICT', 'NO ACTION')) + && in_array($cmp->on_delete, array(null, 'RESTRICT', 'NO ACTION'))) ) { return -2; } if ($this->on_update != $cmp->on_update - && !in_array($this->on_update, array(null, 'RESTRICT', 'NO ACTION')) - && !in_array($cmp->on_update, array(null, 'RESTRICT', 'NO ACTION')) + && !(in_array($this->on_update, array(null, 'RESTRICT', 'NO ACTION')) + && in_array($cmp->on_update, array(null, 'RESTRICT', 'NO ACTION'))) ) { return -3; } diff --git a/classes/ETL/DbModel/Table.php b/classes/ETL/DbModel/Table.php index de0437a1b9..8060f40d35 100644 --- a/classes/ETL/DbModel/Table.php +++ b/classes/ETL/DbModel/Table.php @@ -735,6 +735,7 @@ public function getAlterSql($destination, $includeSchema = true) $alterList = array(); $changeList = array(); $triggerList = array(); + $foreignKeyList = array(); // -------------------------------------------------------------------------------- // Process columns @@ -954,8 +955,8 @@ public function getAlterSql($destination, $includeSchema = true) if ( 0 == $destForeignKeyConstraint->compare($this->getForeignKeyConstraint($name)) ) { continue; } - $alterList[] = 'DROP FOREIGN KEY ' . $destForeignKeyConstraint->getName(true); - $alterList[] = 'ADD ' . $destForeignKeyConstraint->getSql($includeSchema); + $foreignKeyList[] = 'DROP FOREIGN KEY ' . $destForeignKeyConstraint->getName(true); + $foreignKeyList[] = 'ADD ' . $destForeignKeyConstraint->getSql($includeSchema); } // -------------------------------------------------------------------------------- @@ -998,7 +999,7 @@ public function getAlterSql($destination, $includeSchema = true) // -------------------------------------------------------------------------------- // Put it all together - if ( 0 == count($alterList) && 0 == count($changeList) && 0 == count($triggerList) ) { + if ( 0 == count($alterList) && 0 == count($changeList) && 0 == count($triggerList) && 0 == count($foreignKeyList) ) { return false; } @@ -1013,6 +1014,10 @@ public function getAlterSql($destination, $includeSchema = true) $sqlList[] = sprintf("ALTER TABLE %s\n%s;", $tableName, implode(",\n", $changeList)); } + foreach ($foreignKeyList as $fkey) { + $sqlList[] = sprintf("ALTER TABLE %s\n%s;", $tableName, $fkey); + } + if ( 0 != count($triggerList) ) { foreach ( $triggerList as $trigger ) { $sqlList[] = $trigger; diff --git a/configuration/etl/etl_tables.d/acls/report_template_acls.json b/configuration/etl/etl_tables.d/acls/report_template_acls.json index 6fe721b5b3..bce0bdd1f9 100644 --- a/configuration/etl/etl_tables.d/acls/report_template_acls.json +++ b/configuration/etl/etl_tables.d/acls/report_template_acls.json @@ -52,7 +52,8 @@ "referenced_table": "acls", "referenced_columns": [ "acl_id" - ] + ], + "on_delete": "CASCADE" } ] } diff --git a/tests/ci/validate.sh b/tests/ci/validate.sh index 5256416d77..a823914fc4 100755 --- a/tests/ci/validate.sh +++ b/tests/ci/validate.sh @@ -38,4 +38,10 @@ then exitcode=1 fi +if ! mysqldump -d moddb report_template_acls | grep -q "ON DELETE CASCADE" +then + echo "Missing/incorrect foreign key constraint on report_template_acls table" + exitcode=1 +fi + exit $exitcode From 2c9a1fa31d228af23baef9416f38edceccd1ec90 Mon Sep 17 00:00:00 2001 From: "Joseph P. White" Date: Mon, 2 Dec 2019 15:50:38 -0500 Subject: [PATCH 2/3] Update existing test artifacts --- .../xdmod/etlv2/dbmodel/output/alter_table-charset.sql | 8 +++++--- .../etlv2/dbmodel/output/alter_table_manually-charset.sql | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/artifacts/xdmod/etlv2/dbmodel/output/alter_table-charset.sql b/tests/artifacts/xdmod/etlv2/dbmodel/output/alter_table-charset.sql index 3f68260faf..33280a0ed1 100644 --- a/tests/artifacts/xdmod/etlv2/dbmodel/output/alter_table-charset.sql +++ b/tests/artifacts/xdmod/etlv2/dbmodel/output/alter_table-charset.sql @@ -2,9 +2,11 @@ ALTER TABLE `test_db_model` CHARSET = utf8mb4, COLLATE = utf8mb4_general_ci, DROP INDEX `fk_instance`, -ADD INDEX `fk_instance` USING BTREE (`instance_id`, `inferred`), -DROP FOREIGN KEY `con_col1`, -ADD CONSTRAINT `con_col1` FOREIGN KEY (`col1`) REFERENCES `db_test_model2` (`col4`); +ADD INDEX `fk_instance` USING BTREE (`instance_id`, `inferred`); ALTER TABLE `test_db_model` CHANGE COLUMN `col1` `col1` varchar(32) CHARSET utf8mb4 COLLATE utf8mb4_general_ci NOT NULL DEFAULT 'mydefault' , CHANGE COLUMN `instance_id` `instance_id` int(11) NULL DEFAULT -1 ; +ALTER TABLE `test_db_model` +DROP FOREIGN KEY `con_col1`; +ALTER TABLE `test_db_model` +ADD CONSTRAINT `con_col1` FOREIGN KEY (`col1`) REFERENCES `db_test_model2` (`col4`); diff --git a/tests/artifacts/xdmod/etlv2/dbmodel/output/alter_table_manually-charset.sql b/tests/artifacts/xdmod/etlv2/dbmodel/output/alter_table_manually-charset.sql index df4f004c96..d79f891376 100644 --- a/tests/artifacts/xdmod/etlv2/dbmodel/output/alter_table_manually-charset.sql +++ b/tests/artifacts/xdmod/etlv2/dbmodel/output/alter_table_manually-charset.sql @@ -6,11 +6,13 @@ ADD COLUMN `new_column2` char(64) CHARSET utf8mb4 COLLATE utf8mb4_general_ci NUL ADD INDEX `index_new_column` (`new_column`), DROP INDEX `fk_instance`, ADD INDEX `fk_instance` USING BTREE (`instance_id`, `inferred`), -ADD CONSTRAINT `fk_new_column` FOREIGN KEY (`new_column`) REFERENCES `other_table` (`other_column`), -DROP FOREIGN KEY `con_col1`, -ADD CONSTRAINT `con_col1` FOREIGN KEY (`col1`) REFERENCES `db_test_model2` (`col4`); +ADD CONSTRAINT `fk_new_column` FOREIGN KEY (`new_column`) REFERENCES `other_table` (`other_column`); ALTER TABLE `test_db_model` CHANGE COLUMN `col1` `col1` varchar(32) CHARSET utf8mb4 COLLATE utf8mb4_general_ci NOT NULL DEFAULT 'mydefault' , CHANGE COLUMN `instance_id` `instance_id` int(11) NULL DEFAULT -1 ; +ALTER TABLE `test_db_model` +DROP FOREIGN KEY `con_col1`; +ALTER TABLE `test_db_model` +ADD CONSTRAINT `con_col1` FOREIGN KEY (`col1`) REFERENCES `db_test_model2` (`col4`); CREATE TRIGGER `before_ins` BEFORE INSERT ON `jobfact` FOR EACH ROW BEGIN DELETE FROM jobfactstatus WHERE job_id = NEW.job_id; END From eaf737cf8cb12ed027b5f4d6eff909486ecb59b5 Mon Sep 17 00:00:00 2001 From: "Joseph P. White" Date: Tue, 3 Dec 2019 14:50:53 -0500 Subject: [PATCH 3/3] Add explicit test for NO ACTION to CASCADE updates --- tests/unit/lib/ETL/DbModel/DbModelTest.php | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/unit/lib/ETL/DbModel/DbModelTest.php b/tests/unit/lib/ETL/DbModel/DbModelTest.php index a5bde78b2e..c307d2156a 100644 --- a/tests/unit/lib/ETL/DbModel/DbModelTest.php +++ b/tests/unit/lib/ETL/DbModel/DbModelTest.php @@ -259,6 +259,38 @@ public function testCreateSql() } + /** + * Confirm foreign key contraint changes work. + */ + public function testAlterTableContraints() + { + $config = self::TEST_ARTIFACT_INPUT_PATH . '/table_def-charset.json'; + + $origTable = new Table($config, '`', self::$logger); + $fkconfig = (object) array( + 'columns' => array('new_column'), + 'referenced_table' => 'other_table', + 'referenced_columns' => array('other_column'), + 'on_delete' => 'NO ACTION' + ); + $origTable->addForeignKeyConstraint($fkconfig); + + $targetTable = new Table($config, '`', self::$logger); + $fkconfig1 = (object) array( + 'columns' => array('new_column'), + 'referenced_table' => 'other_table', + 'referenced_columns' => array('other_column'), + 'on_delete' => 'CASCADE' + ); + $targetTable->addForeignKeyConstraint($fkconfig1); + + $sql = $origTable->getAlterSql($targetTable); + + $this->assertCount(2, $sql); + $this->assertEquals("ALTER TABLE `test_db_model`\nDROP FOREIGN KEY `fk_new_column`;", trim($sql[0])); + $this->assertEquals("ALTER TABLE `test_db_model`\nADD CONSTRAINT `fk_new_column` FOREIGN KEY (`new_column`) REFERENCES `other_table` (`other_column`) ON DELETE CASCADE;", trim($sql[1])); + } + /** * Test comparing 2 tables and the ALTER TABLE statement needed to go from one to the other. * Also manually add elements and verify the ALTER TABLE statement generated.