AugustNagro / magnum

A 'new look' for database access in Scala
Apache License 2.0
153 stars 10 forks source link

Comparing nulls in sql or how to handle Option in sql interpolator #28

Closed cornerman closed 4 weeks ago

cornerman commented 3 months ago

Of course, I fell into the trap of writing Spec[Something].where(sql"column = ${someOption}"). This works fine if someOption is defined, but will obviously not find anything if it is None. In this case, I actually wanted to switch to sql"column is NULL".

Is there already a helper to compare Option like this?

I had the idea of adding helpers like Spec[Something].whereEq("column", someOption). Alternatively, one could also model different cooperators and have a method like where("column", Compare.Eq, someOption). Or maybe we could provide something on the Frag level or in the interpolator like sql"column ${Compare.Eq(someOption)}".

What do you think or suggest here?

AugustNagro commented 3 months ago

Hi, that's a common issue, and I think we need better documentation here.

As you noted, in SQL queries, X = NULL will always return false, so we have to use X is NULL instead.

Lets say you have a user request DTO like

case class UserReq(company: String, department: Option[String])

What does department = None mean for the query? It could be that we want to

(1) Ignore this filter from the query and return all departments. This is the most common scenario IME. We can write the spec like:

Spec[User]
  .where(sql"company = ${userReq.company}")
  .where(sql"department = ${userReq.department} OR ${userReq.department} IS NULL")

(2) Include only results where department is NULL (your scenario). We can write the spec like:

Spec[User]
  .where(sql"company = ${userReq.company}")
  .where(sql"department = ${userReq.department} OR (department IS NULL AND ${userReq.company} IS NULL)")
cornerman commented 3 months ago

Thank you for your answer - both scenarios are indeed valid, depending on the use-case. I was probably looking at it from a specific perspective. A solution should be able to handle both cases and/or be clear about what it does.

For the two queries that you posted, I think, it makes sense to not send the where with a null value to the database, if the value is None. Maybe like this:

(1)

Spec[User]
  .where(sql"company = ${userReq.company}")
  .tap(spec => userReq.department.fold(spec)(department => spec.where(sql"department = ${department})))

(2)

Spec[User]
  .where(sql"company = ${userReq.company}")
  .where(spec => userReq.department.fold(sql"department is NULL")(department => sql"department = ${department}))

All of the above queries that we wrote are quite short - and they are explicit. But when writing where conditions on repositories, I felt that a little dsl for building the actual where-condition would help. Especially when refactoring code where a field becomes optional, it is easy to miss a where condition.

Apart from this issue, what are your thoughts on extending the Spec-type to include specific field-information like the TableInfo does for the column-names? I could totally imagine functions like Spec[User].whereCompany(?, userReq.company) :)

AugustNagro commented 2 months ago

Why does your Spec[User].whereCompany(?, userReq.company) example have two parameters?

cornerman commented 2 months ago

Why does your Spec[User].whereCompany(?, userReq.company) example have two parameters?

Sorry, that was unclear. I had an operator in mind, to select how you want to compare the value.

AugustNagro commented 2 months ago

Got it. Maybe a trait like

trait SpecOptionMapping:
  def frag[A](columnName: String, opt: Option[A], codec: DbCodec[Option[A]]): Frag

object SkipIfNone extends SpecOptionMapping: //example (1)
  def frag[A](columnName: String, opt: Option[A], codec: DbCodec[Option[A]]): Frag =
    opt match
      case None => sql""
      case Some(a) =>
        val sqlString = s"$columnName = ${codec.queryRepr}"
        val params = Vector(opt)
        val writer: FragWriter = (ps, pos) =>
          codec.writeSingle(opt, ps, pos)
          pos + codec.cols.length
        Frag(sqlString, params, writer)

Then

Spec[User].whereCompany(userReq.company, SpecOptionMapping.SkipIfNone)

One other style for example (1) that requires no code changes is:

Spec[User]
  .where(sql"company = ${userReq.company}")
  .where(userReq.department.map(dpt => sql"department = $dpt").getOrElse(sql""))

And this for example (2):

Spec[User]
  .where(sql"company = ${userReq.company}")
  .where(userReq.department.map(dpt => sql"department = $dpt").getOrElse(sql"department IS NULL"))

Especially when refactoring code where a field becomes optional, it is easy to miss a where condition.

However like you say, this is still an issue. Safe refactoring is very important..

AugustNagro commented 4 weeks ago

Coming back to this, I think that, since we can use TableInfo to write the .where clauses in a refactorable way, we don't need additional methods on spec (like Spec[User].whereCompany(userReq.company, SpecOptionMapping.SkipIfNone)).

Instead I like the examples

For case (1):

val u = TableInfo[UserCreator, User, Long]

Spec[User]
  .where(sql"${u.company} = ${userReq.company}")
  .where(userReq.department.map(dpt => sql"${u.department} = $dpt").getOrElse(sql""))

And this for example (2):

val u = TableInfo[UserCreator, User, Long]

Spec[User]
  .where(sql"${u.company} = ${userReq.company}")
  .where(userReq.department.map(dpt => sql"${u.department} = $dpt").getOrElse(sql"${u.department} IS NULL"))

I'll close this ticket and add documentation to the readme.