server: run_sql to not drop the SQL triggers created by the graphql-engine

PR-URL: https://github.com/hasura/graphql-engine-mono/pull/4397
GitOrigin-RevId: dcd43fc31f64af8c6c9c92c66d46a593d5b12fbd

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Bug fixes and improvements
 
+- server: don't drop the SQL triggers defined by the graphql-engine when DDL changes are made using the `run_sql` API
 - server: fixed a bug where timestamp values sent to postgres would erroneously trim leading zeroes (#8096)
 - server: fix bug when event triggers where defined on tables that contained non lower-case alphabet characters
 - server: avoid encoding 'varchar' values to UTF8 in MSSQL backends

server/graphql-engine.cabal

@@ -1018,6 +1018,7 @@ test-suite tests-hspec
     , http-client-tls
     , http-conduit
     , http-types
+    , HUnit
     , insert-ordered-containers
     , jose
     , kan-extensions
@@ -1081,6 +1082,7 @@ test-suite tests-hspec
     Harness.Http
     Harness.RemoteServer
     Harness.TestEnvironment
+    Harness.Webhook
     Harness.Yaml
 
  -- Harness.Backend
@@ -1106,6 +1108,7 @@ test-suite tests-hspec
     Test.ColumnPresetsSpec
     Test.CustomFieldNamesSpec
     Test.DirectivesSpec
+    Test.EventTriggersRunSQLSpec
     Test.HelloWorldSpec
     Test.InsertCheckPermissionSpec
     Test.InsertEnumColumnSpec

server/src-lib/Hasura/Backends/Postgres/DDL/RunSQL.hs

@@ -229,13 +229,8 @@ withMetadataCheck ::
 withMetadataCheck source cascade txAccess runSQLQuery = do
   SourceInfo _ tableCache functionCache sourceConfig _ _ <- askSourceInfo @('Postgres pgKind) source
 
-  let dropTriggersAndRunSQL = do
-        -- We need to drop existing event triggers so that no interference is caused to the sql query execution
-        dropExistingEventTriggers tableCache
-        runSQLQuery
-
   -- Run SQL query and metadata checker in a transaction
-  (queryResult, metadataUpdater) <- runTxWithMetadataCheck source sourceConfig txAccess tableCache functionCache cascade dropTriggersAndRunSQL
+  (queryResult, metadataUpdater) <- runTxWithMetadataCheck source sourceConfig txAccess tableCache functionCache cascade runSQLQuery
 
   -- Build schema cache with updated metadata
   withNewInconsistentObjsCheck $
@@ -248,12 +243,6 @@ withMetadataCheck source cascade txAccess runSQLQuery = do
 
   pure queryResult
   where
-    dropExistingEventTriggers :: TableCache ('Postgres pgKind) -> Q.TxET QErr m ()
-    dropExistingEventTriggers tableCache =
-      forM_ (M.elems tableCache) $ \tableInfo -> do
-        let eventTriggers = _tiEventTriggerInfoMap tableInfo
-        forM_ (M.keys eventTriggers) (liftTx . dropTriggerQ)
-
     recreateEventTriggers :: PGSourceConfig -> SchemaCache -> m ()
     recreateEventTriggers sourceConfig schemaCache = do
       let tables = fromMaybe mempty $ unsafeTableCache @('Postgres pgKind) source $ scSources schemaCache

server/tests-hspec/Harness/Http.hs

@@ -1,6 +1,7 @@
 -- | Helper functions for HTTP requests.
 module Harness.Http
   ( get_,
+    post,
     postValue,
     postValueWithStatus,
     healthCheck,
@@ -36,15 +37,19 @@ get_ url = do
 postValue :: HasCallStack => String -> Http.RequestHeaders -> Value -> IO Value
 postValue = postValueWithStatus 200
 
--- | Post the JSON to the given URL and expected HTTP response code.
--- Produces a very descriptive exception or failure.
-postValueWithStatus :: HasCallStack => Int -> String -> Http.RequestHeaders -> Value -> IO Value
-postValueWithStatus statusCode url headers value = do
+post :: String -> Http.RequestHeaders -> Value -> IO (Http.Response L8.ByteString)
+post url headers value = do
   let request =
         Http.setRequestHeaders headers $
           Http.setRequestMethod Http.methodPost $
             Http.setRequestBodyJSON value (fromString url)
-  response <- Http.httpLbs request
+  Http.httpLbs request
+
+-- | Post the JSON to the given URL and expected HTTP response code.
+-- Produces a very descriptive exception or failure.
+postValueWithStatus :: HasCallStack => Int -> String -> Http.RequestHeaders -> Value -> IO Value
+postValueWithStatus statusCode url headers value = do
+  response <- post url headers value
   let requestBodyString = L8.unpack $ encode value
       responseBodyString = L8.unpack $ Http.getResponseBody response
       responseStatusCode = Http.getResponseStatusCode response

server/tests-hspec/Harness/Webhook.hs

@@ -0,0 +1,67 @@
+-- | Functions to setup and run a dedicated webhook server
+module Harness.Webhook
+  ( run,
+    EventsQueue (..),
+  )
+where
+
+import Control.Concurrent (forkIO)
+import Control.Concurrent.Chan qualified as Chan
+import Control.Exception.Safe (bracket)
+import Control.Monad.IO.Class (liftIO)
+import Data.Aeson qualified as Aeson
+import Data.Parser.JSONPath (parseJSONPath)
+import Harness.Http qualified as Http
+import Harness.TestEnvironment (Server (..), serverUrl)
+import Hasura.Base.Error (iResultToMaybe)
+import Hasura.Prelude (fromMaybe)
+import Hasura.Server.Utils (executeJSONPath)
+import Network.Socket qualified as Socket
+import Network.Wai.Extended qualified as Wai
+import Network.Wai.Handler.Warp qualified as Warp
+import Web.Spock.Core qualified as Spock
+import Prelude
+
+newtype EventsQueue = EventsQueue (Chan.Chan Aeson.Value)
+
+-- | This function starts a new thread with a minimal server on the
+-- first available port. It returns the corresponding 'Server'.
+--
+-- This new server serves the following routes:
+--   - GET on @/@, which returns a simple 200 OK;
+--   - POST on @/echo@, which extracts the event data from the body
+--     of the request and inserts it into the `EventsQueue`.
+--
+-- This function performs a health check, using a GET on /, to ensure that the
+-- server was started correctly, and will throw an exception if the health check
+-- fails. This function does NOT attempt to kill the thread in such a case,
+-- which might result in a leak if the thread is still running but the server
+-- fails its health check.
+run :: IO (Server, EventsQueue)
+run = do
+  let urlPrefix = "http://127.0.0.1"
+  port <- bracket (Warp.openFreePort) (Socket.close . snd) (pure . fst)
+  eventsQueueChan <- Chan.newChan
+  let eventsQueue = EventsQueue eventsQueueChan
+  threadId <- forkIO $
+    Spock.runSpockNoBanner port $
+      Spock.spockT id $ do
+        Spock.get "/" $
+          Spock.json $ Aeson.String "OK"
+        Spock.post "/echo" $ do
+          req <- Spock.request
+          body <- liftIO $ Wai.strictRequestBody req
+          let jsonBody = Aeson.decode body
+          let eventDataPayload =
+                -- Only extract the data payload from the request body
+                let mkJSONPathE = either error id . parseJSONPath
+                    eventJSONPath = mkJSONPathE "$.event.data"
+                 in iResultToMaybe =<< executeJSONPath eventJSONPath <$> jsonBody
+          liftIO $
+            Chan.writeChan eventsQueueChan $
+              fromMaybe (error "error in parsing the event data from the body") eventDataPayload
+          Spock.setHeader "Content-Type" "application/json; charset=utf-8"
+          Spock.json $ Aeson.object ["success" Aeson..= True]
+  let server = Server {port = fromIntegral port, urlPrefix, threadId}
+  Http.healthCheck $ serverUrl server
+  pure (server, eventsQueue)

server/tests-hspec/Test/EventTriggersRunSQLSpec.hs

@@ -0,0 +1,327 @@
+{-# LANGUAGE QuasiQuotes #-}
+{-# LANGUAGE ViewPatterns #-}
+
+-- | Testing the `run_sql` API
+module Test.EventTriggersRunSQLSpec (spec) where
+
+import Control.Concurrent.Chan qualified as Chan
+import Data.Aeson (eitherDecode)
+import Data.ByteString.Lazy.Char8 qualified as L8
+import Harness.Backend.Postgres qualified as Postgres
+import Harness.GraphqlEngine qualified as GraphqlEngine
+import Harness.Http qualified as Http
+import Harness.Quoter.Yaml
+import Harness.Test.Context qualified as Context
+import Harness.Test.Schema qualified as Schema
+import Harness.TestEnvironment (Server (..), TestEnvironment, getServer, stopServer)
+import Harness.Webhook qualified as Webhook
+import Hasura.Prelude (Text, onLeft, onNothing)
+import Network.HTTP.Simple qualified as Http
+import System.Timeout (timeout)
+import Test.HUnit.Base (assertFailure)
+import Test.Hspec (SpecWith, it, shouldBe)
+import Prelude
+
+--------------------------------------------------------------------------------
+-- Preamble
+
+spec :: SpecWith TestEnvironment
+spec =
+  Context.runWithLocalTestEnvironment
+    [ Context.Context
+        { name = Context.Backend Context.Postgres,
+          -- setup the webhook server as the local test environment,
+          -- so that the server can be referenced while testing
+          mkLocalTestEnvironment = webhookServerMkLocalTestEnvironment,
+          setup = postgresSetup,
+          teardown = postgresTeardown,
+          customOptions = Nothing
+        }
+    ]
+    tests
+
+--------------------------------------------------------------------------------
+
+-- * Backend
+
+-- ** Schema
+
+authorsTable :: Text -> Schema.Table
+authorsTable tableName =
+  Schema.Table
+    { tableName = tableName,
+      tableColumns =
+        [ Schema.column "id" Schema.TStr,
+          Schema.column "name" Schema.TStr,
+          Schema.column "created_at" Schema.TUTCTime
+        ],
+      tablePrimaryKey = ["id"],
+      tableReferences = [],
+      tableData = []
+    }
+
+usersTable :: Schema.Table
+usersTable =
+  Schema.Table
+    { tableName = "users",
+      tableColumns =
+        [ Schema.column "id" Schema.TStr,
+          Schema.column "name" Schema.TStr,
+          Schema.column "created_at" Schema.TUTCTime
+        ],
+      tablePrimaryKey = ["id"],
+      tableReferences = [],
+      tableData = []
+    }
+
+schema :: Text -> [Schema.Table]
+schema authorTableName =
+  [ authorsTable authorTableName,
+    usersTable
+  ]
+
+--------------------------------------------------------------------------------
+-- Tests
+
+tests :: Context.Options -> SpecWith (TestEnvironment, (GraphqlEngine.Server, Webhook.EventsQueue))
+tests opts = do
+  triggerListeningToAllColumnTests opts
+  triggerListeningToSpecificColumnsTests opts
+  dropTableContainingTriggerTest opts
+  renameTableContainingTriggerTests opts
+
+triggerListeningToAllColumnTests :: Context.Options -> SpecWith (TestEnvironment, (GraphqlEngine.Server, Webhook.EventsQueue))
+triggerListeningToAllColumnTests opts = do
+  it
+    ( "when a run_sql query drops a column of a table,"
+        <> " it should not throw any error even when an event trigger"
+        <> " that accesses all the columns of that table exists"
+    )
+    $ \(testEnvironment, _) ->
+      shouldReturnYaml
+        opts
+        ( GraphqlEngine.postV2Query
+            200
+            testEnvironment
+            [yaml|
+type: run_sql
+args:
+  source: postgres
+  sql: "ALTER TABLE authors DROP COLUMN created_at;"
+|]
+        )
+        [yaml|
+result_type: CommandOk
+result: null
+         |]
+  it "inserting a new row should work fine" $
+    \(testEnvironment, (_, (Webhook.EventsQueue eventsQueue))) -> do
+      shouldReturnYaml
+        opts
+        ( GraphqlEngine.postV2Query
+            200
+            testEnvironment
+            [yaml|
+type: run_sql
+args:
+  source: postgres
+  sql: "INSERT INTO authors (id, name) values (1, 'john') RETURNING name"
+|]
+        )
+        [yaml|
+result_type: TuplesOk
+result:
+  - - name
+  - - john
+         |]
+      eventPayload <-
+        -- wait for the event for a maximum of 5 seconds
+        timeout (5 * 1000000) (Chan.readChan eventsQueue)
+          >>= (`onNothing` (assertFailure "Event expected, but not fired"))
+      eventPayload
+        `shouldBeYaml` [yaml|
+old: null
+new:
+  name: john
+  id: '1'
+                                        |]
+
+triggerListeningToSpecificColumnsTests :: Context.Options -> SpecWith (TestEnvironment, (GraphqlEngine.Server, Webhook.EventsQueue))
+triggerListeningToSpecificColumnsTests _ = do
+  it
+    ( "when a run_sql query drops a column of a table"
+        <> " and an event trigger is defined to access that column"
+        <> " dependency error should be thrown"
+    )
+    $ \((getServer -> Server {urlPrefix, port}), _) -> do
+      response <-
+        Http.post
+          (urlPrefix ++ ":" ++ show port ++ "/v2/query")
+          mempty
+          [yaml|
+type: run_sql
+args:
+  source: postgres
+  sql: "ALTER TABLE users DROP COLUMN created_at;"
+|]
+      Http.getResponseStatusCode response `shouldBe` 400
+      let responseBody = Http.getResponseBody response
+
+      responseValue <-
+        eitherDecode responseBody
+          `onLeft` \err ->
+            assertFailure
+              ( "In request: " ++ "/v2/query"
+                  ++ "Couldn't decode JSON body:"
+                  ++ show err
+                  ++ "Body was:"
+                  ++ L8.unpack responseBody
+              )
+      responseValue
+        `shouldBeYaml` [yaml|
+path: $
+error: 'cannot drop due to the following dependent objects : event-trigger hasura.users.users_name_created_at
+                   in source "postgres"'
+code: dependency-error
+                                        |]
+
+dropTableContainingTriggerTest :: Context.Options -> SpecWith (TestEnvironment, (GraphqlEngine.Server, Webhook.EventsQueue))
+dropTableContainingTriggerTest opts = do
+  it
+    ( "when a run_sql query drops a table"
+        <> " dependency error should be thrown when an event trigger"
+        <> " accesses specific columns of the table"
+    )
+    $ \(testEnvironment, _) ->
+      shouldReturnYaml
+        opts
+        ( GraphqlEngine.postV2Query
+            200
+            testEnvironment
+            [yaml|
+type: run_sql
+args:
+  source: postgres
+  sql: "DROP TABLE users"
+|]
+        )
+        [yaml|
+result_type: CommandOk
+result: null
+         |]
+
+renameTableContainingTriggerTests :: Context.Options -> SpecWith (TestEnvironment, (GraphqlEngine.Server, Webhook.EventsQueue))
+renameTableContainingTriggerTests opts = do
+  it
+    ( "when a run_sql query drops a column of a table"
+        <> " should not throw any error even when an event trigger"
+        <> " that accesses all the columns of that table exists"
+    )
+    $ \(testEnvironment, _) ->
+      shouldReturnYaml
+        opts
+        ( GraphqlEngine.postV2Query
+            200
+            testEnvironment
+            [yaml|
+type: run_sql
+args:
+  source: postgres
+  sql: "ALTER TABLE authors RENAME TO authors_new;"
+|]
+        )
+        [yaml|
+result_type: CommandOk
+result: null
+         |]
+  it "inserting a new row should work fine" $
+    \(testEnvironment, (_, (Webhook.EventsQueue eventsQueue))) -> do
+      shouldReturnYaml
+        opts
+        ( GraphqlEngine.postV2Query
+            200
+            testEnvironment
+            [yaml|
+type: run_sql
+args:
+  source: postgres
+  sql: "INSERT INTO authors_new (id, name) values (2, 'dan') RETURNING name"
+|]
+        )
+        [yaml|
+result_type: TuplesOk
+result:
+  - - name
+  - - dan
+         |]
+      eventPayload <-
+        -- wait for the event for a maximum of 5 seconds
+        timeout (5 * 1000000) (Chan.readChan eventsQueue)
+          >>= (`onNothing` (assertFailure "Event expected, but not fired"))
+      eventPayload
+        `shouldBeYaml` [yaml|
+old: null
+new:
+  name: dan
+  id: '2'
+                                        |]
+
+--------------------------------------------------------------------------------
+
+-- ** Setup and teardown override
+
+postgresSetup :: (TestEnvironment, (GraphqlEngine.Server, Webhook.EventsQueue)) -> IO ()
+postgresSetup (testEnvironment, (webhookServer, _)) = do
+  Postgres.setup (schema "authors") (testEnvironment, ())
+  let webhookServerEchoEndpoint = GraphqlEngine.serverUrl webhookServer ++ "/echo"
+  GraphqlEngine.postMetadata_ testEnvironment $
+    [yaml|
+type: bulk
+args:
+- type: pg_create_event_trigger
+  args:
+    name: authors_all
+    source: postgres
+    table:
+      name: authors
+      schema: hasura
+    webhook: *webhookServerEchoEndpoint
+    insert:
+      columns: "*"
+- type: pg_create_event_trigger
+  args:
+    name: users_name_created_at
+    source: postgres
+    table:
+      name: users
+      schema: hasura
+    webhook: *webhookServerEchoEndpoint
+    insert:
+      columns:
+        - name
+        - created_at
+|]
+
+postgresTeardown :: (TestEnvironment, (GraphqlEngine.Server, Webhook.EventsQueue)) -> IO ()
+postgresTeardown (testEnvironment, (server, _)) = do
+  GraphqlEngine.postMetadata_ testEnvironment $
+    [yaml|
+type: bulk
+args:
+- type: pg_delete_event_trigger
+  args:
+    name: authors_all
+    source: postgres
+|]
+  stopServer server
+  -- only authors table needs to be tear down because
+  -- the users table has already been dropped in the
+  -- `dropTableContainingTriggerTest` test.
+
+  -- The authors table was renamed in the `renameTableContainingTriggerTests` test
+  Postgres.teardown [(authorsTable "authors_new")] (testEnvironment, ())
+
+webhookServerMkLocalTestEnvironment ::
+  TestEnvironment -> IO (GraphqlEngine.Server, Webhook.EventsQueue)
+webhookServerMkLocalTestEnvironment _ = do
+  Webhook.run