Document a bit more the Database.Transactional module

src/Gargantext/Database/Transactional.hs

 {-# LANGUAGE KindSignatures      #-}
 {-# LANGUAGE LambdaCase          #-}
 {-# LANGUAGE ScopedTypeVariables #-}
-{-# LANGUAGE TypeApplications #-}
+{-# LANGUAGE TypeApplications    #-}
+
+{--| This module exposes a custom monad and functions to model database operations within Gargantext.
+ The peculiarity of the custom monad is that it describe a DSL for the operations we wish to perform,
+ and there is an evaluator that runs everything at the end of the transactional block. This means
+ that we can \"bundle\" different SQL queries together in a single IO operation (in the evaluation) and
+ we can wrap everything with \"withTransaction\", meaning that everything will run in a single database
+ transaction \"by construction\", and using the same DB connection. This limits greatly the surface area
+ for concurrency bugs stemming from indepedent DB transactions running against the same subset of data. -}
+
 module Gargantext.Database.Transactional (
     DBOperation
   , DBTransactionOp -- opaque
@@ -24,7 +33,7 @@ module Gargantext.Database.Transactional (
   , mkOpaInsert
   , mkOpaDelete
 
-  -- * Throwing errors (which allow rollbacks)
+  -- * Throwing errors (which allows rollbacks)
   , dbFail
   ) where
 
@@ -49,6 +58,39 @@ data DBOperation = DBRead | DBWrite
 -- | A functor describing a single operation on the database. Each constructor takes a continuation
 -- argument which can be used to derive a sound 'Functor' instance, making this viable to be applied
 -- in a monadic/free context.
+--
+-- /IMPORTANT/: If you read this comment, you are probably wondering about adding more operations
+-- to this functor, maybe because the query/update you are writing can't be expressed directly with
+-- the subset here. While adding more operations is certainly possible, it's also important to
+-- understand that this monad has to contain as little constructors as possible as it's expressing
+-- /THE OPERATIONS WE CAN SAFELY DO IN A DATABASE TRANSACTION/. In other terms, everything we add
+-- here will have to run in a DB transaction, and the latter might at /any/ point have to rollback!
+-- This means that destructive operations can't be rolled back, so they /shouldn't/ be modelled as
+-- new 'DBTransactionOp' constructors.
+--
+-- Here is a practical example of operations we /could/ add in the future, which would still be ok:
+--
+-- GetTxCurrentTime :: (UTCTime -> next) -> DBTransactionOp err r next
+-- LogMsg :: LogLevel -> T.Text -> (() -> next) -> DBTransactionOp err r next
+--
+-- Here is a practical example of operations which /ARE NOT OK/:
+--
+-- EmitCENotification :: CEMessage -> (() -> next) -> DBTransactionOp err r next
+--
+-- What's the difference between what's OK and what's not? The rollback. If we add the ability to get
+-- the current time, or log a message, even if we rollback, nothing destructive has happened: sure, in
+-- the worst case scenario we would be logging a message for something that eventually we aborted, but
+-- we haven't compromised the internal correctness of the system.
+--
+-- Conversely, if we emit a CE message /and then we have to rollback due to an exception/, now it means
+-- we have notified upstream about something that we eventually cancelled! That is wrong, and is a bug.
+--
+-- Please refer to your best judgement and add here only operations which do have some meanining in the
+-- context of a DB transaction, and keep the rollback behaviour as your north-star to decided whether or
+-- not that is a good idea.
+-- Everything else can just be passed as an input to a 'DBTransactionOp' or simply returned as a result
+-- from a 'DBTransactionOp', and later used in the concreted monad. That's what we do for CE notifications;
+-- we have DB operations returning them, and we fire them \"outside\".
 data DBTransactionOp err (r :: DBOperation) next where
   -- | A Postgres /read/, returning a list of results. The 'r' in the result is polymorphic
   -- so that reads can be embedded in updates transactions.
@@ -72,10 +114,15 @@ data DBTransactionOp err (r :: DBOperation) next where
   -- | An Opaleye /write/, returning a result depending on the input 'Update'. It can be used only in
   -- 'DBWrite' transactions.
   OpaUpdate :: Update a -> (a -> next) -> DBTransactionOp err DBWrite next
+  -- | An Opaleye /delete/, returning a result depending on the input 'Delete'. It can be used only in
+  -- 'DBWrite' transactions.
   OpaDelete :: Delete a -> (a -> next) -> DBTransactionOp err DBWrite next
   -- | Monadic failure for DB transactions.
   DBFail    :: err -> DBTransactionOp err r next
 
+-- | A 'DBTx' is a free monad (using the free church-encoding 'F') using 'DBTransactionOp' as the functor.
+-- In practical terms, it's just a monad where we can execute just the operations described by the
+-- 'DBTransactionOp', and nothing more.
 newtype DBTx err r a = DBTx { _DBTx :: F (DBTransactionOp err r) a }
   deriving (Functor, Applicative, Monad)
 
@@ -84,7 +131,7 @@ type DBUpdate err a     = DBTx err DBWrite a
 type DBReadOnly err r a = DBTx err DBRead a
 
 -- Strict constraints to perform transactional read and writes.
--- Isomorphic to a DBCmd but it doesn't impose a 'HasConfig' constraint, as
+-- Isomorphic to a DBCmd, but it doesn't impose a 'HasConfig' constraint, as
 -- values can always be passed as parameters of a query or update.
 type DBTxCmd err a = forall m env. (IsCmd env err m, HasConnectionPool env) => m a
 
@@ -106,6 +153,7 @@ instance Functor (DBTransactionOp err r) where
 withResourceM :: MonadBaseControl IO m => Pool a -> (a -> m b) -> m b
 withResourceM pool func = control $ \run -> withResource pool (run . func)
 
+-- | Generalised version of 'withTransaction' to work over any unlifted monad.
 withTransactionM
   :: forall m a.
      MonadBaseControl IO m
@@ -114,7 +162,8 @@ withTransactionM
   -> m a
 withTransactionM conn action = control $ \runInIO -> PG.withTransaction conn $ runInIO action
 
--- | Run a PostgreSQL "read-only" transaction, suitable for read-only queries.
+-- | Executes the input action in a single PostgreSQL "read-only" transaction,
+-- suitable for read-only queries.
 withReadOnlyTransactionM
   :: forall m a.
      MonadBaseControl IO m
@@ -128,12 +177,15 @@ withReadOnlyTransactionM conn action =
     tmode :: PG.TransactionMode
     tmode = PG.TransactionMode PG.DefaultIsolationLevel PG.ReadOnly
 
+-- | Run a PostgreSQL transaction, suitable for operations that mixes read and writes,
+-- and actually the only choice available to run 'DBUpdate' operations.
 runDBTx :: DBUpdate err a -> DBTxCmd err a
 runDBTx (DBTx m) = do
   pool <- view connPool
   withResourceM pool $ \conn -> withTransactionM conn $ foldF (evalOp conn) m
 
--- | /NOTE/ the input type is 'DBReadOnly', i.e. a transaction where /all/
+-- | Runs a DB query.
+-- /NOTE/ the input type is 'DBReadOnly', i.e. a transaction where /all/
 -- the operations are 'DBRead'. This makes impossible to sneak in updates
 -- into otherwise read-only queries.
 runDBQuery :: DBReadOnly err r a -> DBTxCmd err a
@@ -162,6 +214,12 @@ evalOpaCountQuery conn sel = do
   -- countRows is guaranteed to return a list with exactly one row so DL.head is safe here
   pure $ fromIntegral @Int64 @Int $ head counts
 
+-- | Executes a Postgres query that /HAS/ to contain a \"RETURNING ..\" statement /AND/
+-- a \"where\" predicate that indexes a /UNIQUE/ field, i.e. something like \"WHERE id = ? RETURNING id\".
+-- The above SQL fragment ensures that Postgres will return us a single row with a single result, which
+-- is what we are enforcing here.
+-- This function will fail with an error (and the whole transaction will rollback) if the SQL query
+-- violates the contract.
 queryOne :: (PG.ToRow q, PG.FromRow r) => PG.Connection -> PG.Query -> q -> IO r
 queryOne conn q v = do
   rs <- PG.query conn q v
@@ -173,6 +231,9 @@ queryOne conn q v = do
 --
 -- Smart constructors
 --
+-- The following functions are just mechanical wrappers around the raw data constructors, which
+-- we are not exposing for information hiding purposes.
+--
 
 dbFail :: err -> DBTx err r b
 dbFail = DBTx . liftF . DBFail