Support UPDATE/DELETE .. RETURNING

[ulidtko]

Howdy!

This implements #44 🎉 The aggregate issue #306 is also related.

beginner friendly indeed 👌 was a bit scary at first, but I found a way! Pretty pleased with the result.

Kindly review & merge 🙏

---

PR template checklist:

• Bumped the version number.

• Documented new APIs with Haddock markup.

• Added @since declarations to the Haddock.

• Ran stylish-haskell and otherwise adhered to the style guide
  ⚠️ this fails with syntax error:

  src/Database/Esqueleto/Internal/Internal.hs: RealSrcSpan SrcSpanPoint "src/Database/Esqueleto/Internal/Internal.hs" 427 1: lexical error in string/character literal at character 's'

  BTW that weird sub_select deprecation notice is also breaking VS Code syntax highlighting, badly. Vim handles it, but also emits a parse error:
  

  Edit: fixed ✔️ in one of the PR commits.

After submitting your PR:

• Update the Changelog.md file with a link to your PR.
• Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts).

[ulidtko]

One more thing... as I read around, UPDATE .. RETURNING isn't unique to Postgres — MariaDB, Oracle, even MSSQL support it too. The implementation in this PR is pretty much DBMS-agnostic as well.

Not sure what to make of that; my usecase is Postgres, so... 🤷

[ulidtko]

Gee, INSERT also supports RETURNING

[ulidtko]

Changelog rebased after v3.5.10.0 release. @parsonsmatt @ivanbakel please review

[ulidtko]

Nicely wrapping INSERT .. RETURNING as well in this DSL is above my head at this point. 😕

I'd tried several times; the best I got is a failing test that generates the expected query:

[Debug#SQL] INSERT INTO "Person"("name","age","weight","favNum") VALUES(?,?,?,?) RETURNING "id"; [PersistText "John",PersistInt64 36,PersistNull,PersistInt64 1]

but then result readout fails with UnsupportedSqlInsertIntoType, apparently here:

esqueleto/src/Database/Esqueleto/Internal/Internal.hs

Line 3302 in e50fed1

sqlInsertInto = throw (UnexpectedCaseErr UnsupportedSqlInsertIntoType)

But then to use the other SqlSelect instance with Insertion a, I'd need to fix its sqlSelectProcessRow, and then somehow combine rawSelectSource and rawEsqueletto to both be passing input data "into" the query, and get the conduit for reading "out" of it.

---

Factoring in the complete radio silence from maintainers — I won't invest much further effort.

[ulidtko]

Hello @parsonsmatt @9999years would you be so kind to take a look. Review the PR?

Changelog rebased once again, and @since tags fixed, after releases v3.5.10.1 and v3.5.10.2.

[ulidtko]

Ping @parsonsmatt @9999years @arguri @halogenandtoast @isomorpheme @ttuegel

[parsonsmatt]

Hey, I'm sorry it's taken me a long time to look at this. I'll review it today.

[parsonsmatt]

Thanks for the contribution! I apologize again for taking so long to review it.

I've suggested an alternative implementation which should be safer and possibly even slightly simpler. The backbone comes from refactoring toRawSql to expose the ret from the SqlQuery, which allows you to append the RETURNING clause directly and avoid modifying SideData:

-- | (Internal) Pretty prints a 'SqlQuery' into a SQL query.
--
-- Note: if you're curious about the SQL query being generated by
-- @esqueleto@, instead of manually using this function (which is
-- possible but tedious), see the 'renderQueryToText' function (along with
-- 'renderQuerySelect', 'renderQueryUpdate', etc).
toRawSql
  :: (SqlSelect a r, BackendCompatible SqlBackend backend)
  => Mode -> (backend, IdentState) -> SqlQuery a -> (TLB.Builder, [PersistValue])
toRawSql mode queryEnv =
    snd . toRawSqlWith mode queryEnv

-- | Render a 'SqlQuery' into the internal representation of the returned
-- result, the text 'TLB.Builder', and the @['PersistValue']@ to interpolate
-- into the query text.
toRawSqlWith
    :: (SqlSelect a r, BackendCompatible SqlBackend backend)
    => Mode -> (backend, IdentState) -> SqlQuery a -> (a, (TLB.Builder, [PersistValue]))
toRawSqlWith mode (conn, firstIdentState) query =
    let ((ret, sd), finalIdentState) =
            flip S.runState firstIdentState $
            W.runWriterT $
            unQ query
        deleteRepeatedNewlines txt =
            let
                (preNewlines, rest) = TL.break (== '\n') txt
                (_, rest') = TL.break (/= '\n') rest
             in
                if TL.null rest'
                    then preNewlines <> "\n"
                    else preNewlines <> "\n" <> deleteRepeatedNewlines rest'

        SideData distinctClause
                 fromClauses
                 setClauses
                 whereClauses
                 groupByClause
                 havingClause
                 orderByClauses
                 limitClause
                 lockingClause
                 cteClause = 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
        -- appear on the expressions below.
        info = (projectBackend conn, finalIdentState)

        overFst f (a, b) = (f a, b)
        convert = TLB.fromLazyText . deleteRepeatedNewlines . TL.strip . TLB.toLazyText

    in (,) ret $ overFst convert $ mconcat $ intersperse ("\n", [])
        [ makeCte        info cteClause
        , makeInsertInto info mode ret
        , makeSelect     info mode distinctClause ret
        , makeFrom       info mode fromClauses
        , makeSet        info setClauses
        , makeWhere      info whereClauses
        , makeGroupBy    info groupByClause
        , makeHaving     info havingClause
        , makeOrderBy    info orderByClauses
        , makeLimit      info limitClause
        , makeLocking    info lockingClause
        ]

[parsonsmatt]

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.

[ulidtko]

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

Will refactor.

[parsonsmatt]

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.

[ulidtko]

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.

[parsonsmatt]

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

[parsonsmatt]

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.

[parsonsmatt]

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.

[ulidtko]

Thanks for review @parsonsmatt 🙏

Easy nits done; I'll work on this some more. Your suggestion does make sense, I see its merits.

One question I got which could use your input: how to implement the INSERT … RETURNING … variant? I've got at the point of building the intended query, but then hit issues trying to combine the output conduit with a way to simultaneously input values into the query. Modulo the (?) placeholders, none of other query types allow to both input & output entire relations at the same time. So I struggled to come up with a way to do it. Any approach you can suggest?

[parsonsmatt]

insertReturning is a good question. I think the overall signature we want is something like:

insertSelectReturning 
    :: (SqlSelect entity x, SqlSelect returning r) =>
     => SqlQuery (SqlExpr (Insertion entity), returning)
    -> ReaderT backend m r

So folks would write:

f :: SqlPersistT m [FooId]
f = do
    insertSelectReturning do
        t <- from $ table @Foo
        pure 
          ( Bar <# (t ^. FooName) <&> (t ^. FooBar)
          , t ^. FooId
          )

I think you can do fmap fst :: SqlQuery (SqlExpr (Insertion a), returning) -> SqlQuery (SqlExpr (Insertion a)) and run that through to generate the main query. Then fmap snd :: SqlQuery (SqlExpr (Insertion a), returning) -> SqlQuery returning, which you'd want to run to get the returning, and then you can construct the returning clause for it.

It may also be better to define a data InsertReturning ins ret = InsertReturning { insertClause :: SqlExpr (Insertion ins)), returningClause :: ret } with it's own SqlSelect instance. Then you'd write pure InsertReturning { insertClause = ... etc } which may be clearer, and provide better documentation/behavior for the actual query generation.

[belevy]

So I just hit a reason to implement insertSelectWithConflictReturning and it led me to want something like SqlQuery specifically for inserts. My original type was SqlInsert insertType returningType, I ended up moving to SqlInsert backend insertType returningType though really it could probably just be monomorphic in SqlBackend.

So my idea for would be something like

data OnConflictDo ent 
  = DoNothing 
  | DoUpdate (SqlExpr (Entity ent) -> SqlExpr (Entity ent) -> [SqlExpr (Entity ent) ->SqlExpr Update])
runInsertReturning :: SqlSelect a r => SqlInsert backend insertType a -> ReaderT backend m [r]
onConflict :: EI.KnowResult uniq ~ Unique ent => uniq -> OnConflictDo ent -> SqlInsert backend ent a -> SqlInsert backend ent a

let doNothing = DoNothing
in insertFrom query 
  & onConflict UniqueField doNothing
  & runInsertReturning

[belevy]

Having worked with this a bit more, the backend can always just be SqlBackend inside of the SqlInsert type. Additionally runInsertReturning should take a projection function to determine the output rather than figuring out how to return an a directly.