From 2f71e2e7c9943cda3a34db9760c7a87a40f5cbfc Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Wed, 4 Aug 2021 20:21:20 +0530 Subject: [PATCH] server: fine tune cron triggers behaviour in replace metadata API call https://github.com/hasura/graphql-engine-mono/pull/1991 GitOrigin-RevId: 7eb7d7b20d0a03eda7829d3a17a35dffe0f7bf1a --- CHANGELOG.md | 1 + .../Data/HashMap/Strict/InsOrd/Extended.hs | 14 ++- server/src-lib/Hasura/RQL/DDL/Metadata.hs | 104 ++++++++++++------ 3 files changed, 85 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d05dc5e9856..4b603c2b7fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Next release (Add entries below in the order of server, console, cli, docs, others) +- server: preserve unchanged cron triggers in `replace_metadata` API - server: fix inherited roles bug where mutations were not accessible when inherited roles was enabled - server: reintroduce the unique name constraint in allowed lists - server: fix http-log bug where batches with only one request emitted the parameterised query hash as a string instead of in a singleton array diff --git a/server/src-lib/Data/HashMap/Strict/InsOrd/Extended.hs b/server/src-lib/Data/HashMap/Strict/InsOrd/Extended.hs index dd6f28e243d..2b99aa255fd 100644 --- a/server/src-lib/Data/HashMap/Strict/InsOrd/Extended.hs +++ b/server/src-lib/Data/HashMap/Strict/InsOrd/Extended.hs @@ -2,15 +2,16 @@ module Data.HashMap.Strict.InsOrd.Extended ( module OMap , groupTuples , groupListWith + , partition ) where import Data.HashMap.Strict.InsOrd as OMap -import qualified Data.Sequence.NonEmpty as NE import qualified Data.List as L +import qualified Data.Sequence.NonEmpty as NE import Data.Hashable (Hashable) -import Prelude (Eq, Foldable, Functor, flip, fmap, ($), (<>)) +import Prelude groupTuples :: (Eq k, Hashable k, Foldable t) @@ -26,3 +27,12 @@ groupListWith => (v -> k) -> t v -> OMap.InsOrdHashMap k (NE.NESeq v) groupListWith f l = groupTuples $ fmap (\v -> (f v, v)) l + +partition :: (Eq k, Hashable k) => (v -> Bool) -> OMap.InsOrdHashMap k v -> (OMap.InsOrdHashMap k v, OMap.InsOrdHashMap k v) +partition predicate = + OMap.foldlWithKey' + (\(left, right) key val -> + if (predicate val) + then (OMap.insert key val left, right) + else (left, OMap.insert key val right)) + (mempty, mempty) diff --git a/server/src-lib/Hasura/RQL/DDL/Metadata.hs b/server/src-lib/Hasura/RQL/DDL/Metadata.hs index cfc0081ed4f..7e3e67f625b 100644 --- a/server/src-lib/Hasura/RQL/DDL/Metadata.hs +++ b/server/src-lib/Hasura/RQL/DDL/Metadata.hs @@ -19,18 +19,19 @@ module Hasura.RQL.DDL.Metadata import Hasura.Prelude -import qualified Data.Aeson.Ordered as AO -import qualified Data.HashMap.Strict.InsOrd as OMap -import qualified Data.HashSet as HS -import qualified Data.List as L +import qualified Data.Aeson.Ordered as AO +import qualified Data.HashMap.Strict as Map +import qualified Data.HashMap.Strict.InsOrd.Extended as OMap +import qualified Data.HashSet as HS +import qualified Data.List as L -import Control.Lens ((.~), (^?)) +import Control.Lens ((.~), (^?)) import Data.Aeson -import Data.Text.Extended ((<<>)) +import Data.Text.Extended ((<<>)) -import qualified Hasura.SQL.AnyBackend as AB +import qualified Hasura.SQL.AnyBackend as AB -import Hasura.Backends.Postgres.DDL.Table (delTriggerQ) +import Hasura.Backends.Postgres.DDL.Table (delTriggerQ) import Hasura.Metadata.Class import Hasura.RQL.DDL.Action import Hasura.RQL.DDL.ComputedField @@ -49,7 +50,7 @@ import Hasura.Base.Error import Hasura.EncJSON import Hasura.RQL.DDL.Metadata.Types import Hasura.RQL.Types -import Hasura.Server.Types (ExperimentalFeature (..)) +import Hasura.Server.Types (ExperimentalFeature (..)) runClearMetadata @@ -149,28 +150,11 @@ runReplaceMetadataV2 ReplaceMetadataV2{..} = do RMWithoutSources _ -> emptyQueryTagsConfig oldMetadata <- getMetadata - let (oldCronTriggersIncludedInMetadata, oldCronTriggersNotIncludedInMetadata) = - (OMap.filter ctIncludeInMetadata (_metaCronTriggers oldMetadata) - ,OMap.filter (not . ctIncludeInMetadata) (_metaCronTriggers oldMetadata)) - newCronTriggers = - case _rmv2Metadata of - RMWithoutSources m -> _mnsCronTriggers m - RMWithSources m -> _metaCronTriggers m - dropFutureCronEvents $ MetadataCronTriggers $ OMap.keys oldCronTriggersIncludedInMetadata - cronTriggers <- do - -- traverse over the new cron triggers and check if any of them - -- already exists as a cron trigger with "included_in_metadata: false" - for_ newCronTriggers $ \ct -> - when (ctName ct `OMap.member` oldCronTriggersNotIncludedInMetadata) $ - throw400 AlreadyExists $ - "cron trigger with name " - <> ctName ct - <<> " already exists as a cron trigger with \"included_in_metadata\" as false" - -- we add the old cron triggers with included_in_metadata set to false with the - -- newly added cron triggers - pure $ newCronTriggers <> oldCronTriggersNotIncludedInMetadata + + (cronTriggersMetadata, cronTriggersToBeAdded) <- processCronTriggers oldMetadata + metadata <- case _rmv2Metadata of - RMWithSources m -> pure $ m { _metaCronTriggers = cronTriggers } + RMWithSources m -> pure $ m { _metaCronTriggers = cronTriggersMetadata } RMWithoutSources MetadataNoSources{..} -> do let maybeDefaultSourceMetadata = oldMetadata ^? metaSources.ix defaultSource.toSourceMetadata defaultSourceMetadata <- onNothing maybeDefaultSourceMetadata $ @@ -181,7 +165,7 @@ runReplaceMetadataV2 ReplaceMetadataV2{..} = do } pure $ Metadata (OMap.singleton defaultSource newDefaultSourceMetadata) _mnsRemoteSchemas _mnsQueryCollections _mnsAllowlist - _mnsCustomTypes _mnsActions cronTriggers (_metaRestEndpoints oldMetadata) + _mnsCustomTypes _mnsActions cronTriggersMetadata (_metaRestEndpoints oldMetadata) emptyApiLimit emptyMetricsConfig mempty introspectionDisabledRoles queryTagsConfig putMetadata metadata @@ -192,7 +176,7 @@ runReplaceMetadataV2 ReplaceMetadataV2{..} = do buildSchemaCacheStrict -- populate future cron events for all the new cron triggers that are imported - for_ newCronTriggers $ \CronTriggerMetadata {..} -> + for_ cronTriggersToBeAdded $ \CronTriggerMetadata {..} -> populateInitialCronTriggerEvents ctSchedule ctName -- See Note [Clear postgres schema for dropped triggers] @@ -203,6 +187,62 @@ runReplaceMetadataV2 ReplaceMetadataV2{..} = do getOnlyPGSources :: Metadata -> InsOrdHashMap SourceName (SourceMetadata ('Postgres 'Vanilla)) getOnlyPGSources = OMap.mapMaybe AB.unpackAnyBackend . _metaSources + {- Note [Cron triggers behaviour with replace metadata] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + + When the metadata is replaced, we delete only the cron triggers + that were deleted, instead of deleting all the old cron triggers (which + existed in the metadata before it was replaced) and inserting all the + new cron triggers. This is done this way, because when a cron trigger is + dropped, the cron events associated with it will also be dropped from the DB + and when a new cron trigger is added, new cron events are generated by the + graphql-engine. So, this way we only delete and insert the data which has been changed. + + The cron triggers that were deleted is calculated by getting a diff + of the old cron triggers and the new cron triggers. Note that we don't just + check the name of the trigger to calculate the diff, the whole cron trigger + definition is considered in the calculation. + + Note: Only cron triggers with `include_in_metadata` set to `true` can be updated/deleted + via the replace metadata API. Cron triggers with `include_in_metadata` can only be modified + via the `create_cron_trigger` and `delete_cron_trigger` APIs. + + -} + processCronTriggers oldMetadata = do + let (oldCronTriggersIncludedInMetadata, oldCronTriggersNotIncludedInMetadata) = + OMap.partition ctIncludeInMetadata (_metaCronTriggers oldMetadata) + allNewCronTriggers = + case _rmv2Metadata of + RMWithoutSources m -> _mnsCronTriggers m + RMWithSources m -> _metaCronTriggers m + -- this function is intended to use with `Map.differenceWith`, it's used when two + -- equal keys are encountered, then the values are compared to calculate the diff. + -- see https://hackage.haskell.org/package/unordered-containers-0.2.14.0/docs/Data-HashMap-Internal.html#v:differenceWith + leftIfDifferent l r + | l == r = Nothing + | otherwise = Just l + cronTriggersToBeAdded = Map.differenceWith leftIfDifferent + (OMap.toHashMap allNewCronTriggers) + (OMap.toHashMap oldCronTriggersIncludedInMetadata) + cronTriggersToBeDropped = Map.differenceWith leftIfDifferent + (OMap.toHashMap oldCronTriggersIncludedInMetadata) + (OMap.toHashMap allNewCronTriggers) + dropFutureCronEvents $ MetadataCronTriggers $ Map.keys cronTriggersToBeDropped + cronTriggers <- do + -- traverse over the new cron triggers and check if any of them + -- already exists as a cron trigger with "included_in_metadata: false" + for_ allNewCronTriggers $ \ct -> + when (ctName ct `OMap.member` oldCronTriggersNotIncludedInMetadata) $ + throw400 AlreadyExists $ + "cron trigger with name " + <> ctName ct + <<> " already exists as a cron trigger with \"included_in_metadata\" as false" + -- we add the old cron triggers with included_in_metadata set to false with the + -- newly added cron triggers + pure $ allNewCronTriggers <> oldCronTriggersNotIncludedInMetadata + pure $ (cronTriggers, cronTriggersToBeAdded) + + dropPostgresTriggers :: InsOrdHashMap SourceName (SourceMetadata ('Postgres 'Vanilla)) -- ^ old pg sources -> InsOrdHashMap SourceName (SourceMetadata ('Postgres 'Vanilla)) -- ^ new pg sources