Fix foreign key constraints (#704)

- Add an index for each ACL foreign key constraint - Add verification that each foreign key constraint has a corresponding index - Fix comparison function to correctly check "ON DELETE" and "ON UPDATE" operations
ubccr · Oct 16, 2018 · 1c60333 · 1c60333
1 parent e79790b
commit 1c60333
Show file tree

Hide file tree

Showing 17 changed files with 216 additions and 11 deletions.
diff --git a/classes/ETL/DbModel/ForeignKeyConstraint.php b/classes/ETL/DbModel/ForeignKeyConstraint.php
@@ -169,7 +169,9 @@ private function generateForeignKeyConstraintName(array $columns)
     }
 
     /**
-     * Foreign key constraints are considered equal if all properties are the same.
+     * Foreign key constraints are considered equal if all properties are the
+     * same.  For "ON DELETE" and "ON UPDATE" the operations "NO ACTION" and
+     * "RESTRICT" are the same as omitting an operation.
      */
     public function compare(iEntity $cmp)
     {
@@ -181,12 +183,24 @@ public function compare(iEntity $cmp)
             || $this->columns != $cmp->columns
             || $this->referenced_table != $cmp->referenced_table
             || $this->referenced_columns != $cmp->referenced_columns
-            || $this->on_delete != $cmp->on_delete
-            || $this->on_update != $cmp->on_update
         ) {
             return -1;
         }
 
+        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'))
+        ) {
+            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'))
+        ) {
+            return -3;
+        }
+
         return 0;
     }
 

diff --git a/classes/ETL/DbModel/Table.php b/classes/ETL/DbModel/Table.php
@@ -166,7 +166,8 @@ public function verify()
             }
         }  // foreach ( $this->indexes as $index )
 
-        // Verify foreign key constraint columns match table columns
+        // Verify foreign key constraint columns match table columns and are
+        // contained in the beginning of an index.
 
         foreach ( $this->foreign_key_constraints as $constraint ) {
             $missingColumnNames = array_diff($constraint->columns, $columnNames);
@@ -175,6 +176,28 @@ public function verify()
                     sprintf("Columns in foreign key constraint '%s' not found in table definition: %s", $constraint->name, implode(", ", $missingColumnNames))
                 );
             }
+
+            $foundCorrespondingIndex = false;
+            foreach ( $this->indexes as $index ) {
+                // Skip any index with fewer columns than the constraint.
+                if ( count($constraint->columns) > count($index->columns) ) {
+                    continue;
+                }
+                // Compare columns starting at the beginning of the index.
+                foreach ( $constraint->columns as $i => $column ) {
+                    if ( $column != $index->columns[$i] ) {
+                        // Index doesn't match, check next index.
+                        continue 2;
+                    }
+                }
+                $foundCorrespondingIndex = true;
+                break;
+            }
+            if ( ! $foundCorrespondingIndex ) {
+                $this->logAndThrowException(
+                    sprintf("Columns in foreign key constraint '%s' must be contained at the beginning of an index", $constraint->name)
+                );
+            }
         }  // foreach ( $this->foreign_key_constraints as $constraint )
 
         return true;
@@ -333,13 +356,18 @@ public function discover($source)
     tc.constraint_name AS name,
     GROUP_CONCAT(kcu.column_name ORDER BY position_in_unique_constraint ASC) AS columns,
     kcu.referenced_table_name AS referenced_table,
-    GROUP_CONCAT(kcu.referenced_column_name ORDER BY position_in_unique_constraint ASC) AS referenced_columns
+    GROUP_CONCAT(kcu.referenced_column_name ORDER BY position_in_unique_constraint ASC) AS referenced_columns,
+    rc.update_rule AS on_update,
+    rc.delete_rule AS on_delete
 FROM information_schema.table_constraints tc
 INNER JOIN information_schema.key_column_usage kcu
     ON tc.table_schema = kcu.table_schema
     AND tc.table_name = kcu.table_name
     AND tc.constraint_schema = kcu.constraint_schema
     AND tc.constraint_name = kcu.constraint_name
+INNER JOIN information_schema.referential_constraints rc
+    ON tc.constraint_schema = rc.constraint_schema
+    AND tc.constraint_name = rc.constraint_name
 WHERE tc.constraint_type = 'FOREIGN KEY'
     AND tc.table_schema = :schema
     AND tc.table_name = :tablename
@@ -795,7 +823,7 @@ public function getAlterSql($destination, $includeSchema = true)
 
             // When adding columns, maintain the same order as in the definition array. Note that
             // array_diff() maintains the array index so we are able to look up the previous column.
-            
+
             $position = "FIRST";
             if ( $index > 0 ) {
                 $afterColName = $destColNames[$index-1];
@@ -806,7 +834,7 @@ public function getAlterSql($destination, $includeSchema = true)
                 }
                 $position = "AFTER " . $destination->quote($afterColName);
             }
-            
+
             $alterList[] = sprintf(
                 "ADD COLUMN %s %s",
                 $destination->getColumn($name)->getSql($includeSchema),
@@ -845,7 +873,7 @@ public function getAlterSql($destination, $includeSchema = true)
                     $position = "AFTER " . $destination->quote($destColNames[$index-1]);
                 }
             }
-            
+
             $changeList[] = sprintf(
                 "CHANGE COLUMN %s %s %s",
                 $destination->quote($name),
@@ -858,7 +886,7 @@ public function getAlterSql($destination, $includeSchema = true)
             $destColumn = $destination->getColumn($toColumnName);
             $currentColumn = $this->getColumn($fromColumnName);
             $index = array_search($toColumnName, $destColNames);
-            
+
             $position = "FIRST";
             if ( $index > 0 ) {
                 $position = "AFTER " . $destination->quote($destColNames[$index-1]);

diff --git a/configuration/etl/etl_tables.d/acls/acl_group_bys.json b/configuration/etl/etl_tables.d/acls/acl_group_bys.json
@@ -62,6 +62,30 @@
         ],
         "type": "BTREE",
         "is_unique": true
+      },
+      {
+        "name": "idx_acl_id",
+        "columns": [
+          "acl_id"
+        ]
+      },
+      {
+        "name": "idx_group_by_id",
+        "columns": [
+          "group_by_id"
+        ]
+      },
+      {
+        "name": "idx_realm_id",
+        "columns": [
+          "realm_id"
+        ]
+      },
+      {
+        "name": "idx_statistic_id",
+        "columns": [
+          "statistic_id"
+        ]
       }
     ],
     "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/acl_hierarchies.json b/configuration/etl/etl_tables.d/acls/acl_hierarchies.json
@@ -49,6 +49,12 @@
           "hierarchy_id"
         ],
         "type": "BTREE"
+      },
+      {
+        "name": "idx_hierarchy_id",
+        "columns": [
+          "hierarchy_id"
+        ]
       }
     ],
     "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/acl_tabs.json b/configuration/etl/etl_tables.d/acls/acl_tabs.json
@@ -40,6 +40,12 @@
                     "acl_id",
                     "tab_id"
                 ]
+            },
+            {
+                "name": "idx_tab_id",
+                "columns": [
+                    "tab_id"
+                ]
             }
         ],
         "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/acl_types.json b/configuration/etl/etl_tables.d/acls/acl_types.json
@@ -34,6 +34,12 @@
         ],
         "type": "BTREE",
         "is_unique": true
+      },
+      {
+        "name": "idx_module_id",
+        "columns": [
+          "module_id"
+        ]
       }
     ],
     "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/acls.json b/configuration/etl/etl_tables.d/acls/acls.json
@@ -54,6 +54,12 @@
         ],
         "type": "BTREE",
         "is_unique": true
+      },
+      {
+        "name": "idx_type_id",
+        "columns": [
+          "acl_type_id"
+        ]
       }
     ],
     "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/group_bys.json b/configuration/etl/etl_tables.d/acls/group_bys.json
@@ -34,6 +34,18 @@
         ],
         "type": "BTREE",
         "is_unique": true
+      },
+      {
+        "name": "idx_module_id",
+        "columns": [
+          "module_id"
+        ]
+      },
+      {
+        "name": "idx_realm_id",
+        "columns": [
+          "realm_id"
+        ]
       }
     ],
     "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/hierarchies.json b/configuration/etl/etl_tables.d/acls/hierarchies.json
@@ -34,6 +34,12 @@
         ],
         "type": "BTREE",
         "is_unique": true
+      },
+      {
+        "name": "idx_module_id",
+        "columns": [
+          "module_id"
+        ]
       }
     ],
     "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/realms.json b/configuration/etl/etl_tables.d/acls/realms.json
@@ -34,6 +34,12 @@
         ],
         "type": "BTREE",
         "is_unique": true
+      },
+      {
+        "name": "fk_r_module_id",
+        "columns": [
+          "module_id"
+        ]
       }
     ],
     "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/statistics.json b/configuration/etl/etl_tables.d/acls/statistics.json
@@ -34,6 +34,18 @@
         ],
         "type": "BTREE",
         "is_unique": true
+      },
+      {
+        "name": "idx_module_id",
+        "columns": [
+          "module_id"
+        ]
+      },
+      {
+        "name": "idx_realm_id",
+        "columns": [
+          "realm_id"
+        ]
       }
     ],
     "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/tabs.json b/configuration/etl/etl_tables.d/acls/tabs.json
@@ -33,6 +33,18 @@
                 ],
                 "type": "BTREE",
                 "is_unique": true
+            },
+            {
+                "name": "idx_module_id",
+                "columns": [
+                    "module_id"
+                ]
+            },
+            {
+                "name": "idx_parent_tab_id",
+                "columns": [
+                    "parent_tab_id"
+                ]
             }
         ],
         "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/user_acl_group_by_parameters.json b/configuration/etl/etl_tables.d/acls/user_acl_group_by_parameters.json
@@ -39,6 +39,24 @@
         ],
         "type": "BTREE",
         "is_unique": true
+      },
+      {
+        "name": "idx_user_id",
+        "columns": [
+          "user_id"
+        ]
+      },
+      {
+        "name": "idx_acl_id",
+        "columns": [
+          "acl_id"
+        ]
+      },
+      {
+        "name": "idx_group_by_id",
+        "columns": [
+          "group_by_id"
+        ]
       }
     ],
     "foreign_key_constraints": [

diff --git a/configuration/etl/etl_tables.d/acls/user_acls.json b/configuration/etl/etl_tables.d/acls/user_acls.json
@@ -29,6 +29,18 @@
         ],
         "type": "BTREE",
         "is_unique": true
+      },
+      {
+        "name": "idx_user_id",
+        "columns": [
+          "user_id"
+        ]
+      },
+      {
+        "name": "idx_acl_id",
+        "columns": [
+          "acl_id"
+        ]
       }
     ],
     "foreign_key_constraints": [

diff --git a/open_xdmod/modules/xdmod/tests/lib/ETL/DbModel/DbModelTest.php b/open_xdmod/modules/xdmod/tests/lib/ETL/DbModel/DbModelTest.php
@@ -140,6 +140,37 @@ public function testTableVerificationError()
         $table->verify();
     }
 
+    /**
+     * Test foreign key constraint verification error
+     *
+     * @expectedException Exception
+     * @expectedExceptionMessage Columns in foreign key constraint 'invalid_fk' must be contained at the beginning of an index
+     */
+
+    public function testForeignKeyConstraintVerificationError()
+    {
+        // Foreign key constraint with no corresponding index.
+        $config = (object) array(
+            'name' => "fk_verification_error",
+            'columns' => array( (object) array(
+                'name' => 'column1',
+                'type' => 'int(11)',
+                'nullable' => true,
+            )),
+            'foreign_key_constraints' => array( (object) array(
+                'name' => 'invalid_fk',
+                'columns' => array('column1'),
+                'referenced_table' => 'other_table',
+                'referenced_columns' => array(
+                    'other_column',
+                ),
+            )),
+        );
+
+        $table = new Table($config);  // No logger here
+        $table->verify();
+    }
+
     /**
      * Verify creating table elements manually.
      */
@@ -246,6 +277,10 @@ public function testAlterTable()
         $expected = trim(file_get_contents($file));
         $this->assertEquals($expected, $generated, sprintf("%s(): %s", __FUNCTION__, $file));
 
+        // The table should generate no SQL if there are no changes.
+        $generated = $currentTable->getAlterSql($currentTable);
+        $this->assertFalse($generated, sprintf("%s(): Expected false (no SQL)", __FUNCTION__));
+
         // Alter the table by manually adding columns, index, and trigger.
 
         $config = (object) array(