Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support UPDATE/DELETE .. RETURNING #362

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion changelog.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
3.5.11.3 (unreleased)
========
- @ulidtko
- [#362](https://github.com/bitemyapp/esqueleto/pull/362)
- Add `updateReturning`, `deleteReturning` as Postgres extensions.
Comment on lines +1 to +5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation adds a field to the SideData type, which unfortunately does make this a breaking change and would need to go out as 3.6.0.0.

However, I've suggested an alternative implementation that should be strictly additive, allowing it to be 3.5.12.0.


3.5.11.2
========
- @arguri
Expand Down Expand Up @@ -53,7 +59,6 @@
- [#363](https://github.com/bitemyapp/esqueleto/pull/363)
- Add missing `just` to left join examples in the Haddocks


3.5.9.0
=======
- @9999years
Expand Down
118 changes: 87 additions & 31 deletions src/Database/Esqueleto/Internal/Internal.hs
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
{-# LANGUAGE CPP #-}
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE TypeOperators #-}
{-# LANGUAGE TypeApplications #-}
{-# language DerivingStrategies, GeneralizedNewtypeDeriving #-}

{-# LANGUAGE ConstraintKinds #-}
{-# LANGUAGE DeriveDataTypeable #-}
Expand All @@ -14,7 +11,6 @@
{-# LANGUAGE GADTs #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE InstanceSigs #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE Rank2Types #-}
{-# LANGUAGE ScopedTypeVariables #-}
Expand Down Expand Up @@ -60,7 +56,7 @@
import Data.Kind (Type)
import qualified Data.List as List
import qualified Data.Map.Strict as Map
import qualified Data.Monoid as Monoid

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 8.6.5)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.4.5)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.4.5)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 8.8.4)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 8.8.4)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 8.10.4)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 8.10.4)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.2.2)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.2.2)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.0.2)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.0.2)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.6.2)

The qualified import of ‘Data.Monoid’ is redundant

Check warning on line 59 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.6.2)

The qualified import of ‘Data.Monoid’ is redundant
import Data.Proxy (Proxy(..))
import Data.Set (Set)
import qualified Data.Set as Set
Expand All @@ -70,7 +66,7 @@
import Data.Typeable (Typeable)
import Database.Esqueleto.Internal.ExprParser (TableAccess(..), parseOnExpr)
import Database.Esqueleto.Internal.PersistentImport
import Database.Persist (EntityNameDB(..), FieldNameDB(..), SymbolToField(..))

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.4.5)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.4.5)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 8.8.4)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 8.8.4)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 8.10.4)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 8.10.4)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.2.2)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.2.2)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.0.2)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.0.2)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.6.2)

The import of ‘Database.Persist’ is redundant

Check warning on line 69 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.6.2)

The import of ‘Database.Persist’ is redundant
import qualified Database.Persist
import Database.Persist.Sql.Util
( entityColumnCount
Expand Down Expand Up @@ -427,21 +423,23 @@
putLocking :: LockingClause -> SqlQuery ()
putLocking clause = Q $ W.tell mempty { sdLockingClause = clause }

{-#
DEPRECATED
sub_select
"sub_select \n \
sub_select is an unsafe function to use. If used with a SqlQuery that \n \
returns 0 results, then it may return NULL despite not mentioning Maybe \n \
in the return type. If it returns more than 1 result, then it will throw a \n \
SQL error.\n\n Instead, consider using one of the following alternatives: \n \
- subSelect: attaches a LIMIT 1 and the Maybe return type, totally safe. \n \
- subSelectMaybe: Attaches a LIMIT 1, useful for a query that already \n \
has a Maybe in the return type. \n \
- subSelectCount: Performs a count of the query - this is always safe. \n \
- subSelectUnsafe: Performs no checks or guarantees. Safe to use with \n \
countRows and friends."
#-}
-- | (Internal) Remember a @RETURNING@ clause in a query
tellReturning :: ReturningClause -> SqlQuery ()
tellReturning clause = Q $ W.tell mempty { sdReturningClause = clause }

{-# DEPRECATED sub_select
[ "sub_select is an unsafe function to use. If used with a SqlQuery that"
, "returns 0 results, then it may return NULL despite not mentioning Maybe"
, "in the return type. If it returns more than 1 result, then it will throw a"
, "SQL error.\n\n Instead, consider using one of the following alternatives:"
, "- subSelect: attaches a LIMIT 1 and the Maybe return type, totally safe. "
, "- subSelectMaybe: Attaches a LIMIT 1, useful for a query that already"
, " has a Maybe in the return type."
, "- subSelectCount: Performs a count of the query - this is always safe."
, "- subSelectUnsafe: Performs no checks or guarantees. Safe to use with"
, " countRows and friends."
]
#-}
-- | Execute a subquery @SELECT@ in an SqlExpression. Returns a
-- simple value so should be used only when the @SELECT@ query
-- is guaranteed to return just one row.
Expand Down Expand Up @@ -665,7 +663,7 @@
first (parensM p) . isNullExpr $ f Never info
where
isNullExpr :: (TLB.Builder, a) -> (TLB.Builder, a)
isNullExpr = first ((<> " IS NULL"))
isNullExpr = first (<> " IS NULL")

-- | An alias for 'isNothing' that avoids clashing with the function from
-- "Data.Maybe" 'Data.Maybe.isNothing'.
Expand Down Expand Up @@ -1319,7 +1317,7 @@
unique = finalR uniqueConstructor
-- there must be a better way to get the constrain name from a unique, make this not a list search
filterF = (==) (persistUniqueToFieldNames unique) . uniqueFields
uniqueDef = head . filter filterF . getEntityUniques . entityDef $ proxy

Check warning on line 1320 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

In the use of ‘head’

Check warning on line 1320 in src/Database/Esqueleto/Internal/Internal.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

In the use of ‘head’

-- | Render updates to be use in a SET clause for a given sql backend.
--
Expand Down Expand Up @@ -1835,14 +1833,15 @@
, sdLimitClause :: !LimitClause
, sdLockingClause :: !LockingClause
, sdCteClause :: ![CommonTableExpressionClause]
, sdReturningClause :: !ReturningClause
ulidtko marked this conversation as resolved.
Show resolved Hide resolved
}

instance Semigroup SideData where
SideData d f s w g h o l k c <> SideData d' f' s' w' g' h' o' l' k' c' =
SideData (d <> d') (f <> f') (s <> s') (w <> w') (g <> g') (h <> h') (o <> o') (l <> l') (k <> k') (c <> c')
SideData d f s w g h o l k c r <> SideData d' f' s' w' g' h' o' l' k' c' r' =
SideData (d <> d') (f <> f') (s <> s') (w <> w') (g <> g') (h <> h') (o <> o') (l <> l') (k <> k') (c <> c') (r <> r')

instance Monoid SideData where
mempty = SideData mempty mempty mempty mempty mempty mempty mempty mempty mempty mempty
mempty = SideData mempty mempty mempty mempty mempty mempty mempty mempty mempty mempty mempty
mappend = (<>)

-- | The @DISTINCT@ "clause".
Expand Down Expand Up @@ -1879,6 +1878,10 @@
data CommonTableExpressionClause =
CommonTableExpressionClause CommonTableExpressionKind Ident (IdentInfo -> (TLB.Builder, [PersistValue]))

data ReturningClause
= ReturningNothing -- ^ The default, absent clause.
| ReturningStar -- ^ @RETURNING@ is present.
Comment on lines +1881 to +1883
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use Maybe ReturningClause as the SideData type, then this has one fewer case:

Suggested change
data ReturningClause
= ReturningNothing -- ^ The default, absent clause.
| ReturningStar -- ^ @RETURNING@ is present.
data ReturningClause
= ReturningStar -- ^ @RETURNING@ is present.

But ReturningStar itself isn't carring any useful information - we could further fold that Maybe ReturningClause into just a Bool.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep you're right; I initially tried something different, this had more constructors — but it ended up isomorphic to Maybe ().

Will refactor.


data SubQueryType
= NormalSubQuery
| LateralSubQuery
Expand Down Expand Up @@ -2079,10 +2082,10 @@
mempty = GroupBy []
mappend = (<>)

-- | A @HAVING@ cause.
-- | A @HAVING@ clause.
type HavingClause = WhereClause

-- | A @ORDER BY@ clause.
-- | An @ORDER BY@ clause.
type OrderByClause = SqlExpr OrderBy

-- | A @LIMIT@ clause.
Expand Down Expand Up @@ -2117,6 +2120,15 @@
mempty = NoLockingClause
mappend = (<>)

instance Semigroup ReturningClause where
(<>) ReturningNothing x = x
(<>) x ReturningNothing = x
(<>) ReturningStar ReturningStar = ReturningStar

instance Monoid ReturningClause where
mempty = ReturningNothing
mappend = (<>)

----------------------------------------------------------------------

-- | Identifier used for table names.
Expand Down Expand Up @@ -2400,6 +2412,42 @@
true :: SqlExpr (Value Bool)
true = val True

-- | (Internal) The types which can appear in @RETURNING@ part of @UPDATE@ or @DELETE@
--
-- Many constructs appearing in @SELECT@ can go under @RETURNING@ -- but not all (e.g.
-- certainly not subqueries, @VALUES@ and such). Thus, this is a subclass of 'SqlSelect'.
--
-- The fundeps duplicate those of 'SqlSelect' solely to provide somewhat more directly
-- understandable type errors.
class SqlSelect a r => InferReturning a r | r -> a, a -> r
instance PersistEntity ent => InferReturning (SqlExpr (Entity ent)) (Entity ent)
instance PersistEntity ent => InferReturning (SqlExpr (Maybe (Entity ent))) (Maybe (Entity ent))
instance PersistField a => InferReturning (SqlExpr (Value a)) (Value a)
instance ( InferReturning a ra, InferReturning b rb) => InferReturning (a, b) (ra, rb)
instance ( InferReturning a ra
, InferReturning b rb
, InferReturning c rc
) => InferReturning (a, b, c) (ra, rb, rc)
instance ( InferReturning a ra
, InferReturning b rb
, InferReturning c rc
, InferReturning d rd
) => InferReturning (a, b, c, d) (ra, rb, rc, rd)
instance ( InferReturning a ra
, InferReturning b rb
, InferReturning c rc
, InferReturning d rd
, InferReturning e re
) => InferReturning (a, b, c, d, e) (ra, rb, rc, rd, re)
instance ( InferReturning a ra
ulidtko marked this conversation as resolved.
Show resolved Hide resolved
, InferReturning b rb
, InferReturning c rc
, InferReturning d rd
, InferReturning e re
, InferReturning f rf
) => InferReturning (a, b, c, d, e, f) (ra, rb, rc, rd, re, rf)
-- tuple nesting provides unlimited arity if 6-tuple isn't enough

-- | (Internal) Create a case statement.
--
-- Since: 2.1.1
Expand All @@ -2424,7 +2472,7 @@
in ( b0 <> " WHEN " <> b1 <> " THEN " <> b2, vals0 <> vals1 <> vals2 )

valueToSql :: SqlExpr (Value a) -> NeedParens -> IdentInfo -> (TLB.Builder, [PersistValue])
valueToSql (ERaw _ f) p = f p
valueToSql (ERaw _ f) = f

-- | (Internal) Create a custom binary operator. You /should/
-- /not/ use this function directly since its type is very
Expand Down Expand Up @@ -2908,7 +2956,7 @@
:: (MonadIO m, SqlBackendCanWrite backend)
=> SqlQuery ()
-> R.ReaderT backend m Int64
deleteCount a = rawEsqueleto DELETE a
deleteCount = rawEsqueleto DELETE

-- | Execute an @esqueleto@ @UPDATE@ query inside @persistent@'s
-- 'SqlPersistT' monad. Note that currently there are no type
Expand Down Expand Up @@ -2981,7 +3029,8 @@
orderByClauses
limitClause
lockingClause
cteClause = sd
cteClause
returningClause = sd
-- Pass the finalIdentState (containing all identifiers
-- that were used) to the subsequent calls. This ensures
-- that no name clashes will occur on subqueries that may
Expand All @@ -2999,6 +3048,7 @@
, makeOrderBy info orderByClauses
, makeLimit info limitClause
, makeLocking info lockingClause
, makeReturning info returningClause ret
]


Expand Down Expand Up @@ -3268,6 +3318,12 @@
plain v = (v,[])
makeLocking _ NoLockingClause = mempty

makeReturning :: SqlSelect a r
=> IdentInfo -> ReturningClause -> a -> (TLB.Builder, [PersistValue])
makeReturning _ ReturningNothing _ = mempty
makeReturning info ReturningStar ret = ("RETURNING ", []) <> sqlSelectCols info ret
Comment on lines +3329 to +3332
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting the SqlSelect a r instance and using that to provide sqlSelectCols info ret here.

A typical update in esqueleto has type update :: (SqlExpr (Entity val) -> SqlQuery ()) -> SqlPersistT m (). This would give us an a of (), which would use this instance:

-- | Not useful for 'select', but used for 'update' and 'delete'.
instance SqlSelect () () where
    sqlSelectCols _ _ = ("1", [])
    sqlSelectColCount _ = 1
    sqlSelectProcessRow _ = Right ()

RETURNING 1.

If this clause ReturningStar is enabled in a non-update/insert/delete - ie,

oops = select do
  t <- from $ table @Foo
  tellReturning ReturningStar
  pure t

Then we'll generate a query like:

SELECT foo.a, foo.b, foo.c
FROM foo
RETURNING foo.a, foo.b, foo.c

Which is definitely an error.

So the possibility of error here makes me feel like exploring a different approach.

Copy link
Author

@ulidtko ulidtko May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I thought about that, but tellReturning is in Internal module... its haddock says so, too:

-- | (Internal) Remember a @RETURNING@ clause in a query
tellReturning :: ReturningClause -> SqlQuery ()

The public interface is supposed to be only the added functions in Database.Esqueleto.PostgreSQL (plus associated instances).

Further, the rest of query builder logic didn't seem like it provided any sql correctness guarantees internally.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah,it's not a generally safe thing, but I do want to avoid adding more unsafety where possible.



parens :: TLB.Builder -> TLB.Builder
parens b = "(" <> (b <> ")")

Expand Down Expand Up @@ -3937,14 +3993,14 @@
:: (MonadIO m, PersistEntity a, SqlBackendCanWrite backend)
=> SqlQuery (SqlExpr (Insertion a))
-> R.ReaderT backend m ()
insertSelect a = void $ insertSelectCount a
insertSelect = void . insertSelectCount

-- | Insert a 'PersistField' for every selected value, return the count afterward
insertSelectCount
:: (MonadIO m, PersistEntity a, SqlBackendCanWrite backend)
=> SqlQuery (SqlExpr (Insertion a))
-> R.ReaderT backend m Int64
insertSelectCount a = rawEsqueleto INSERT_INTO a
insertSelectCount = rawEsqueleto INSERT_INTO

-- | Renders an expression into 'Text'. Only useful for creating a textual
-- representation of the clauses passed to an "On" clause.
Expand All @@ -3954,7 +4010,7 @@
renderExpr sqlBackend e = case e of
ERaw _ mkBuilderValues ->
let (builder, _) = mkBuilderValues Never (sqlBackend, initialIdentState)
in (builderToText builder)
in builderToText builder
ulidtko marked this conversation as resolved.
Show resolved Hide resolved

-- | An exception thrown by 'RenderExpr' - it's not designed to handle composite
-- keys, and will blow up if you give it one.
Expand Down
51 changes: 50 additions & 1 deletion src/Database/Esqueleto/PostgreSQL.hs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
, chr
, now_
, random_
, deleteReturning
, updateReturning
, upsert
, upsertBy
, insertSelectWithConflict
Expand All @@ -41,6 +43,7 @@
#if __GLASGOW_HASKELL__ < 804
import Data.Semigroup
#endif
import Conduit (withAcquire)
import Control.Arrow (first)
import Control.Exception (throw)
import Control.Monad (void)
Expand Down Expand Up @@ -75,7 +78,7 @@
(PersistField a, PersistField [a])
=> SqlExpr (Value (Maybe [a]))
-> SqlExpr (Value [a])
maybeArray x = coalesceDefault [x] (emptyArray)
maybeArray x = coalesceDefault [x] emptyArray

-- | Aggregate mode
data AggMode
Expand Down Expand Up @@ -238,7 +241,7 @@
queryVals =
addVals updateVals
xs <- rawSql queryText queryVals
pure (head xs)

Check warning on line 244 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

In the use of ‘head’

Check warning on line 244 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

In the use of ‘head’
#else
uDef = toUniqueDef uniqueKey
handler conn f = fmap head $ uncurry rawSql $
Expand Down Expand Up @@ -280,7 +283,7 @@
-- @since 3.1.3
insertSelectWithConflict
:: forall a m val backend
. (FinalResult a, KnowResult a ~ Unique val, MonadIO m, PersistEntity val, SqlBackendCanWrite backend)

Check warning on line 286 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.4.5)

The use of ‘~’ without TypeOperators

Check warning on line 286 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.4.5)

The use of ‘~’ without TypeOperators

Check warning on line 286 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

The use of ‘~’ without TypeOperators

Check warning on line 286 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

The use of ‘~’ without TypeOperators

Check warning on line 286 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.6.2)

The use of ‘~’ without TypeOperators

Check warning on line 286 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.6.2)

The use of ‘~’ without TypeOperators
=> a
-- ^ Unique constructor or a unique, this is used just to get the name of
-- the postgres constraint, the value(s) is(are) never used, so if you have
Expand All @@ -300,7 +303,7 @@
-- @since 3.1.3
insertSelectWithConflictCount
:: forall a val m backend
. (FinalResult a, KnowResult a ~ Unique val, MonadIO m, PersistEntity val,

Check warning on line 306 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.4.5)

The use of ‘~’ without TypeOperators

Check warning on line 306 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.4.5)

The use of ‘~’ without TypeOperators

Check warning on line 306 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

The use of ‘~’ without TypeOperators

Check warning on line 306 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.8.1)

The use of ‘~’ without TypeOperators

Check warning on line 306 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.6.2)

The use of ‘~’ without TypeOperators

Check warning on line 306 in src/Database/Esqueleto/PostgreSQL.hs

View workflow job for this annotation

GitHub Actions / build (3.10.2.1, 9.6.2)

The use of ‘~’ without TypeOperators
SqlBackendCanWrite backend)
=> a
-> SqlQuery (SqlExpr (Insertion val))
Expand Down Expand Up @@ -477,3 +480,49 @@
forShareOf :: LockableEntity a => a -> OnLockedBehavior -> SqlQuery ()
forShareOf lockableEntities onLockedBehavior =
putLocking $ PostgresLockingClauses [PostgresLockingKind PostgresForShare (Just $ LockingOfClause lockableEntities) onLockedBehavior]

-- | `UPDATE .. RETURNING ..` SQL extension supported by Postgres.
--
-- The instances of 'InferReturning' say what can be returned; currently includes
-- whole entities, 'PersistField's, SQL expressions, tuples (possibly nested).
--
-- Usage example:
--
-- @
-- tuples <- updateReturning $ \p -> do
-- set p [ PersonAge =. val (Just 0) ]
-- where_ (isNothing $ p ^. PersonAge)
-- return (val True, p ^. PersonName, (p ^. PersonFavNum) *. val 100)
-- -- return p -- also works, returning (Entity Person)
-- @
--
-- @since 3.5.11.3
updateReturning :: (MonadIO m, From from, InferReturning ex ret, SqlBackendCanWrite backend)
=> (from -> SqlQuery ex)
-> R.ReaderT backend m [ret]
updateReturning block = do
conn <- R.ask
conduit <- rawSelectSource UPDATE (tellReturning ReturningStar >> from block)
liftIO . withAcquire conduit $ flip R.runReaderT conn . runSource
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the use of the new functionality - neat.

This is where we say "Do the RETURNING thing":

rawSelectSource UPDATE (tellReturning ReturningStar >> from block)

So we modify the SideData for the query, then it gets rendered and turned into a conduit, which we run.

I think we can avoid modifying SideData and still mostly keep this interface.

Consider renderQueryToText. This takes a SqlQuery a and renders it out.

If the SqlQuery a has a ReturningStar in the SideData, then we graft on the appropriate text to the query. However, we can just do that directly without modifying the internals and introducing potential unsafety.

If we look at the code for rawSelectSource, we can see that it's just doing toRawSql on the query and then rawQueryRes. The rest is handling the Conduit (which imo isn't super useful since the query must be loaded into memory anyway).

So, if we inline this and use rawQuery instead (which just does the list directly), we can do:

updateReturning block = do
    conn <- R.ask
    let 
        query' = toRawSql UPDATE (conn, initialIdentState) block
        ((ret, _sd), _finalIdent) = flip S.runState -- ... (from toRawSql) 
        returningClause =  ("RETURNING ", []) <> sqlSelectCols info ret
        (queryWithReturning, vals) = 
             query' <> ("\n", []) <> returningClause
    rawQuery queryWithReturning vals

Now, it is unsatisfying that we have to run the query building twice with this - once in toRawSql and once to get the ret to make the returningClause with.

If toRawSql is refactored into a variant that also returns the ret, then I think we're able to avoid that - then this can be:

    conn <- R.ask
    let 
        (ret, query') = toRawSqlWith UPDATE (conn, initialIdentState) block
        returningClause =  ("\nRETURNING ", []) <> sqlSelectCols info ret
        (queryWithReturning, vals) = 
             query' <> returningClause
    rawQuery queryWithReturning vals

Then I think there's probably virtue in writing makeReturning :: (InferReturning a r) => a -> (TLB.Builder, [PersistValue]) which can then be reused in deleteReturning.


-- | `DELETE .. RETURNING` SQL extension supported by Postgres.
--
-- The instances of 'InferReturning' say what can be returned; currently includes
-- whole entities, 'PersistField's, SQL expressions, tuples (possibly nested).
--
-- Usage example:
--
-- @
-- removedRowsWithNames <- deleteReturning $ \p -> do
-- where_ (isNothing $ p ^. PersonWeight)
-- return (p, p ^. PersonName)
-- @
--
-- @since 3.5.11.3
deleteReturning :: (MonadIO m, From from, InferReturning ex ret, SqlBackendCanWrite backend)
=> (from -> SqlQuery ex)
-> R.ReaderT backend m [ret]
deleteReturning block = do
conn <- R.ask
conduit <- rawSelectSource DELETE (tellReturning ReturningStar >> from block)
liftIO . withAcquire conduit $ flip R.runReaderT conn . runSource
ulidtko marked this conversation as resolved.
Show resolved Hide resolved
33 changes: 33 additions & 0 deletions test/PostgreSQL/Test.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1056,6 +1056,38 @@ testUpsert =
u3e <- EP.upsert u3 [OneUniqueName =. val "fifth"]
liftIO $ entityVal u3e `shouldBe` u1{oneUniqueName="fifth"}

testUpdateDeleteReturning :: SpecDb
testUpdateDeleteReturning =
describe "UPDATE .. RETURNING .." $ do
itDb "Whole entities, expressions and tuples get returned from UPDATE" $ do
[_p1k, _p2k, _p3k, p4k, _p5k] <- mapM insert [p1, p2, p3, p4, p5]
ret1 <- EP.updateReturning $ \p -> do
set p [ PersonFavNum =. val 42 ]
where_ (p ^. PersonFavNum ==. val 4)
return p
asserting $ ret1 `shouldBe` [Entity p4k p4{ personFavNum = 42 }]
ulidtko marked this conversation as resolved.
Show resolved Hide resolved

ret2 <- EP.updateReturning $ \p -> do
set p [ PersonAge =. val (Just 0) ]
where_ (isNothing $ p ^. PersonAge)
return (val True, p ^. PersonName, (p ^. PersonFavNum) *. val 100)
asserting $ ret2 `shouldBe` [ (Value True, Value "Rachel", Value 200)
, (Value True, Value "Mitch", Value 500) ]

itDb "Whole entities, expressions and tuples get returned from DELETE" $ do
[_p1k, p2k, _p3k, p4k, _p5k] <- mapM insert [p1, p2, p3, p4, p5]
ret1 <- EP.deleteReturning $ \p -> do
where_ (isNothing $ p ^. PersonWeight)
return (val (1 :: Int, 2 :: Int), p ^. PersonName, isNothing (p ^. PersonAge))
asserting $ ret1 `shouldBe` [ (Value (1, 2), Value "John", Value False)
, (Value (1, 2), Value "Mike", Value False)
, (Value (1, 2), Value "Mitch", Value True) ]
ret2 <- EP.deleteReturning $ \p -> do
-- empty WHERE -- delete everything remaining... but:
return (p, p ^. PersonName)
asserting $ ret2 `shouldBe` [ (Entity p2k p2, Value "Rachel")
, (Entity p4k p4, Value "Livia") ]
ulidtko marked this conversation as resolved.
Show resolved Hide resolved

testInsertSelectWithConflict :: SpecDb
testInsertSelectWithConflict =
describe "insertSelectWithConflict test" $ do
Expand Down Expand Up @@ -1629,6 +1661,7 @@ spec = beforeAll mkConnectionPool $ do
testPostgresqlTextFunctions
testInsertUniqueViolation
testUpsert
testUpdateDeleteReturning
testInsertSelectWithConflict
testFilterWhere
testCommonTableExpressions
Expand Down
Loading