server/mssql: rollback a transaction based on the transaction state

PR-URL: https://github.com/hasura/graphql-engine-mono/pull/3247
GitOrigin-RevId: 70307d8723a63b9314b6ff4d897dcac6e8e93fb9

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Next release
 (Add highlights/major features below)
 
+- server: in mssql transactions, rollback only if the transaction is active
 - server: add request and response bodies to OpenAPI specification of REST endpoints
 - server: implement upsert mutations for MS SQL Server (close #7864)
 

server/src-lib/Database/MSSQL/Transaction.hs

@@ -29,11 +29,12 @@ import Control.Monad.Morph (MFunctor (hoist), MonadTrans (..))
 import Control.Monad.Reader (MonadFix, MonadReader, ReaderT (..))
 import Database.ODBC.SQLServer (FromRow)
 import Database.ODBC.SQLServer qualified as ODBC
-import Hasura.Prelude (hoistEither, liftEither, mapLeft)
+import Hasura.Prelude (Text, hoistEither, liftEither, mapLeft)
 import Prelude
 
 data MSSQLTxError
-  = MSSQLTxError !ODBC.Query !ODBC.ODBCException
+  = MSSQLQueryError !ODBC.Query !ODBC.ODBCException
+  | MSSQLInternal !Text
   deriving (Eq, Show)
 
 -- | A successful result from a query is a list of rows where each row contains list of column values
@@ -59,12 +60,27 @@ beginTx =
   unitQuery "BEGIN TRANSACTION"
 
 commitTx :: MonadIO m => TxT m ()
-commitTx =
-  unitQuery "COMMIT TRANSACTION"
+commitTx = do
+  transactionState <- getTransactionState
+  case transactionState of
+    TSActive -> unitQuery "COMMIT TRANSACTION"
+    TSUncommittable -> throwError $ MSSQLInternal "Transaction is uncommittable"
+    TSNoActive -> throwError $ MSSQLInternal "No active transaction exist; cannot commit"
 
 rollbackTx :: MonadIO m => TxT m ()
-rollbackTx =
-  unitQuery "ROLLBACK TRANSACTION"
+rollbackTx = do
+  transactionState <- getTransactionState
+  let rollback = unitQuery "ROLLBACK TRANSACTION"
+  case transactionState of
+    TSActive -> rollback
+    TSUncommittable ->
+      -- We can only do a full rollback of an uncommittable transaction
+      rollback
+    TSNoActive ->
+      -- Few query exceptions result in an auto-rollback of the transaction.
+      -- For eg. Creating a table with already existing table name (See https://github.com/hasura/graphql-engine-mono/issues/3046)
+      -- In such cases, we shouldn't rollback the transaction again.
+      pure ()
 
 -- | Useful for building transactions which returns no data
 --
@@ -109,6 +125,28 @@ singleRowQueryE ef = rawQueryE ef singleRowResult
     singleRowResult (MSSQLResult [row]) = ODBC.fromRow row
     singleRowResult (MSSQLResult _) = Left "expecting single row"
 
+-- | The transaction state of the current connection
+data TransactionState
+  = -- | Has an active transaction.
+    TSActive
+  | -- | Has no active transaction.
+    TSNoActive
+  | -- | An error occurred that caused the transaction to be uncommittable. We cannot commit or
+    -- rollback to a savepoint; we can only do a full rollback of the transaction.
+    TSUncommittable
+
+-- | Get the @'TransactionState' of current connection
+-- For more details, refer to https://docs.microsoft.com/en-us/sql/t-sql/functions/xact-state-transact-sql?view=sql-server-ver15
+getTransactionState :: (MonadIO m) => TxT m TransactionState
+getTransactionState = do
+  let query = "SELECT XACT_STATE()"
+  xactState :: Int <- singleRowQuery query
+  case xactState of
+    1 -> pure TSActive
+    0 -> pure TSNoActive
+    -1 -> pure TSUncommittable
+    _ -> throwError $ MSSQLQueryError query $ ODBC.DataRetrievalError "Unexpected value for XACT_STATE"
+
 -- | Useful for building query transactions which returns multiple rows.
 --
 -- selectIds :: TxT m [Int]
@@ -149,7 +187,7 @@ rawQueryE ef rf q = TxET $
     hoist liftIO $
       withExceptT ef $
         execQuery conn q
-          >>= liftEither . mapLeft (MSSQLTxError q . ODBC.DataRetrievalError) . rf
+          >>= liftEither . mapLeft (MSSQLQueryError q . ODBC.DataRetrievalError) . rf
 
 -- | Build a generic transaction out of an IO action
 buildGenericTxE ::
@@ -171,7 +209,7 @@ execQuery ::
   ExceptT MSSQLTxError m MSSQLResult
 execQuery conn query = do
   result :: Either ODBC.ODBCException [[ODBC.Value]] <- liftIO $ try $ ODBC.query conn query
-  withExceptT (MSSQLTxError query) $ hoistEither $ MSSQLResult <$> result
+  withExceptT (MSSQLQueryError query) $ hoistEither $ MSSQLResult <$> result
 
 -- | Run a command on the given connection wrapped in a transaction.
 runTx ::

server/src-lib/Hasura/Backends/MSSQL/Connection.hs

@@ -247,13 +247,18 @@ newtype MSSQLConnErr = MSSQLConnErr {getConnErr :: Text}
   deriving (Show, Eq, ToJSON)
 
 fromMSSQLTxError :: MSSQLTxError -> QErr
-fromMSSQLTxError (MSSQLTxError query exception) =
-  (internalError "database query error")
-    { qeInternal =
-        Just $
-          ExtraInternal $
-            object
-              [ "query" .= ODBC.renderQuery query,
-                "exception" .= odbcExceptionToJSONValue exception
-              ]
-    }
+fromMSSQLTxError = \case
+  MSSQLQueryError query exception ->
+    (internalError "database query error")
+      { qeInternal =
+          Just $
+            ExtraInternal $
+              object
+                [ "query" .= ODBC.renderQuery query,
+                  "exception" .= odbcExceptionToJSONValue exception
+                ]
+      }
+  MSSQLInternal err ->
+    (internalError "mssql internal error")
+      { qeInternal = Just $ ExtraInternal $ object ["error" .= err]
+      }

server/src-test/Database/MSSQL/TransactionSpec.hs

@@ -31,7 +31,10 @@ spec connString = do
     it "an unsuccesful transaction, expecting Int" $ do
       result <- runInConn connString selectIntQueryFail
       either
-        (\(MSSQLTxError _ err) -> err `shouldBe` DataRetrievalError "Expected Int, but got: ByteStringValue \"hello\"")
+        ( \case
+            (MSSQLQueryError _ err) -> err `shouldBe` DataRetrievalError "Expected Int, but got: ByteStringValue \"hello\""
+            (MSSQLInternal _) -> expectationFailure unexpectedMSSQLInternalError
+        )
         (\r -> expectationFailure $ "expected Left, returned " <> show r)
         result
 
@@ -42,16 +45,21 @@ spec connString = do
     it "an unsuccesful transaction; expecting single row" $ do
       result <- runInConn connString selectIdQueryFail
       either
-        (\(MSSQLTxError _ err) -> err `shouldBe` DataRetrievalError "expecting single row")
+        ( \case
+            (MSSQLQueryError _ err) -> err `shouldBe` DataRetrievalError "expecting single row"
+            (MSSQLInternal _) -> expectationFailure unexpectedMSSQLInternalError
+        )
         (\r -> expectationFailure $ "expected Left, returned " <> show r)
         result
 
     it "displays the SQL Server error on an unsuccessful transaction" $ do
       result <- runInConn connString badQuery
       either
-        ( \(MSSQLTxError _ err) ->
-            err
-              `shouldBe` UnsuccessfulReturnCode "odbc_SQLExecDirectW" (-1) invalidSyntaxError
+        ( \case
+            (MSSQLQueryError _ err) ->
+              err
+                `shouldBe` UnsuccessfulReturnCode "odbc_SQLExecDirectW" (-1) invalidSyntaxError
+            (MSSQLInternal _) -> expectationFailure unexpectedMSSQLInternalError
         )
         (\() -> expectationFailure "expected Left, returned ()")
         result
@@ -112,3 +120,7 @@ runInConn connString query =
 invalidSyntaxError :: String
 invalidSyntaxError =
   "[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The definition for column 'INVALID_SYNTAX' must include a data type.[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The definition for column 'INVALID_SYNTAX' must include a data type."
+
+unexpectedMSSQLInternalError :: String
+unexpectedMSSQLInternalError =
+  "Expected MSSQLQueryError, but got: MSSQLInternal"

server/tests-py/queries/v2/mssql/run_sql/create_author_table_fail.yaml

@@ -0,0 +1,21 @@
+description: Trying to re-create a table which results in database exeption
+url: /v2/query
+status: 400
+query:
+  type: mssql_run_sql
+  args:
+    source: mssql
+    sql: |
+      CREATE TABLE [author]([id] int not null);
+response:
+  internal:
+    tag: unsuccessful_return_code
+    contents:
+    - odbc_SQLExecDirectW
+    - -1
+    - "[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]There is already an object\
+      \ named 'author' in the database.[Microsoft][ODBC Driver 17 for SQL Server][SQL\
+      \ Server]There is already an object named 'author' in the database."
+  path: $
+  error: sql query exception
+  code: mssql-error

server/tests-py/test_v2_queries.py

@@ -34,6 +34,9 @@ class TestRunSQLMSSQL:
     def test_drop_article_table_with_cascade(self, hge_ctx, backend):
         check_query_f(hge_ctx, self.dir() + '/drop_article_table_with_cascade.yaml')
 
+    def test_create_author_table_fail(self, hge_ctx, backend):
+        check_query_f(hge_ctx, self.dir() + '/create_author_table_fail.yaml')
+
     @classmethod
     def dir(cls):
         return 'queries/v2/mssql/run_sql'