groue / GRDB.swift

A toolkit for SQLite databases, with a focus on application development
MIT License
6.88k stars 708 forks source link

Row value nil-coalescing stopped working? #1632

Closed philmitchell closed 1 month ago

philmitchell commented 1 month ago

What did you do?

After upgrading from GRDB.swift/SQLCipher 5.x to 6.x, this no longer works:

let myInt: Int? = row["int_A"] ?? row["int_B"]

What did you expect to happen?

If row["int_A"] is nil and row["int_B"] has a value, myInt should receive that value.

What happened instead?

myInt is nil.

Environment

GRDB flavor(s): (GRDB, SQLCipher, Custom SQLite build?) GRDB.swift/SQLCipher, installed via CocoaPods

GRDB version: Fails in 6.24.1 (works in 5.26.1)

Installation method: (CocoaPods, SPM, manual?) CocoaPods

Xcode version: 16

Swift version: 5.10

Platform(s) running GRDB: (iOS, macOS, watchOS?) iOS

macOS version running Xcode: 14.6.1

groue commented 1 month ago

Hello @philmitchell,

I did not know GRDB 6 had introduced a breaking change here. That's why it is not listed in the migration guide. Maybe the cause of the change is the compiler version...

I could find this workaround:

let myInt: Int? = (row["int_A"] as Int?) ?? row["int_B"]
groue commented 1 month ago

I have closed the issue, but this does not mean I'm not interested in restoring the old feature, if possible: row["int_A"] ?? row["int_B"] looks pretty good.

I do not intend to personally work of this, though. This is an exercise left to the reader.

philmitchell commented 1 month ago

@groue I wasn't sure if this was a bug or a feature, so thank you for the nudge :) I'll take a look at this.

groue commented 1 month ago

Thanks @philmitchell

I'll take a look at this.

Now that I think about it, I'm not sure you'll succeed 😬 Use your free time with care.

The change was very likely introduced in 58293be - BREAKING: Optional is a database value like others . There used to be two distinct code paths, one for non-optional values, and one for optional values. Now there is a single one. This is why I'm not sure it is possible to instruct the compiler to be "smarter" in its handling of type inference and the ?? operator. And I'll reject any technique based on overloading the ?? operator.

groue commented 1 month ago

It happens that I'm doing some server work these days, and this morning I just met Postgres COALESCE. It also exists in SQLite: coalesce.

We could create a new Row.coalesce method:

extension Row {
    // Returns the first non-null value, if any
    func coalesce<T: DatabaseValueConvertible>(_ columns: [String]) -> T? { ... }

    // Returns the first non-null value, if any
    func coalesce<T: DatabaseValueConvertible>(_ columns: [any ColumnExpression]) -> T? { ... }
}

let myInt: Int? = row.coalesce(["int_A", "int_B"])
let myInt: Int? = row.coalesce([Column("int_A"), Column("int_B")])

There may exist better method names that could better fit the ecosystem. Something based on first, for example. Ideas welcome.

There is an argument for coalesce, though, which is that GRDB could also ship a free function with that name, just as we already ship min, cast, etc:


Player.select([
    Column("a"),
    max(Column("b"), Column("c")),
    cast(Column("d"), as: .real),
    coalesce([Column("e"), Column("f")]), // NEW!
    ...
    ])
philmitchell commented 1 month ago

Thanks for the hints @groue, I'll try not to spin my wheels on this. The coalesce idea is neat (!), but it does still leave this unexpected behavior hanging around to bite someone (ie., me in a month).

groue commented 1 month ago

If I had met this behavior of ??, I would have been just as surprised, and concerned, as you are. I also see how the suggestion of row.coalesce can look like a workaround for a bug. Even if there is a rationale for the current behavior, and if coalesce is the best we can give, I certainly do not want to prevent you from scratching that itch 👍