Jdbc context run action returning result is type as single value instead of list

[tpetillot]

Version: v3.8.0
Module: quill-jdbc

https://github.com/getquill/quill/blob/876cae2c6b56317f373426b6049a40273d8301d0/quill-jdbc/src/main/scala/io/getquill/context/jdbc/JdbcContext.scala#L24

Expected behavior

Delete returning should return a list as many value could have been deleted.

Actual behavior

Return only the first deleted value, same behavior on update. (may be induce by insert returning?)

Steps to reproduce the behavior

Define a quote using returning on delete or update, result is a single value

Workaround

Unkown

@getquill/maintainers

[tflucke]

I'm having the same problem. I'd offer to fix it myself but I'm going to be busy with personal matters for the next few weeks.

Hopefully I'll have some time to take a look and the issue is simple enough that a novice like me can fix it.

This seems especially strange to me because update/delete should be able to return zero results without error so this behavior seems simply incorrect.

[deusaquilus]

Yeah, it definitely sounds incorrect. @tflucke if you want to look into it I can try to provide some guidance.

[easel]

Perhaps consider making it "returningList" or similar and possibly deprecating and renaming "returning" to "returningOne"?

I believe the root issue is JdbcRunContext.scala:65 just needs to not wrap extractResult in handleSingleResult:

  def executeActionReturning[O](sql: String, prepare: Prepare = identityPrepare, extractor: Extractor[O], returningBehavior: ReturnAction)(info: ExecutionInfo, dc: Runner): Result[O] =
    withConnectionWrapped { conn =>
      val (params, ps) = prepare(prepareWithReturning(sql, conn, returningBehavior), conn)
      logger.logQuery(sql, params)
      ps.executeUpdate()
      handleSingleResult(extractResult(ps.getGeneratedKeys, conn, extractor))
    }

[easel]

I started chasing this down (#2461). It doesn't look like it's too bad, but since executeActionReturning is part of the top level quill API it's a pretty impactful change. I think we should decide whether or not to change the existing api before chasing down all the implementations. OTOH, the existing api is more or less broken, so maybe a change is due?

Implementing a new "returningList" or similar with more appropriate semantics would make it possible to limit the blast radius of the change, maintain backwards compatibility and possibly even maintain backwards binary compatibility.

[tflucke]

What would be the semantics of "returningOne"? What happens specifically when:

1. There are zero results?
2. There are two or more results?
3. There is an error?

It seems contrary to the design philosophy of Quill to start introducing new semantics like this. Is the goal just to maintain backwards compatibility? If so, I would leave "returning" as-is deprecated and not introduce anything with identical functionality.

If a user needs the old functionality, they can use ".headOption" or a match statement and write their own error handling (which should have been how this was originally handled).

[deusaquilus]

I think it should be returningMany and there should be a corresponding executeActionReturningMany. @easel if you'd like to try to do implement this (or anyone else) I can try to sketch out what needs to be done. Please let me know.