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

Weird db read issue #1539

Closed
ahartman opened this issue May 1, 2024 · 7 comments
Closed

Weird db read issue #1539

ahartman opened this issue May 1, 2024 · 7 comments

Comments

@ahartman
Copy link

ahartman commented May 1, 2024

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

groue commented May 1, 2024

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

ahartman commented May 1, 2024 via email

@ahartman
Copy link
Author

ahartman commented May 1, 2024

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

groue commented May 1, 2024

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

groue commented May 1, 2024

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

groue commented May 1, 2024

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.

groue added a commit that referenced this issue May 1, 2024
Related issue: #1539
@groue groue closed this as completed May 1, 2024
@ahartman
Copy link
Author

ahartman commented May 2, 2024 via email

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

No branches or pull requests

2 participants