groue / GRDB.swift

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

Weird db read issue #1539

Closed ahartman closed 2 months ago

ahartman commented 2 months ago

Execute read query filtering on a column == nil

Retrieve only records with filtered nil value

Some records are included as with nil value although they contain a real value in that column

Environment

GRDB flavor(s): GRDB GRDB version: Master Installation method: Package Xcode version: latest Swift version: 5 Platform(s) running GRDB: macCatalyst macOS version running Xcode: latest

Demo Project

I attach a screen image.

Scherm­afbeelding 2024-05-01 om 12 04 17

You can see the query in 'getNoGeoPatients', part of the results below there and the db records on the left. Please observe: Record 14 ' Allessandro Tina' is not retrieved as it contains a value for patientLatitude Record 17 'Allard Veronique' is retrieved with nil for patientLatitude and nil for patientLongitude, although the database contains values for patientLatitude and patientLongitude.

Total mystery for me, I hope you have an idea. Regards, André Hartman

groue commented 2 months ago

Hello @ahartman, welcome back.

Probably the application on the right of the screenshot does not fetch values from the database displayed on the left. Or maybe the database was modified since the DB Browser has performed its last fetch.

ahartman commented 2 months ago

Dear Gwendal,

I deleted my issue again but you already read it. It is indeed that the db has changed compared to the screen image. I wil find out on my own as it must be in my code. Nice job for a national holiday today.

Regards, André Hartman

Op 1 mei 2024, om 12:35 heeft Gwendal Roué @.***> het volgende geschreven:

Hello @ahartman https://github.com/ahartman, welcome back.

Probably the application on the right of the screenshot does not fetch values from the database displayed on the left. Or maybe the database was modified since the DB Browser has performed its last fetch.

— Reply to this email directly, view it on GitHub https://github.com/groue/GRDB.swift/issues/1539#issuecomment-2088269646, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFIBPA7UTFQIOFRGGSDRNTZADAN5AVCNFSM6AAAAABHBYHJCKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYGI3DSNRUGY. You are receiving this because you were mentioned.

ahartman commented 2 months ago

I found the issue and you may be interested. I create a table as below:

try db.write { db in
                try db.create(table: "patient", ifNotExists: true) { t in
                    t.autoIncrementedPrimaryKey("id")
                    t.column("patientName", .text)
                        .indexed()
                        .unique()
                        .notNull()
                    t.column("patientLatitude", .real)
                        .indexed()
                    t.column("patientLongitude", .real)
                }
            }

patientLatitude and patientLongitude are optional values and I want to protect existing values in the db from being overwritten with NULLS, as below:

func updateDBFromPatients(patients: [PatientInfo], dates: PeriodStartEnd) {
        do {
            try db.inTransaction { db in
                for patient in patients {
                    var tempPatient = PatientInfo.Patient(
                        patientName: patient.patientName
                    )
                    tempPatient = try tempPatient.upsertAndFetch(
                        db, onConflict: ["patientName"],
                        doUpdate: { _ in
                            [Column("latitude").noOverwrite,
                             Column("longitude").noOverwrite]
                        }
                    )
                    for visit in patient.visits {
                        var tempVisit = visit
                        tempVisit.patientId = tempPatient.id!
                        try tempVisit.upsert(db)
                    }
                }
                return .commit
            }
        } catch {
            fatalError("Unresolved error \(error)")
        }
    }

The issue is as follows: as some point in time I renamed the db columns latitude and longitude into patientLatitude and patientLongitude. I forgot to update the corresponding column names in the function updateDBFromPatients, so in the function there are still latitude and longitude as column names. That invalidated the overwrite protection, which is why patientLatitude and patientLongitude were overwritten with NULL at each update.

The weird thing is there are no errors or warnings about using column names (latitude and longitude) that do not exist in the table.

groue commented 2 months ago

I forgot to update the corresponding column names in the function updateDBFromPatients, so in the function there are still latitude and longitude as column names.

To prevent this kind of programmer error, I tend to define column constants. Often I derive columns constants from CodingKeys, which are compiler-generated for Codable records. They make sure no such mistep can happen:

struct Patient: Codable {
  var patientLatitude: CLLocationDegrees
  var patientLongitude: CLLocationDegrees
}

extension Patient: TableRecord {
  // Derive Column constants from CodingKeys.
  // Those automatically detect renamed properties.
  enum Columns {
    static let patientLatitude = Column(CodingKeys.patientLatitude)
    static let patientLongitude = Column(CodingKeys.patientLongitude)
  }

  // Possible alternative for non-Codable records.
  // Those do not automatically detect renamed properties, though.
  enum Columns: String, ColumnExpression {
    case patientLatitude, patientLongitude
  }
}

The upsert would then read:

func updateDBFromPatients(patients: [PatientInfo], dates: PeriodStartEnd) {
  ...
  tempPatient = try tempPatient.upsertAndFetch(
    db, onConflict: ["patientName"],
    doUpdate: { _ in
      [Patient.Columns.patientLatitude.noOverwrite,
       Patient.Columns.patientLongitude.noOverwrite]
    }
  )
  ...
}

When I'm bothered by the long column names, I sometimes define a private typealias in the file that needs them:

private typealias PColumns = Patient.Columns

func updateDBFromPatients(patients: [PatientInfo], dates: PeriodStartEnd) {
  ...
  tempPatient = try tempPatient.upsertAndFetch(
    db, onConflict: ["patientName"],
    doUpdate: { _ in
      [PColumns.patientLatitude.noOverwrite,
       PColumns.patientLongitude.noOverwrite]
    }
  )
  ...
}
groue commented 2 months ago

The weird thing is there are no errors or warnings about using column names (latitude and longitude) that do not exist in the table.

😬 This is because columns which are not overwritten are not present in the generated UPSERT SQL query. That's probably something GRDB could detect...

groue commented 2 months ago

That's probably something GRDB could detect...

Yes, that is possible. We could enhance GRBD so that it throws some kind of "no such column" error. I'm adding this to the TODO list.

ahartman commented 2 months ago

Dear Gwendal,

I think it is more something that SQLite should detect in a submitted query with columns that do not exist.

Regards, André Hartman

Op 1 mei 2024, om 13:40 heeft Gwendal Roué @.***> het volgende geschreven:

Closed #1539 https://github.com/groue/GRDB.swift/issues/1539 as completed.

— Reply to this email directly, view it on GitHub https://github.com/groue/GRDB.swift/issues/1539#event-12669544762, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFIBPDKKBJ5OR77HUKWCITZADIDPAVCNFSM6AAAAABHBYHJCKVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGY3DSNJUGQ3TMMQ. You are receiving this because you were mentioned.