Introduce updateSetOpt fragment (#1893)

[matsluni]

This tries to solve #1893.

I hope the tests run through, as I could not successfully run all tests locally. At some point the testsuite just hanged on my Apple M1. Also, locally, I needed to switch the mysql docker image in the docker compose file to work on the M1.

[matsluni]

Here, we currently have an empty space, if the WHERE clause is empty. Let me know, if this should be handled differently.

[jatcwang]

The spaces are fine :)

[jatcwang]

Thanks this is a good start!
In my comment what I was thinking was a fragment for the where condition, so the signature should be condition: Fragment

Usage will look like:

updateSetOpt(
  fr"my_table",
  ...,
  condition = fr"some_col > 50"
)

or a more complicated example

updateSetOpt(
  fr"my_table",
  ...,
  condition = fragments.inOpt(fr"name", nameList).getOrElse(fr"false")
)

This way the user have full control over the condition.

[matsluni]

Hi @jatcwang, thanks for the clarification. I updated the signature accordingly.

[satorg]

I'd like to weigh in a little, if I may.

It looks like the proposed updateSetOpt will result to None if columnUpdates is empty.

Wouldn't it be better to use the Reducible constraint instead to prevent messing with optionality there in runtime, e.g.:

def updateSet[F[_]: Reducible](
      tableName: Fragment,
      columnUpdates: F[Fragment],
      condition: Fragment
  ): Fragment = ...

Also I'm not sure if it makes sense to include the condition parameter whatsoever, because it just concatenates WHERE clause, but Doobie has a plenty of fragment constructors for it. I.e. it should be fairly easy to write something like this:

updateSet("my_table", myColumnFragments) ++ whereAnd(myConditions)

OR if whereOr is desired

updateSet("my_table", myColumnFragments) ++ whereOr(myConditions)

Note that UPDATE also allows FROM and RETURNING clauses, so one may want to have something like the this:

updateSet("my_table", myColumns) ++
  fr"FROM" ++ values(...) ++
  whereOr(...) ++
  fr"RETURNING" ++ returningStatement

Therefore including the WHERE clause straight into the updateSet[Opt] builder saves not that much but makes the latter less generic and flexible.

[jatcwang]

@satorg With these fragment helpers the general principle I'm following is that it should be hard / impossible to generate invalid SQL or (worse) SQL that lead to the wrong behaviour. Hence I support having condition parameter because it's good to prompt the user to think "oh yes I need a condition for update".

Regarding requiring Reducible instead of Foldable, It's a matter of convenience I guess.

NonEmptyList.from(columnUpdates) { colUpdatesNel =>
  updateSet(
    myTable,
    colUpdatesNel,
    fr".."
  )
}

vs

updateSetOpt(
    myTable,
    colUpdates,
    fr".."
  )

We can have both like we do for most combinators :)

RETURNING doesn't seem to be supported in MySQL so makes sense to leave that out and let people add it themselves.

[satorg]

@jatcwang ,

With these fragment helpers the general principle I'm following is that it should be hard / impossible to generate invalid SQL or (worse) SQL that lead to the wrong behaviour. Hence I support having condition parameter because it's good to prompt the user to think "oh yes I need a condition for update".

Not sure I'm completely following, sorry. AFAIK, the WHERE clause is optional for UPDATE, it is totally valid to use UPDATE without it: https://dbfiddle.uk/27vmy0cg

Moreover, I think it would be nice to have an override of updateSet that could benefit from var-args as well, e.g.:

def updateSet(tableName: Fragment, colUpdate1: Fragment, colUpdates: Fragment*): Fragment = ...

In this case the condition parameter does not fit well.

Regarding requiring Reducible instead of Foldable, It's a matter of convenience I guess.
...
We can have both like we do for most combinators :)

Agree. Although it is hard for me to imagine where the -Opt variant could be useful, but I admit there could be some.

RETURNING doesn't seem to be supported in MySQL so makes sense to leave that out and let people add it themselves.

Yeah, kinda agree too. I feel though that everything but UPDATE and SET can be left out just as well (since UPDATE and SET are the only requisites here).

[jatcwang]

@satorg Good point that with the current signature we cannot generate queries like UPDATE .. SET .. FROM .. WHERE . Let's remove condition parameter then. (Sorry @matsluni!)

Also @matsluni feel free to focus on addressing your immediate usecase (i.e. just implement updateSetOpt) for this PR. We can yakshave in follow-up issues/PRs :)

[matsluni]

Thanks @satorg and @jatcwang for your comments. I will move forward without the where clause. That should also simplify a bit.

@jatcwang, any idea, why the testsuite does not complete on my machine? Is that known already?

[jatcwang]

Thanks @matsluni. I'm not sure why it's failing on M1 - Do you remember what sort of errors you're getting?

[matsluni]

Thanks @matsluni. I'm not sure why it's failing on M1 - Do you remember what sort of errors you're getting?

@jatcwang, firstly I needed to switch to aarm64 compatible mysql image in the docker compose. I think I finally tested with arm64v8/mysql:oracle (ideally, this can be switched to a multiplatform image. But I am no expert for MySql, what image is best to use.). Secondly, the testsuite never finishes for me calling sbt test, It just hangs after successfully completeted doobie.postgres.TextSuite (I aborted after 10 minutes).

[jatcwang]

Thanks for the info! I'll see if I can reproduce and fix it.

[jatcwang]

I have created #2126 to track this