From f09d0cf56ae9abd6d271d52431e31c4e47f4304e Mon Sep 17 00:00:00 2001 From: Alexis King Date: Thu, 5 Nov 2020 17:46:01 -0600 Subject: [PATCH 1/4] Fix event trigger cleanup on deletion via replace_metadata (fix #5461) --- CHANGELOG.md | 1 + server/src-lib/Hasura/RQL/DDL/EventTrigger.hs | 52 ++++++++++---- server/src-lib/Hasura/RQL/DDL/Metadata.hs | 15 ++-- server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs | 2 +- server/src-rsr/catalog_version.txt | 2 +- server/src-rsr/initialise.sql | 4 +- server/src-rsr/migrations/41_to_42.sql | 2 + server/src-rsr/migrations/42_to_41.sql | 5 ++ .../create-delete/create_and_reset.yaml | 68 +++++++++++++------ 9 files changed, 107 insertions(+), 44 deletions(-) create mode 100644 server/src-rsr/migrations/41_to_42.sql create mode 100644 server/src-rsr/migrations/42_to_41.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index f94fb10f0ed94..7bbf670314094 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -127,6 +127,7 @@ This release contains the [PDV refactor (#4111)](https://github.com/hasura/graph - server: fix issue with tracking custom functions that return `SETOF` materialized view (close #5294) (#5945) - server: introduce optional custom table name in table configuration to track the table according to the custom name. The `set_table_custom_fields` API has been deprecated, A new API `set_table_customization` has been added to set the configuration. (#3811) - server: allow remote relationships with union, interface and enum type fields as well (fixes #5875) (#6080) +- server: fix event trigger cleanup on deletion via replace_metadata (fix #5461) (#6137) - console: allow user to cascade Postgres dependencies when dropping Postgres objects (close #5109) (#5248) - console: mark inconsistent remote schemas in the UI (close #5093) (#5181) - console: remove ONLY as default for ALTER TABLE in column alter operations (close #5512) #5706 diff --git a/server/src-lib/Hasura/RQL/DDL/EventTrigger.hs b/server/src-lib/Hasura/RQL/DDL/EventTrigger.hs index b040da72f9f48..83f92ba46f643 100644 --- a/server/src-lib/Hasura/RQL/DDL/EventTrigger.hs +++ b/server/src-lib/Hasura/RQL/DDL/EventTrigger.hs @@ -9,19 +9,20 @@ module Hasura.RQL.DDL.EventTrigger -- TODO(from master): review , delEventTriggerFromCatalog - , subTableP2 - , subTableP2Setup + , mkEventTriggerInfo , mkAllTriggersQ , delTriggerQ , getEventTriggerDef , getWebhookInfoFromConf , getHeaderInfosFromConf , updateEventTriggerInCatalog + , replaceEventTriggersInCatalog ) where import Hasura.Prelude import qualified Data.Environment as Env +import qualified Data.HashMap.Strict.Extended as Map import qualified Data.Text as T import qualified Data.Text.Lazy as TL import qualified Database.PG.Query as Q @@ -213,13 +214,13 @@ subTableP1 (CreateEventTriggerQuery name qt insert update delete enableManual re SubCStar -> return () SubCArray pgcols -> forM_ pgcols (assertPGCol (_tciFieldInfoMap ti) "") -subTableP2Setup +mkEventTriggerInfo :: QErrM m => Env.Environment -> QualifiedTable -> EventTriggerConf -> m (EventTriggerInfo, [SchemaDependency]) -subTableP2Setup env qt (EventTriggerConf name def webhook webhookFromEnv rconf mheaders) = do +mkEventTriggerInfo env qt (EventTriggerConf name def webhook webhookFromEnv rconf mheaders) = do webhookConf <- case (webhook, webhookFromEnv) of (Just w, Nothing) -> return $ WCValue w (Nothing, Just wEnv) -> return $ WCEnv wEnv @@ -251,19 +252,14 @@ getTrigDefDeps qt (TriggerOpsDef mIns mUpd mDel _) = SubCStar -> [] SubCArray pgcols -> pgcols -subTableP2 - :: (MonadTx m) - => QualifiedTable -> Bool -> EventTriggerConf -> m () -subTableP2 qt replace etc = liftTx if replace - then updateEventTriggerInCatalog etc - else addEventTriggerToCatalog qt etc - runCreateEventTriggerQuery :: (QErrM m, UserInfoM m, CacheRWM m, MonadTx m) => CreateEventTriggerQuery -> m EncJSON runCreateEventTriggerQuery q = do (qt, replace, etc) <- subTableP1 q - subTableP2 qt replace etc + liftTx if replace + then updateEventTriggerInCatalog etc + else addEventTriggerToCatalog qt etc buildSchemaCacheFor $ MOTableObj qt (MTOTrigger $ etcName etc) return successMsg @@ -365,3 +361,35 @@ updateEventTriggerInCatalog trigConf = configuration = $1 WHERE name = $2 |] (Q.AltJ $ toJSON trigConf, etcName trigConf) True + +-- | Replaces /all/ event triggers in the catalog with new ones, taking care to +-- drop SQL trigger functions and archive events for any deleted event triggers. +-- +-- See Note [Diff-and-patch event triggers on replace] for more details. +replaceEventTriggersInCatalog + :: MonadTx m + => HashMap TriggerName (QualifiedTable, EventTriggerConf) + -> m () +replaceEventTriggersInCatalog triggerConfs = do + existingTriggers <- Map.fromListOn id <$> fetchExistingTriggers + liftTx $ for_ (align existingTriggers triggerConfs) \case + This triggerName -> delEventTriggerFromCatalog triggerName + That (tableName, triggerConf) -> addEventTriggerToCatalog tableName triggerConf + These _ (_, triggerConf) -> updateEventTriggerInCatalog triggerConf + where + fetchExistingTriggers = liftTx $ map runIdentity <$> + Q.listQE defaultTxErrorHandler + [Q.sql|SELECT name FROM hdb_catalog.event_triggers|] () True + +{- Note [Diff-and-patch event triggers on replace] +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +When executing a replace_metadata API call, we usually just drop everything in +the catalog and recreate it from scratch, then rebuild the schema cache. This +works fine for most things, but it’s a bad idea for event triggers, because +delEventTriggerFromCatalog does extra work: it deletes the SQL trigger functions +and archives all associated events. + +Therefore, we have to be more careful about which event triggers we drop. We +diff the new metadata against the old metadata, and we only drop triggers that +are actually absent in the new metadata. The replaceEventTriggersInCatalog +function implements this diff-and-patch operation. -} diff --git a/server/src-lib/Hasura/RQL/DDL/Metadata.hs b/server/src-lib/Hasura/RQL/DDL/Metadata.hs index 0a83932bac29d..c26cfc0dbafe7 100644 --- a/server/src-lib/Hasura/RQL/DDL/Metadata.hs +++ b/server/src-lib/Hasura/RQL/DDL/Metadata.hs @@ -15,6 +15,7 @@ module Hasura.RQL.DDL.Metadata import Hasura.Prelude import qualified Data.Aeson.Ordered as AO +import qualified Data.HashMap.Strict as HM import qualified Data.HashMap.Strict.InsOrd as HMIns import qualified Data.HashSet as HS import qualified Data.HashSet.InsOrd as HSIns @@ -36,7 +37,8 @@ import qualified Hasura.RQL.DDL.Schema as Schema import Hasura.Backends.Postgres.SQL.Types import Hasura.EncJSON import Hasura.RQL.DDL.ComputedField (dropComputedFieldFromCatalog) -import Hasura.RQL.DDL.EventTrigger (delEventTriggerFromCatalog, subTableP2) +import Hasura.RQL.DDL.EventTrigger (delEventTriggerFromCatalog, + replaceEventTriggersInCatalog) import Hasura.RQL.DDL.Metadata.Types import Hasura.RQL.DDL.Permission.Internal (dropPermFromCatalog) import Hasura.RQL.DDL.RemoteSchema (addRemoteSchemaToCatalog, @@ -49,10 +51,11 @@ import Hasura.RQL.Types -- | Purge all user-defined metadata; metadata with is_system_defined = false clearUserMetadata :: MonadTx m => m () clearUserMetadata = liftTx $ Q.catchE defaultTxErrorHandler $ do + -- Note: we don’t drop event triggers here because we update them a different + -- way; see Note [Diff-and-patch event triggers on replace] in Hasura.RQL.DDL.EventTrigger. Q.unitQ "DELETE FROM hdb_catalog.hdb_function WHERE is_system_defined <> 'true'" () False Q.unitQ "DELETE FROM hdb_catalog.hdb_permission WHERE is_system_defined <> 'true'" () False Q.unitQ "DELETE FROM hdb_catalog.hdb_relationship WHERE is_system_defined <> 'true'" () False - Q.unitQ "DELETE FROM hdb_catalog.event_triggers" () False Q.unitQ "DELETE FROM hdb_catalog.hdb_computed_field" () False Q.unitQ "DELETE FROM hdb_catalog.hdb_remote_relationship" () False Q.unitQ "DELETE FROM hdb_catalog.hdb_table WHERE is_system_defined <> 'true'" () False @@ -69,6 +72,7 @@ runClearMetadata => ClearMetadata -> m EncJSON runClearMetadata _ = do clearUserMetadata + replaceEventTriggersInCatalog mempty buildSchemaCacheStrict return successMsg @@ -110,9 +114,10 @@ saveMetadata (Metadata tables functions withPathK "update_permissions" $ processPerms _tmTable _tmUpdatePermissions withPathK "delete_permissions" $ processPerms _tmTable _tmDeletePermissions - -- Event triggers - withPathK "event_triggers" $ - indexedForM_ _tmEventTriggers $ \etc -> subTableP2 _tmTable False etc + -- Event triggers + let allEventTriggers = HMIns.elems tables & map \table -> + (_tmTable table,) <$> HMIns.toHashMap (_tmEventTriggers table) + replaceEventTriggersInCatalog $ HM.unions allEventTriggers -- sql functions withPathK "functions" $ indexedForM_ functions $ diff --git a/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs b/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs index ef3532cce233f..605a55ec23537 100644 --- a/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs +++ b/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs @@ -332,7 +332,7 @@ buildSchemaCacheRule env = proc (catalogMetadata, invalidationKeys) -> do (| withRecordInconsistency ( (| modifyErrA (do etc <- bindErrorA -< decodeValue configuration - (info, dependencies) <- bindErrorA -< subTableP2Setup env qt etc + (info, dependencies) <- bindErrorA -< mkEventTriggerInfo env qt etc let tableColumns = M.mapMaybe (^? _FIColumn) (_tciFieldInfoMap tableInfo) recreateViewIfNeeded -< (qt, tableColumns, trn, etcDefinition etc) recordDependencies -< (metadataObject, schemaObjectId, dependencies) diff --git a/server/src-rsr/catalog_version.txt b/server/src-rsr/catalog_version.txt index 87523dd7a0632..d81cc0710eb6c 100644 --- a/server/src-rsr/catalog_version.txt +++ b/server/src-rsr/catalog_version.txt @@ -1 +1 @@ -41 +42 diff --git a/server/src-rsr/initialise.sql b/server/src-rsr/initialise.sql index e194ae34a26b6..bb9772700b7ce 100644 --- a/server/src-rsr/initialise.sql +++ b/server/src-rsr/initialise.sql @@ -297,9 +297,7 @@ CREATE TABLE hdb_catalog.event_triggers schema_name TEXT NOT NULL, table_name TEXT NOT NULL, configuration JSON, - comment TEXT, - FOREIGN KEY (schema_name, table_name) - REFERENCES hdb_catalog.hdb_table(table_schema, table_name) ON UPDATE CASCADE + comment TEXT ); CREATE TABLE hdb_catalog.event_log diff --git a/server/src-rsr/migrations/41_to_42.sql b/server/src-rsr/migrations/41_to_42.sql new file mode 100644 index 0000000000000..33e6af6c1f752 --- /dev/null +++ b/server/src-rsr/migrations/41_to_42.sql @@ -0,0 +1,2 @@ +ALTER TABLE hdb_catalog.event_triggers + DROP CONSTRAINT event_triggers_schema_name_table_name_fkey; diff --git a/server/src-rsr/migrations/42_to_41.sql b/server/src-rsr/migrations/42_to_41.sql new file mode 100644 index 0000000000000..88db86cae8d3c --- /dev/null +++ b/server/src-rsr/migrations/42_to_41.sql @@ -0,0 +1,5 @@ +ALTER TABLE hdb_catalog.event_triggers + ADD CONSTRAINT event_triggers_schema_name_table_name_fkey + FOREIGN KEY (schema_name, table_name) + REFERENCES hdb_catalog.hdb_table(table_schema, table_name) + ON UPDATE CASCADE; diff --git a/server/tests-py/queries/event_triggers/create-delete/create_and_reset.yaml b/server/tests-py/queries/event_triggers/create-delete/create_and_reset.yaml index 981737f15386a..2c146f5dc3dff 100644 --- a/server/tests-py/queries/event_triggers/create-delete/create_and_reset.yaml +++ b/server/tests-py/queries/event_triggers/create-delete/create_and_reset.yaml @@ -1,26 +1,50 @@ -description: create an event trigger and then reset metadata -url: /v1/query -status: 200 -query: - type: bulk - args: - - type: track_table +- description: create an event trigger and then reset metadata + url: /v1/query + status: 200 + query: + type: bulk args: - schema: hge_tests - name: test_t1 - - type: create_event_trigger - args: &def_args - name: t1_1 - table: + - type: track_table + args: schema: hge_tests name: test_t1 - insert: - columns: "*" - update: - columns: "*" - delete: - columns: "*" - webhook: http://127.0.0.1:5592 + - type: create_event_trigger + args: &def_args + name: t1_1 + table: + schema: hge_tests + name: test_t1 + insert: + columns: "*" + update: + columns: "*" + delete: + columns: "*" + webhook: http://127.0.0.1:5592 + - type: insert + args: + table: + schema: hge_tests + name: test_t1 + objects: + - c1: 1 + c2: world + returning: [] + - type: clear_metadata + args: {} - - type: clear_metadata - args: {} +- description: ensure the event was archived + url: /v1/query + status: 200 + response: + - trigger_name: t1_1 + archived: true + query: + type: select + args: + table: + schema: hdb_catalog + name: event_log + columns: [trigger_name, archived] + order_by: ['-created_at'] + limit: 1 From 0c62360aef8488ecd1d9fd8ef14a6cb8025395e6 Mon Sep 17 00:00:00 2001 From: Tirumarai Selvan A Date: Tue, 10 Nov 2020 11:47:32 +0530 Subject: [PATCH 2/4] add trigger function to perform "ON UPDATE CASCADE" Previously, we were relying on "ON UPDATE CASCADE" on the foreign key constraint to do this automatically. --- server/src-rsr/initialise.sql | 21 +++++++++++++++++++++ server/src-rsr/migrations/41_to_42.sql | 21 +++++++++++++++++++++ server/src-rsr/migrations/42_to_41.sql | 4 ++++ 3 files changed, 46 insertions(+) diff --git a/server/src-rsr/initialise.sql b/server/src-rsr/initialise.sql index bb9772700b7ce..d289c684b3d87 100644 --- a/server/src-rsr/initialise.sql +++ b/server/src-rsr/initialise.sql @@ -300,6 +300,27 @@ CREATE TABLE hdb_catalog.event_triggers comment TEXT ); +-- since we do not have a foreign key constraint (with 'ON UPDATE CASCADE') with hdb_catalog.hdb_table +-- (see Note [Diff-and-patch event triggers on replace] in Hasura.RQL.DDL.EventTrigger), we perform the update using trigger +CREATE OR REPLACE FUNCTION hdb_catalog.event_trigger_table_name_update() +RETURNS TRIGGER +LANGUAGE PLPGSQL +AS +$$ +BEGIN + IF (NEW.table_schema, NEW.table_name) <> (OLD.table_schema, OLD.table_name) THEN + UPDATE hdb_catalog.event_triggers + SET schema_name = NEW.table_schema, table_name = NEW.table_name + WHERE (schema_name, table_name) = (OLD.table_schema, OLD.table_name); + END IF; + RETURN NEW; +END; +$$; + +CREATE TRIGGER event_trigger_table_name_update_trigger +AFTER UPDATE ON hdb_catalog.hdb_table +FOR EACH ROW EXECUTE PROCEDURE hdb_catalog.event_trigger_table_name_update(); + CREATE TABLE hdb_catalog.event_log ( id TEXT DEFAULT hdb_catalog.gen_hasura_uuid() PRIMARY KEY, diff --git a/server/src-rsr/migrations/41_to_42.sql b/server/src-rsr/migrations/41_to_42.sql index 33e6af6c1f752..926ba27888b68 100644 --- a/server/src-rsr/migrations/41_to_42.sql +++ b/server/src-rsr/migrations/41_to_42.sql @@ -1,2 +1,23 @@ ALTER TABLE hdb_catalog.event_triggers DROP CONSTRAINT event_triggers_schema_name_table_name_fkey; + +-- since we removed the foreign key constraint with hdb_catalog.hdb_table which had 'ON UPDATE CASCADE' +-- (see Note [Diff-and-patch event triggers on replace] in Hasura.RQL.DDL.EventTrigger), we perform the update using trigger +CREATE OR REPLACE FUNCTION hdb_catalog.event_trigger_table_name_update() +RETURNS TRIGGER +LANGUAGE PLPGSQL +AS +$$ +BEGIN + IF (NEW.table_schema, NEW.table_name) <> (OLD.table_schema, OLD.table_name) THEN + UPDATE hdb_catalog.event_triggers + SET schema_name = NEW.table_schema, table_name = NEW.table_name + WHERE (schema_name, table_name) = (OLD.table_schema, OLD.table_name); + END IF; + RETURN NEW; +END; +$$; + +CREATE TRIGGER event_trigger_table_name_update_trigger +AFTER UPDATE ON hdb_catalog.hdb_table +FOR EACH ROW EXECUTE PROCEDURE hdb_catalog.event_trigger_table_name_update(); diff --git a/server/src-rsr/migrations/42_to_41.sql b/server/src-rsr/migrations/42_to_41.sql index 88db86cae8d3c..a224232ffd966 100644 --- a/server/src-rsr/migrations/42_to_41.sql +++ b/server/src-rsr/migrations/42_to_41.sql @@ -1,3 +1,7 @@ +DROP TRIGGER event_trigger_table_name_update_trigger ON hdb_catalog.hdb_table; + +DROP FUNCTION hdb_catalog.event_trigger_table_name_update(); + ALTER TABLE hdb_catalog.event_triggers ADD CONSTRAINT event_triggers_schema_name_table_name_fkey FOREIGN KEY (schema_name, table_name) From 60556113f04c18ce01f8dafd018ab6eeaf841d16 Mon Sep 17 00:00:00 2001 From: Tirumarai Selvan A Date: Tue, 10 Nov 2020 15:09:18 +0530 Subject: [PATCH 3/4] account for different constraint name in PG < 12 --- server/src-rsr/migrations/41_to_42.sql | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src-rsr/migrations/41_to_42.sql b/server/src-rsr/migrations/41_to_42.sql index 926ba27888b68..c7c24702103af 100644 --- a/server/src-rsr/migrations/41_to_42.sql +++ b/server/src-rsr/migrations/41_to_42.sql @@ -1,5 +1,8 @@ ALTER TABLE hdb_catalog.event_triggers - DROP CONSTRAINT event_triggers_schema_name_table_name_fkey; + DROP CONSTRAINT IF EXISTS event_triggers_schema_name_fkey; + +ALTER TABLE hdb_catalog.event_triggers + DROP CONSTRAINT IF EXISTS event_triggers_schema_name_table_name_fkey; -- since we removed the foreign key constraint with hdb_catalog.hdb_table which had 'ON UPDATE CASCADE' -- (see Note [Diff-and-patch event triggers on replace] in Hasura.RQL.DDL.EventTrigger), we perform the update using trigger From c5a4bbfa254ab39a99e2615e49a39abf58dbfe53 Mon Sep 17 00:00:00 2001 From: Tirumarai Selvan A Date: Tue, 10 Nov 2020 15:40:17 +0530 Subject: [PATCH 4/4] no-op hlint --- server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs b/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs index 605a55ec23537..6c938f2aa3c28 100644 --- a/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs +++ b/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs @@ -487,9 +487,9 @@ withMetadataCheck cascade action = do sc <- askSchemaCache for_ alteredTables $ \(oldQtn, tableDiff) -> do - ti <- case M.lookup oldQtn $ scTables sc of - Just ti -> return ti - Nothing -> throw500 $ "old table metadata not found in cache : " <>> oldQtn + ti <- onNothing + (M.lookup oldQtn $ scTables sc) + (throw500 $ "old table metadata not found in cache : " <>> oldQtn) processTableChanges (_tiCoreInfo ti) tableDiff where SchemaDiff droppedTables alteredTables = schemaDiff