better change detection when altering foreign key constraints (fix #2060) (#2252)

foreign keys are considered to be dropped only if constraint oid changes

console/src/components/Services/Data/TableModify/ModifyActions.js

@@ -263,63 +263,89 @@ const saveForeignKeys = (index, tableSchema, columns) => {
     }
     const lcols = filteredMappings.map(cm => `"${columns[cm.column].name}"`);
     const rcols = filteredMappings.map(cm => `"${cm.refColumn}"`);
-    const migrationUp = [];
-    if (constraintName) {
-      migrationUp.push({
-        type: 'run_sql',
-        args: {
-          sql: `alter table "${schemaName}"."${tableName}" drop constraint "${constraintName}";`,
-        },
-      });
-    }
 
+    const migrationUp = [];
     const generatedConstraintName = generateFKConstraintName(
       tableName,
       lcols,
       tableSchema.foreign_key_constraints
     );
 
-    migrationUp.push({
-      type: 'run_sql',
-      args: {
-        sql: `alter table "${schemaName}"."${tableName}" add constraint "${constraintName ||
-          generatedConstraintName}" foreign key (${lcols.join(
-          ', '
-        )}) references "${refSchemaName}"."${refTableName}"(${rcols.join(
-          ', '
-        )}) on update ${onUpdate} on delete ${onDelete};`,
-      },
-    });
-    const migrationDown = [
-      {
-        type: 'run_sql',
-        args: {
-          sql: `alter table "${schemaName}"."${tableName}" drop constraint "${constraintName ||
-            generatedConstraintName}";`,
-        },
-      },
-    ];
     if (constraintName) {
-      const oldConstraint = tableSchema.foreign_key_constraints[index];
-      migrationDown.push({
+      // foreign key already exists, alter the foreign key
+      const migrationUpAlterFKeySql = `
+             alter table "${schemaName}"."${tableName}" drop constraint "${constraintName}",
+             add constraint "${generatedConstraintName}" 
+             foreign key (${lcols.join(', ')}) 
+             references "${refSchemaName}"."${refTableName}"
+             (${rcols.join(', ')}) on update ${onUpdate} on delete ${onDelete};
+      `;
+
+      migrationUp.push({
         type: 'run_sql',
         args: {
-          sql: `alter table "${schemaName}"."${tableName}" add constraint "${constraintName}" foreign key (${Object.keys(
-            oldConstraint.column_mapping
-          )
-            .map(lc => `"${lc}"`)
-            .join(', ')}) references "${
-            oldConstraint.ref_table_table_schema
-          }"."${oldConstraint.ref_table}"(${Object.values(
-            oldConstraint.column_mapping
-          )
-            .map(rc => `"${rc}"`)
-            .join(', ')}) on update ${
-            pgConfTypes[oldConstraint.on_update]
-          } on delete ${pgConfTypes[oldConstraint.on_delete]};`,
+          sql: migrationUpAlterFKeySql,
+        },
+      });
+    } else {
+      // foreign key not found, create a new one
+      const migrationUpCreateFKeySql = `
+           alter table "${schemaName}"."${tableName}"
+           add constraint "${generatedConstraintName}" 
+           foreign key (${lcols.join(', ')}) 
+           references "${refSchemaName}"."${refTableName}"
+           (${rcols.join(', ')}) on update ${onUpdate} on delete ${onDelete};
+      `;
+
+      migrationUp.push({
+        type: 'run_sql',
+        args: {
+          sql: migrationUpCreateFKeySql,
         },
       });
     }
+
+    const migrationDown = [];
+
+    if (constraintName) {
+      // when foreign key is altered
+      const oldConstraint = tableSchema.foreign_key_constraints[index];
+      const migrationDownAlterFKeySql = `
+          alter table "${schemaName}"."${tableName}" drop constraint "${generatedConstraintName}",
+          add constraint "${constraintName}" 
+          foreign key (${Object.keys(oldConstraint.column_mapping)
+    .map(lc => `"${lc}"`)
+    .join(', ')}) 
+          references "${oldConstraint.ref_table_table_schema}"."${
+  oldConstraint.ref_table
+}"
+          (${Object.values(oldConstraint.column_mapping)
+    .map(rc => `"${rc}"`)
+    .join(', ')}) 
+          on update ${pgConfTypes[oldConstraint.on_update]}
+          on delete ${pgConfTypes[oldConstraint.on_delete]};
+        `;
+
+      migrationDown.push({
+        type: 'run_sql',
+        args: {
+          sql: migrationDownAlterFKeySql,
+        },
+      });
+    } else {
+      // when foreign key is created
+      const migrationDownDeleteFKeySql = `
+          alter table "${schemaName}"."${tableName}" drop constraint "${generatedConstraintName}"
+      `;
+
+      migrationDown.push({
+        type: 'run_sql',
+        args: {
+          sql: migrationDownDeleteFKeySql,
+        },
+      });
+    }
+
     const migrationName = `set_fk_${schemaName}_${tableName}_${lcols.join(
       '_'
     )}`;

server/src-lib/Hasura/RQL/DDL/Relationship.hs

@@ -23,7 +23,6 @@ import           Hasura.RQL.DDL.Deps
 import           Hasura.RQL.DDL.Permission         (purgePerm)
 import           Hasura.RQL.DDL.Relationship.Types
 import           Hasura.RQL.Types
-import           Hasura.RQL.Types.Catalog
 import           Hasura.SQL.Types
 
 import           Data.Aeson.Types
@@ -106,7 +105,7 @@ createObjRelP1 (WithTable qt rd) = do
 
 objRelP2Setup
   :: (QErrM m, CacheRWM m)
-  => QualifiedTable -> HS.HashSet CatalogFKey -> RelDef ObjRelUsing -> m ()
+  => QualifiedTable -> HS.HashSet ForeignKey -> RelDef ObjRelUsing -> m ()
 objRelP2Setup qt fkeys (RelDef rn ru _) = do
   (relInfo, deps) <- case ru of
     RUManual (ObjRelManualConfig rm) -> do
@@ -117,8 +116,8 @@ objRelP2Setup qt fkeys (RelDef rn ru _) = do
       return (RelInfo rn ObjRel (zip lCols rCols) refqt True, deps)
     RUFKeyOn cn -> do
       -- TODO: validation should account for this too
-      CatalogFKey _ refqt consName colMap <-
-        getRequiredFkey cn fkeys $ \fk -> _cfkTable fk == qt
+      ForeignKey _ refqt _ consName colMap <-
+        getRequiredFkey cn fkeys $ \fk -> _fkTable fk == qt
 
       let deps = [ SchemaDependency (SOTableObj qt $ TOCons consName) "fkey"
                  , SchemaDependency (SOTableObj qt $ TOCol cn) "using_col"
@@ -181,7 +180,7 @@ validateArrRel qt (RelDef rn ru _) = do
 
 arrRelP2Setup
   :: (QErrM m, CacheRWM m)
-  => QualifiedTable -> HS.HashSet CatalogFKey -> ArrRelDef -> m ()
+  => QualifiedTable -> HS.HashSet ForeignKey -> ArrRelDef -> m ()
 arrRelP2Setup qt fkeys (RelDef rn ru _) = do
   (relInfo, deps) <- case ru of
     RUManual (ArrRelManualConfig rm) -> do
@@ -192,8 +191,8 @@ arrRelP2Setup qt fkeys (RelDef rn ru _) = do
       return (RelInfo rn ArrRel (zip lCols rCols) refqt True, deps)
     RUFKeyOn (ArrRelUsingFKeyOn refqt refCol) -> do
       -- TODO: validation should account for this too
-      CatalogFKey _ _ consName colMap <- getRequiredFkey refCol fkeys $
-        \fk -> _cfkTable fk == refqt && _cfkRefTable fk == qt
+      ForeignKey _ _ _ consName colMap <- getRequiredFkey refCol fkeys $
+        \fk -> _fkTable fk == refqt && _fkRefTable fk == qt
       let deps = [ SchemaDependency (SOTableObj refqt $ TOCons consName) "remote_fkey"
                  , SchemaDependency (SOTableObj refqt $ TOCol refCol) "using_col"
                  -- we don't need to necessarily track the remote table like we did in
@@ -312,9 +311,9 @@ setRelComment (SetRelComment (QualifiedObject sn tn) rn comment) =
 getRequiredFkey
   :: (QErrM m)
   => PGCol
-  -> HS.HashSet CatalogFKey
-  -> (CatalogFKey -> Bool)
-  -> m CatalogFKey
+  -> HS.HashSet ForeignKey
+  -> (ForeignKey -> Bool)
+  -> m ForeignKey
 getRequiredFkey col fkeySet preCondition =
   case filterFkeys of
     []  -> throw400 ConstraintError
@@ -324,32 +323,34 @@ getRequiredFkey col fkeySet preCondition =
            "more than one foreign key constraint exists on the given column"
   where
     filterFkeys = HS.toList $ HS.filter filterFn fkeySet
-    filterFn k = preCondition k && HM.keys (_cfkColumnMapping k) == [col]
+    filterFn k = preCondition k && HM.keys (_fkColumnMapping k) == [col]
 
-fetchTableFkeys :: QualifiedTable -> Q.TxE QErr (HS.HashSet CatalogFKey)
+fetchTableFkeys :: QualifiedTable -> Q.TxE QErr (HS.HashSet ForeignKey)
 fetchTableFkeys qt@(QualifiedObject sn tn) = do
   r <- Q.listQE defaultTxErrorHandler [Q.sql|
           SELECT f.constraint_name,
                  f.ref_table_table_schema,
                  f.ref_table,
+                 f.constraint_oid,
                  f.column_mapping
             FROM hdb_catalog.hdb_foreign_key_constraint f
            WHERE f.table_schema = $1 AND f.table_name = $2
           |] (sn, tn) True
   fmap HS.fromList $
-    forM r $ \(constr, refsn, reftn, Q.AltJ colMapping) ->
-    return $ CatalogFKey qt (QualifiedObject refsn reftn) constr colMapping
+    forM r $ \(constr, refsn, reftn, oid, Q.AltJ colMapping) ->
+    return $ ForeignKey qt (QualifiedObject refsn reftn) oid constr colMapping
 
-fetchFkeysAsRemoteTable :: QualifiedTable -> Q.TxE QErr (HS.HashSet CatalogFKey)
+fetchFkeysAsRemoteTable :: QualifiedTable -> Q.TxE QErr (HS.HashSet ForeignKey)
 fetchFkeysAsRemoteTable rqt@(QualifiedObject rsn rtn) = do
   r <- Q.listQE defaultTxErrorHandler [Q.sql|
           SELECT f.table_schema,
                  f.table_name,
                  f.constraint_name,
+                 f.constraint_oid,
                  f.column_mapping
             FROM hdb_catalog.hdb_foreign_key_constraint f
            WHERE f.ref_table_table_schema = $1 AND f.ref_table = $2
           |] (rsn, rtn) True
   fmap HS.fromList $
-    forM r $ \(sn, tn, constr, Q.AltJ colMapping) ->
-    return $ CatalogFKey (QualifiedObject sn tn) rqt constr colMapping
+    forM r $ \(sn, tn, constr, oid, Q.AltJ colMapping) ->
+    return $ ForeignKey (QualifiedObject sn tn) rqt oid constr colMapping

server/src-lib/Hasura/RQL/DDL/Schema/Diff.hs

@@ -61,64 +61,15 @@ data TableMeta
   , tmTable       :: !QualifiedTable
   , tmColumns     :: ![PGColMeta]
   , tmConstraints :: ![ConstraintMeta]
+  , tmForeignKeys :: ![ForeignKey]
   } deriving (Show, Eq)
 
 fetchTableMeta :: Q.Tx [TableMeta]
 fetchTableMeta = do
-  res <- Q.listQ [Q.sql|
-    SELECT
-        t.table_schema,
-        t.table_name,
-        t.table_oid,
-        coalesce(c.columns, '[]') as columns,
-        coalesce(f.constraints, '[]') as constraints
-    FROM
-        (SELECT
-             c.oid as table_oid,
-             c.relname as table_name,
-             n.nspname as table_schema
-         FROM
-             pg_catalog.pg_class c
-         JOIN
-             pg_catalog.pg_namespace as n
-           ON
-             c.relnamespace = n.oid
-        ) t
-        LEFT OUTER JOIN
-        (SELECT
-             table_schema,
-             table_name,
-             json_agg((SELECT r FROM (SELECT column_name, udt_name AS data_type, ordinal_position, is_nullable::boolean) r)) as columns
-         FROM
-             information_schema.columns
-         GROUP BY
-             table_schema, table_name) c
-        ON (t.table_schema = c.table_schema AND t.table_name = c.table_name)
-        LEFT OUTER JOIN
-        (SELECT
-             tc.table_schema,
-             tc.table_name,
-             json_agg(
-              json_build_object(
-                  'name', tc.constraint_name,
-                  'oid', r.oid::integer,
-                  'type', tc.constraint_type
-                  )
-             ) as constraints
-         FROM
-             information_schema.table_constraints tc
-             JOIN pg_catalog.pg_constraint r
-             ON tc.constraint_name = r.conname
-         GROUP BY
-             table_schema, table_name) f
-        ON (t.table_schema = f.table_schema AND t.table_name = f.table_name)
-    WHERE
-        t.table_schema NOT LIKE 'pg_%'
-        AND t.table_schema <> 'information_schema'
-        AND t.table_schema <> 'hdb_catalog'
-                |] () False
-  forM res $ \(ts, tn, toid, cols, constrnts) ->
-    return $ TableMeta toid (QualifiedObject ts tn) (Q.getAltJ cols) (Q.getAltJ constrnts)
+  res <- Q.listQ $(Q.sqlFromFile "src-rsr/table_meta.sql") () False
+  forM res $ \(ts, tn, toid, cols, constrnts, fkeys) ->
+    return $ TableMeta toid (QualifiedObject ts tn) (Q.getAltJ cols)
+             (Q.getAltJ constrnts) (Q.getAltJ fkeys)
 
 getOverlap :: (Eq k, Hashable k) => (v -> k) -> [v] -> [v] -> [(v, v)]
 getOverlap getKey left right =
@@ -171,9 +122,18 @@ getTableDiff oldtm newtm =
     alteredCols =
       flip map (filter (uncurry (/=)) existingCols) $ pcmToPci *** pcmToPci
 
-    droppedFKeyConstraints = map cmName $
-      filter (isForeignKey . cmType) $ getDifference cmOid
-      (tmConstraints oldtm) (tmConstraints newtm)
+    -- foreign keys are considered dropped only if their oid
+    -- and (ref-table, column mapping) are changed
+    droppedFKeyConstraints = map _fkConstraint $ HS.toList $
+      droppedFKeysWithOid `HS.intersection` droppedFKeysWithUniq
+
+    droppedFKeysWithOid = HS.fromList $
+      getDifference _fkOid (tmForeignKeys oldtm) (tmForeignKeys newtm)
+
+    droppedFKeysWithUniq = HS.fromList $
+      getDifference mkFKeyUniqId (tmForeignKeys oldtm) (tmForeignKeys newtm)
+
+    mkFKeyUniqId (ForeignKey _ reftn _ _ colMap) = (reftn, colMap)
 
 getTableChangeDeps
   :: (QErrM m, CacheRWM m)

server/src-lib/Hasura/RQL/Types/Catalog.hs

@@ -15,8 +15,6 @@ import           Data.Aeson
 import           Data.Aeson.Casing
 import           Data.Aeson.TH
 
-import qualified Data.HashMap.Strict              as HM
-
 data CatalogTable
   = CatalogTable
   { _ctTable         :: !QualifiedTable
@@ -67,19 +65,6 @@ data CatalogFunction
   } deriving (Show, Eq)
 $(deriveJSON (aesonDrop 3 snakeCase) ''CatalogFunction)
 
-type ColMapping = HM.HashMap PGCol PGCol
-
-data CatalogFKey
-  = CatalogFKey
-  { _cfkTable         :: !QualifiedTable
-  , _cfkRefTable      :: !QualifiedTable
-  , _cfkConstraint    :: !ConstraintName
-  , _cfkColumnMapping :: !ColMapping
-  } deriving (Show, Eq, Generic)
-$(deriveJSON (aesonDrop 4 snakeCase) ''CatalogFKey)
-
-instance Hashable CatalogFKey
-
 data CatalogMetadata
   = CatalogMetadata
   { _cmTables               :: ![CatalogTable]
@@ -89,7 +74,7 @@ data CatalogMetadata
   , _cmEventTriggers        :: ![CatalogEventTrigger]
   , _cmRemoteSchemas        :: ![AddRemoteSchemaQuery]
   , _cmFunctions            :: ![CatalogFunction]
-  , _cmForeignKeys          :: ![CatalogFKey]
+  , _cmForeignKeys          :: ![ForeignKey]
   , _cmAllowlistCollections :: ![CollectionDef]
   } deriving (Show, Eq)
 $(deriveJSON (aesonDrop 3 snakeCase) ''CatalogMetadata)

server/src-lib/Hasura/RQL/Types/Common.hs

@@ -16,6 +16,7 @@ module Hasura.RQL.Types.Common
        , WithTable(..)
        , ColVals
        , MutateResp(..)
+       , ForeignKey(..)
        ) where
 
 import           Hasura.Prelude
@@ -147,3 +148,17 @@ data MutateResp
   , _mrReturningColumns :: ![ColVals]
   } deriving (Show, Eq)
 $(deriveJSON (aesonDrop 3 snakeCase) ''MutateResp)
+
+type ColMapping = HM.HashMap PGCol PGCol
+
+data ForeignKey
+  = ForeignKey
+  { _fkTable         :: !QualifiedTable
+  , _fkRefTable      :: !QualifiedTable
+  , _fkOid           :: !Int
+  , _fkConstraint    :: !ConstraintName
+  , _fkColumnMapping :: !ColMapping
+  } deriving (Show, Eq, Generic)
+$(deriveJSON (aesonDrop 3 snakeCase) ''ForeignKey)
+
+instance Hashable ForeignKey

server/src-rsr/catalog_metadata.sql

@@ -179,6 +179,7 @@ from
               'schema', f.ref_table_table_schema,
               'name', f.ref_table
             ),
+            'oid', f.constraint_oid,
             'constraint', f.constraint_name,
             'column_mapping', f.column_mapping
           ) as info

server/src-rsr/table_meta.sql

@@ -0,0 +1,109 @@
+SELECT
+  t.table_schema,
+  t.table_name,
+  t.table_oid,
+  coalesce(c.columns, '[]') as columns,
+  coalesce(f.constraints, '[]') as constraints,
+  coalesce(fk.fkeys, '[]') as foreign_keys
+FROM
+  (
+    SELECT
+      c.oid as table_oid,
+      c.relname as table_name,
+      n.nspname as table_schema
+    FROM
+      pg_catalog.pg_class c
+      JOIN pg_catalog.pg_namespace as n ON c.relnamespace = n.oid
+  ) t
+  LEFT OUTER JOIN (
+    SELECT
+      table_schema,
+      table_name,
+      json_agg(
+        (
+          SELECT
+            r
+          FROM
+            (
+              SELECT
+                column_name,
+                udt_name AS data_type,
+                ordinal_position,
+                is_nullable :: boolean
+            ) r
+        )
+      ) as columns
+    FROM
+      information_schema.columns
+    GROUP BY
+      table_schema,
+      table_name
+  ) c ON (
+    t.table_schema = c.table_schema
+    AND t.table_name = c.table_name
+  )
+  LEFT OUTER JOIN (
+    SELECT
+      tc.table_schema,
+      tc.table_name,
+      json_agg(
+        json_build_object(
+          'name',
+          tc.constraint_name,
+          'oid',
+          r.oid :: integer,
+          'type',
+          tc.constraint_type
+        )
+      ) as constraints
+    FROM
+      information_schema.table_constraints tc
+      JOIN pg_catalog.pg_constraint r ON tc.constraint_name = r.conname
+    GROUP BY
+      table_schema,
+      table_name
+  ) f ON (
+    t.table_schema = f.table_schema
+    AND t.table_name = f.table_name
+  )
+  LEFT OUTER JOIN (
+    SELECT
+      f.table_schema,
+      f.table_name,
+      json_agg(
+        json_build_object(
+          'table',
+          json_build_object(
+            'schema',
+            f.table_schema,
+            'name',
+            f.table_name
+          ),
+          'ref_table',
+          json_build_object(
+            'schema',
+            f.ref_table_table_schema,
+            'name',
+            f.ref_table
+          ),
+          'oid',
+          f.constraint_oid,
+          'constraint',
+          f.constraint_name,
+          'column_mapping',
+          f.column_mapping
+        )
+      ) as fkeys
+    FROM
+      hdb_catalog.hdb_foreign_key_constraint f
+    GROUP BY
+      table_schema,
+      table_name
+  ) fk ON (
+    fk.table_schema = t.table_schema
+    AND fk.table_name = t.table_name
+  )
+WHERE
+  t.table_schema NOT LIKE 'pg_%'
+  AND t.table_schema <> 'information_schema'
+  AND t.table_schema <> 'hdb_catalog'

server/tests-py/queries/graphql_query/basic/nested_select_with_foreign_key_alter.yaml

@@ -0,0 +1,47 @@
+- description: Alter foreign key constraint on article table
+  url: /v1/query
+  status: 200
+  query:
+    type: run_sql
+    args:
+      sql: |
+        ALTER TABLE article DROP CONSTRAINT article_author_id_fkey,
+          ADD CONSTRAINT article_author_id_fkey FOREIGN KEY (author_id) REFERENCES author(id);
+
+- description: Nested select on article
+  url: /v1/graphql
+  status: 200
+  response:
+    data:
+      article:
+      - id: 1
+        title: Article 1
+        content: Sample article content 1
+        author:
+          id: 1
+          name: Author 1
+      - id: 2
+        title: Article 2
+        content: Sample article content 2
+        author:
+          id: 1
+          name: Author 1
+      - id: 3
+        title: Article 3
+        content: Sample article content 3
+        author:
+          id: 2
+          name: Author 2
+  query:
+    query: |
+      query {
+        article {
+          id
+          title
+          content
+          author {
+            id
+            name
+          }
+        }
+      }

server/tests-py/test_graphql_queries.py

@@ -46,6 +46,10 @@ class TestGraphQLQueryBasic(DefaultTestSelectQueries):
     def test_select_query_user_col_change(self, hge_ctx, transport):
         check_query_f(hge_ctx, self.dir() + "/select_query_user_col_change.yaml")
 
+    def test_nested_select_with_foreign_key_alter(self, hge_ctx, transport):
+        transport = 'http'
+        check_query_f(hge_ctx, self.dir() + "/nested_select_with_foreign_key_alter.yaml", transport)
+
     @classmethod
     def dir(cls):
         return 'queries/graphql_query/basic'