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

ValueObservation filtering by foreign key #516

Closed
mackuba opened this issue Apr 16, 2019 · 7 comments
Closed

ValueObservation filtering by foreign key #516

mackuba opened this issue Apr 16, 2019 · 7 comments

Comments

@mackuba
Copy link

mackuba commented Apr 16, 2019

Hi, it's me again :) With the exact same observation actually as in #514.

I was writing some tests today for this code, and I've noticed that even though I'm trying to observe only changes in the photos/sequences set for a specific gallery, the fetch block is fired whenever a photo/sequence is added to any gallery. This normally doesn't happen in the app since only one gallery is being built at a time in the UI, but I created such situation in the test.

Here's some simplified sample code that seems to be reproducing this:

struct Gallery: Codable, FetchableRecord, MutablePersistableRecord {
    enum CodingKeys: String, CodingKey, ColumnExpression {
        case id
    }

    var id: Int?

    mutating func didInsert(with rowID: Int64, for column: String?) {
        self.id = Int(rowID)
    }

    static let photos = hasMany(Photo.self)

    var photos: QueryInterfaceRequest<Photo> {
        return request(for: Gallery.photos)
    }
}

struct Photo: Codable, FetchableRecord, MutablePersistableRecord {
    enum CodingKeys: String, CodingKey, ColumnExpression {
        case id
        case position
        case galleryId
    }

    var id: Int?
    var position: Int
    var galleryId: Int

    mutating func didInsert(with rowID: Int64, for column: String?) {
        self.id = Int(rowID)
    }

    init(galleryId: Int, position: Int) {
        self.galleryId = galleryId
        self.position = position
    }
}

var config = Configuration()
config.trace = { print($0) }

var migrator = DatabaseMigrator()
migrator.registerMigration("createTables") { database in
    try database.create(table: "gallery") { t in
        t.autoIncrementedPrimaryKey("id")
    }

    try database.create(table: "photo") { t in
        t.autoIncrementedPrimaryKey("id")

        t.column("galleryId", .integer).notNull()
            .indexed()
            .references("gallery", onDelete: .cascade)

        t.column("position", .integer).notNull()
    }
}

let database = DatabaseQueue(configuration: config)
try migrator.migrate(database)

var gallery1 = try database.write { db -> Gallery in
    var gallery = Gallery()
    try gallery.insert(db)
    return gallery
}

var gallery2 = try database.write { db -> Gallery in
    var gallery = Gallery()
    try gallery.insert(db)
    return gallery
}

var photo1 = try database.write { db -> Photo in
    var photo = Photo(galleryId: gallery1.id!, position: 1)
    try photo.insert(db)
    return photo
}

var photo2 = try database.write { db -> Photo in
    var photo = Photo(galleryId: gallery2.id!, position: 1)
    try photo.insert(db)
    return photo
}

let request = gallery1.photos

let builder: ((Database) throws -> ([Photo])) = { db in
    return try request.fetchAll(db)
}

try database.read { db in
    let photos = try request.fetchAll(db)
    print("Photos at the beginning: " + photos.map { String($0.id!) }.joined(separator: ", "))
}

let observation = ValueObservation.tracking([request], fetch: builder)

_ = observation.rx.fetch(in: database)
    .subscribeOn(MainScheduler.instance)
    .subscribe(onNext: { photos in
        print("Photos now: " + photos.map { String($0.id!) }.joined(separator: ", "))
    })

print("Adding photo to first gallery:")

try database.write { db in
    var photo = Photo(galleryId: gallery1.id!, position: 2)
    try photo.insert(db)
}

print("Adding photo to second gallery:")

try database.write { db in
    var photo = Photo(galleryId: gallery2.id!, position: 2)
    try photo.insert(db)
}

And I get this:

Photos at the beginning: 1
Photos now: 1
Adding photo to first gallery:
Adding photo to second gallery:
Photos now: 1, 3
Photos now: 1, 3
@groue
Copy link
Owner

groue commented Apr 17, 2019

Hello again @mackuba :-)

Thanks for the detailed report! You are experiencing normal behavior:

let request = gallery1.photos
let builder: ((Database) throws -> ([Photo])) = { db in
    return try request.fetchAll(db)
}
let observation = ValueObservation.tracking([request], fetch: builder)

This code observes gallery1.photos, which is akin to Photo.filter(Column("galleryId") == 1), or SELECT * FROM photo WHERE galleryId = 1. SQLite is unable to reports sharply tailored changes to this request.

Unlike #514, this is not bug in SQLite. Here we are facing a hard-coded limitation: SQLite can not spot individual rows unless they are identified by their integer primary key:

Photo.filter(Column("galleryId") == 1) // not precise
Photo.filter(Column("position") == 0)  // not precise
Photo.filter(Column("id") == 1)        // precise

Consequence: GRDB has to trigger the ValueObservation for any change that happens to the photo table.

In order to avoid undesired changes notifications, you have to instruct GRDB to perform deduplication of identical results.

You have two ways to do so:

  1. Use ValueObservation.trackingAll/One/Count instead of ValueObservation.tracking(_:fetch:):

    - let observation = ValueObservation.tracking([request], fetch: builder)
    + let observation = ValueObservation.trackingAll(request)

    Those methods won't trigger the observation unless raw database rows are actually different.

  2. Use the distinctUntilChanged() method:

    - let observation = ValueObservation.tracking([request], fetch: builder)
    + let observation = ValueObservation.tracking([request], fetch: builder).distinctUntilChanged()

    This method won't trigger the observation unless fetched values are actually different, according to the Equatable protocol.

    Note: RxSwift also has a distinctUntilChanged operator, which performs a similar job. You may still prefer the GRDB one, because the comparison happens on a background queue: you won't have to hop accross RxSwift schedulers in order to avoid the comparison to happen on your main thread. This also applies to map, compactMap...: favor ValueObservation transformations as much as possible.

@mackuba
Copy link
Author

mackuba commented Apr 17, 2019

Ah, I see... Ok, distinctUntilChanged has fixed it 👍

@groue
Copy link
Owner

groue commented Apr 17, 2019

OK @mackuba! I hope this issue will help other users who face the same surprise. Happy GRDB!

@groue groue closed this as completed Apr 17, 2019
@dawilliams-gpsw
Copy link

Hello @groue , we ran into this issue today. I notice that .removeDuplicates() exists on the ValueObservation but I wasn't able to find your first suggestion (.trackingAll). Is the recommended way to handle this to use .removeDuplicates() with Equatable types (or manually specifying the by closure)? Cheers.

@groue
Copy link
Owner

groue commented Apr 5, 2023

Hello @dawilliams-gpsw. Sample code was given for a former GRDB version indeed.

Here is the modern form (GRDB 6):

// Formerly ValueObservation.trackingAll(request)
let observation = ValueObservation.tracking { db in try request.fetchAll(db) }

As with all Swift closures, you can use the short form:

// Short version
let observation = ValueObservation.tracking(request.fetchAll)

@dawilliams-gpsw
Copy link

Hello @dawilliams-gpsw. Sample code was given for a former GRDB version indeed.

Here is the modern form (GRDB 6):

// Formerly ValueObservation.trackingAll(request)
let observation = ValueObservation.tracking { db in try request.fetchAll(db) }

As with all Swift closures, you can use the short form:

// Short version
let observation = ValueObservation.tracking(request.fetchAll)

Ah, yes, that is indeed the form we're using. In the closure, we're using something like:

try Parent
  .filter(id: someIdentifier)
  .joining(required: Parent.children)
  .asRequest(of: ParentInfo.self)
  .fetchAll(db)

and we are seeing observations triggered for identifiers that weren't someIdentifier. Using removeDuplicates() on the ValueObservation fixed that, but I wasn't sure if there was an alternative method to gain more sharply tailored observations. It sounds like removeDuplicates is the way. Cheers :)

@groue
Copy link
Owner

groue commented Apr 7, 2023

and we are seeing observations triggered for identifiers that weren't someIdentifier.

Oh, yes.

When SQLite notifies database changes, it identifies inserted/updated/deleted rows with their rowid (a numerical id). Some tables use this rowid as the primary key, and for those tables ValueObservation can focus change notifications on the observed rowids only. But for all other tables, tables with string or uuid primary keys for examples, it is not possible to tell that rowid 42 matches the id "1234-5678", or "0000-0000", or any other id. When SQLite notifies a change on the parent with rowid 42, GRDB must notify a change for your request, just in case this rowid 42 would be the parent with id someIdentifier.

This explains the extra change notifications you see. Using removeDuplicates is a good technique when you want to filter them out 👍

If you're interested, check DatabaseRegion:

let request = Parent
  .filter(id: someIdentifier)
  .joining(required: Parent.children)
let region = try request.databaseRegion(db)
print(region)

If the code above prints "parent(*),child(*)", then the tracked region is all columns or all rows of both parent and child tables.

For some requests, you'll see player(id,name)[3]. This means: the tracked region is the id and name columns of the row with rowid 3 in the player table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants