caiyue1993 / IceCream

Sync Realm Database with CloudKit
MIT License
1.95k stars 248 forks source link

Issue when fetching objects with reference/relationship #140

Open davidrothera opened 5 years ago

davidrothera commented 5 years ago

Similar to #95 there is an issue when doing a "fresh" fetch from a device (for example after installing on a new device) where sometimes objects have their reference fields set to nil.

This issue only happens sometimes and so its clear that its a race condition in that the child objects are being created locally before the parent ones.

Expected behavior

All objects fetched from iCloud correctly

Actual behavior(optional)

Some objects fetched with bad linking relationships which causes issues and even ends up syncing bad data back to iCloud

Steps to reproduce the problem(optional)

davidrothera commented 5 years ago

The issue comes from the section of code where we are inserting objects to the Realm, dynamicObject(ofType:forPrimaryKey) can return a nil (which it would if said item didn't yet exist.

https://github.com/caiyue1993/IceCream/blob/a15c7e76b0ce351d111246c357a356c79f3c2a8e/IceCream/Classes/CKRecordRecoverable.swift#L78-L81

One way I can think of solving this is that all objects which are being inserted are added to a queue, if you try and resolve an object like this and fail then you put that item to the back of the queue.

You would still need to solve for the issues of if the reference is never valid in which case the queue could hold some kind of struct holding both the CK item as well as some kind of counter of how many times the action has been retried and at a certain stage give up.

davidrothera commented 5 years ago

After looking into it further I think adding CKRecord objects to a queue would work however it would completely change the way that things work as at the moment every time a record is downloaded it is updated once fetched.

Another possibility here is to change the pull() method to be able to take an optional completion which is executed once the fetch is complete, this way you resolve the issue by:

luispadron commented 5 years ago

I'm also experiencing this issue. Seems to be random at times? I have models that look like:

class Class {
    var id: UUID
    var semester: Semester?
    //...
}

class Semester {
    var id: UUID
    var year: Int
    var term: String
    //...
}

What ends up happening is that the Class objects (the parents) are always synced and ready across devices, that is, adding a class on one device will populate the database of another device with that new class object. However, sometimes the semester field of the new class object will be nil in Realm on the other device.

I've added a completion handler to pull on my fork of this repo and this doesn't seem to have fixed the issue. I also tried calling pushAll after adding any class objects to the database and again, does not seem to fix the issue.

My project is open-source here in case you would like to debug (it's rather big) so maybe using the example @davidrothera would be better.

I'm new to CloudKit but not Realm so I may be doing something wrong on the CloudKit side of things but I don't believe so since it works correctly at times (syncs everything).

Any ideas?

d-sandman commented 5 years ago

Same situation with the to many relationship:

class Person: Object {
  // ...
  let dogs = List<Dog>()
  @objc dynamic var cat: Cat?
  // ...
}
class Dog: Object {
  // ...
  let owners = LinkingObjects(fromType: Person.self, property: "dogs")
  // ...
}
class Cat: Object {
  // ...
  let owner = LinkingObjects(fromType: Person.self, property: "cat")
  // ...
}

Neither of references gets restored on initial fetch, not sometimes but always.

d-sandman commented 5 years ago

As soon as I understand the problem occurs when we try to create a reference on an object which wasn't yet imported? The simplest solution I could think of could be importing references after all records are imported. As an example, Realm does exactly this when converting local realm to synced https://realm.io/docs/cookbook/js/local-realm-to-synced/

PS: Honestly I still think that I'm doing something wrong, I refuse to believe that so many people use it without references being synced :) @caiyue1993 if only you could shed some light! And thanks for the amazing work!

plivesey commented 5 years ago

I'm not sure if this helps (I'm new to both this library and cloudkit), but I don't think to-many relationships are supported: https://github.com/caiyue1993/IceCream/blob/e66225ae7c91ec8f2bce8ce62ddf7079ce0e1e28/IceCream/Classes/CKRecordConvertible.swift#L126

I just found this now and am a bit confused since this seems to be a massive limitation of the library. I'm going to look more into this tomorrow...

davidrothera commented 5 years ago

I might be wrong but I believe it’s meaning many-to-many which require a look through table to implement usually where as most of the above reported issues have been one-to-many

plivesey commented 5 years ago

No, that sounds right...lmk if you find anything here. I'll let you know if I find anything which could help. (Also, I decided not to implement to many relationships and instead am just doing the inverted way)

plivesey commented 5 years ago

So yeah, you were right. And I've also ran into the same problem here and do think that it's a bug in IceCream. If it helps, I solved it by always resolving the sync objects in order. So, parents are always written first, followed by children.

Of course, this isn't a good generic solution because it's possible to have a child and parent of the same type. But for me, that's not a problem (I don't have that relationship), and it sounds like you may not either? So, maybe you can do the same thing.

Here's the commit with my work (it's not tested yet, but I think something like this should work). It's based off my own fork, so you can't copy the code directly, but it's pretty simple: https://github.com/plivesey/IceCream/commit/24c2b84e1a0da8e7f2f3bf2c757d6d27b960da24

ivanvorobei commented 5 years ago

I found this also. And I use relations. @plivesey , you found solution? You can describe more details about it?

plivesey commented 5 years ago

@ivanvorobei did you see the commit I referenced above? Basically, it processes the objects in order from parent to child. You could also take a look at my fork which has many bug fixes and changes (like shared dbs). It still needs a lot of work but eventually, I'm hoping to merge this back in.

ivanvorobei commented 5 years ago

@plivesey for me strange situation. I have to class - Debt and Person.

class Debt: Object {
    @objc dynamic var owner: Person?
}

It to-one relationships. Also class Person:

class Person: Object {
    let debts = LinkingObjects(fromType: Debt.self, property: "owner")
}

I use this scheme because Person can have many debts, but debt have one person.

I think Debt is parent in this example. It true? But if start sync parent, I have trouble with sync (because Persons sync after).

I decided that I have the wrong model, but looked at the realm website. Example of Many-to-one here: https://realm.io/docs/swift/latest/#relationships

If I am right, your commit work incorrect sometimes (for my example). I propose use order when we configure IceCream. We pass objects for sync when configure, maybe can use this order of objects. Write me what you think about it?

SyncEngine(objects: [FistSync, SecondSync...], databaseScope: .private)

Sorry for long-read.

plivesey commented 5 years ago

Oh yeah...parent/child is confusing here. Not sure what the right terminology is here. In this case, I think your model is correct. Your model is actually correct. It's unfortunate, but CloudKit does one to many relationships differently: https://developer.apple.com/library/archive/documentation/DataManagement/Conceptual/CloudKitQuickStart/AddingReferences/AddingReferences.html. So you can't use the canonical realm way of doing it.

Given my code above, you should sync Person first, and then Debt.

I think my commit works correctly for your example, you just need to pass in the person sync object first and the debt sync object second. This line is what should handle this: https://github.com/plivesey/IceCream/commit/24c2b84e1a0da8e7f2f3bf2c757d6d27b960da24#diff-972caea3ef15b318792017da9ba0f50aR164

ivanvorobei commented 5 years ago

@plivesey in your commit check order fir objects? Or how sync start Person?

plivesey commented 5 years ago

When you create the sync engine:

self.engine = SyncEngine(objects: [SyncObject<Person>(...), SyncObject<Debt>(...)])

This ensures that all person objects will be added to the db before debt objects and therefore there won't be any missing references.

plivesey commented 5 years ago

So, the order is created here on initialization.

ivanvorobei commented 5 years ago

@plivesey Oh, true, I mean it. I propose it in my long-read, maybe my English bad) but I did not understand - is it only in your version or is it in the original version? In original it not work, I am test.

UPD: sorry, understand. Maybe propose it to merge?

plivesey commented 5 years ago

This is only in my version. It's only in this commit: https://github.com/plivesey/IceCream/commit/24c2b84e1a0da8e7f2f3bf2c757d6d27b960da24 which is only in my fork. My fork has a bunch of changes which I do eventually intend to merge back in, but I keep changing things as I find better ways of doing things/find bugs.

adamsmaka commented 4 years ago

So unsupported List<Dog>() is a bug which could be fixed soon or should I reconstruct my app to handle this case? My model has to-many relationship to the same ObjectType and it's lost on app restart when IceCream refetch data from iCloud.

Fonceur commented 4 years ago

This is only in my version. It's only in this commit: plivesey@24c2b84 which is only in my fork.

Any chance you could make a small pull request with just that reordering code, as it seems to be a good work around for now?

plivesey commented 4 years ago

@Fonceur sorry, but I ended up just writing my own solution because ice cream was missing too many features. So, I don't really remember much about this code/have the time to get it read to merge. Sorry about that, but good luck.