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

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

Open
tpetillot opened this issue Sep 16, 2021 · 6 comments · May be fixed by #2461
Open

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

tpetillot opened this issue Sep 16, 2021 · 6 comments · May be fixed by #2461

Comments

@tpetillot
Copy link

tpetillot commented Sep 16, 2021

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
Copy link

tflucke commented Jan 30, 2022

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
Copy link
Collaborator

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

@easel
Copy link

easel commented Apr 8, 2022

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 easel linked a pull request Apr 8, 2022 that will close this issue
5 tasks
@easel
Copy link

easel commented Apr 8, 2022

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
Copy link

tflucke commented Apr 8, 2022

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants