realm / realm-swift

Realm is a mobile database: a replacement for Core Data & SQLite
https://realm.io
Apache License 2.0
16.34k stars 2.15k forks source link

Object Change Observers: Partial-KeyPaths Broken for Multi-Level Properties #8473

Open bdkjones opened 10 months ago

bdkjones commented 10 months ago

How frequently does the bug occur?

Always

Description

There is a bug when using partial-keypaths in object change observers. Consider these three model objects, which form a "tree" relationship (Client --> Project --> Job) and would appear in an NSOutlineView:

final class Client: Object
{
    @Persisted var name: String
    @Persisted var projects List<Project>
}
final class Project: Object
{
    @Persisted var name: String
    @Persisted var jobs: List<Job>

    // There should only ever be one parentClient, but Realm does not provide singular LinkingObjects
    @Persisted private var parentClients: LinkingObjects<Client> = LinkingObjects(fromType: Client.self, property: "projects")
    var parentClient: Client? {
        return parentClients.first
    }
}
final class Job: Object
{
    @Persisted var name: String

    // There should only ever be one parentProject, but Realm does not provide singular LinkingObjects
    @Persisted private var parentProjects: LinkingObjects<Project> = LinkingObjects(fromType: Project.self, property: "jobs")
    var parentProject: Project? {
        return parentProjects.first
    }
}

Now, suppose we have a Job object called someJob. We want to be notified when the name of the parent Project or Client changes so that we can update some UI. We can do that with hard-coded strings in an object-change observer, like this:

token = someJob.observe(keyPaths: ["name", "parentProjects.name", "parentProjects.parentClients.name"]) { [weak self] change in
    switch change
    {
         case .change:
             print("Change reached!")  // Update UI here.
         default:
             return   // We don't care about initial state.
         }
    }
}

The above approach works perfectly! The notification fires whenever the name property changes on the Job, Project, or Client.

BUT if we replace the hard-coded strings with partial keypaths, the notification DOES fire when someJob.parentProject?.name changes, but does NOT fire when someJob.parentProject?.parentClient?.name changes:

token = someJob.observe(keyPaths: [\Job.name, \Job.parentProject?.name, \Job.parentProject?.parentClient?.name]) { [weak self] change in
    switch change
    {
         case .change:
             print("Change reached!")  // Update UI here. DOES NOT FIRE for parentProject.parentClient.name changes.
         default:
             return   // We don't care about initial state.
         }
    }
}

The partial keyPaths are MUCH preferred because the compiler can help us if we refactor code. I do not believe they should break if more than one level of parent-child-relationship is traversed. Since the string version works just fine, I assume this is a bug in how Realm handles partial keyPath subscriptions.

Note that I use a computed property so that my model objects expose only a single parent, but I get the exact same result if I specify the keypaths this way instead:

[\Job.name, \Job.parentProjects.first?.name, \Job.parentProjects.first?.parentClients.first?.name]

Changing the Project name fires the notification; changing the Client name does not.

Stacktrace & log output

N/A

Reproduction Steps

See Description above.

Version

10.45.3

What Atlas Services are you using?

Atlas Device Sync

Are you using encryption?

No

Platform OS and version(s)

macOS 14.3

Build environment

Xcode version: 15.2 (15C500b) Dependency manager and version: SPM

Jaycyn commented 10 months ago

While Realm objects are KVO compliant, linking objects are a bit different.

From the Realm Documentation on Key-Value Observation

You cannot observe LinkingObjects properties via Key-value observation.

Due to that limitation, we've implemented code to create our own relationships. While you give up some of the free stuff linking objects provides, manually handling that isn't a big deal. And it seems in your case you're not really using backlinks to multiple objects (many-many) as there is only ever one parent object.

What we do is to create the 1-many manually; here's three updated models

class Client: Object {
    @Persisted var name: String
    @Persisted var projects: RealmSwift.List<Project>

    func addProject(withProject: Project) {
        withProject.parentClient = self
        self.projects.append(withProject)
    }
}

class Project: Object {
    @Persisted var name: String
    @Persisted var parentClient: Client!
    @Persisted var jobs: RealmSwift.List<Job>

    func addJob(withJob: Job) {
        withJob.parentProject = self
        self.jobs.append(withJob)
    }
}

class Job: Object {
    @Persisted var name: String
    @Persisted var parentProject: Project!
}

and then the observer which is about the same

self.token = job.observe(keyPaths: [\Job.name, \Job.parentProject.name, \Job.parentProject?.parentClient.name]) { [weak self] change in
    switch change
    {
     case .change:
         print("Change reached!")  // Update UI here. DOES NOT FIRE for parentProject.parentClient.name changes.
     default:
         return   // We don't care about initial state.
     }
}

Then to populate

let client = Client()
let project = Project()
let job = Job()

client.name = "client name"
project.name = "project name"
job.name = "job name"

client.addProject(withProject: project)

project.addJob(withJob: job)

try! realm.write {
    realm.add(client) //adds all objects to realm
}

Then when any name is updated

let job = realm.objects(Job.self).first!
let parentProj = job.parentProject
let parentClient = parentProj?.parentClient

try! realm.write {
    job.name = "job changed name"
}

try! realm.write {
    parentProj!.name = "parent proj changed name"
}

try! realm.write {
    parentClient!.name = "parent client changed name"
}

the observer is called for each name change

Change reached!
Change reached!
Change reached!
bdkjones commented 10 months ago

@Jaycyn Sure, I see that approach. But there are two complications:

1) The LinkingObjects keyPaths DO work when they're specified as Strings rather than PartialKeyPaths. This implies that Realm can handle this.

2) Model objects where the parent has a strong reference to the child and the child has a strong reference to the parent are, in any other world, bad. It's a retain cycle. The only reason it works in Realm is that the properties are lazily-created on demand. A child object definitely shouldn't "own" its parent, though, and I'd like to avoid that anti pattern if at all possible.

Jaycyn commented 10 months ago

@bdkjones We've had ongoing issues with attempting to 'make it work'. Every time we went down that path, it ended up being intermittent and a bit wonky. The issues with linkingObjects becomes even bigger in a sync'ing situation as then it really fails.

Since the documentation states that functionality is not supported (it appears to work in some cases and then later it doesn't, especially with sync'ing), we decided to abandon the attempt as it may work now, "but not work on a later release", as was suggested to us by support a while back i.e. avoid doing that.

In reference to retain cycles, 100% agree! However, the objects here are not child objects - they are references to another object and are fully fledged objects on their own. Keep in mind the relationship is optional so if an object is deleted and deallocated for example, that reference goes to nil

If you run the debug memory graph on the code I provided, all of the objects play nicely - so far! :-)

We feel this functionality should exist and be supported so I strongly suggest asking for a feature request!

If you want to see what I mean, change your hard-coded observer to this

self.token = job.observe(keyPaths: ["parentProjects.parentClients.name"]) { [weak self] change in

Which should add an observer to watch for the parentClient name change. So then change the parent client name

try! realm.write {
   parentClient!.name = "parent client changed name"
}

and see what is presented to the observer - no info that the clients name was changed even though that's the property being observed via the linking objects!

Property 'parentProjects' of object Job {
    name = job name;
} changed to 'LinkingObjects<Project> <0x15af58150> (
    [0] Project {
        name = project name;
        jobs = List<Job> <0x600002971ce0> (
            [0] Job {
                name = job name;
            }
        );
    }
)'

change your observer to get the details of what changed like this

case .change(let object, let properties):
   for property in properties {
      print("Property '\(property.name)' of object \(object) changed to '\(property.newValue!)'")
}

I could be totally off-base here but that indicates it doesn't really work... Correctly. Which jibs with the docs.

bdkjones commented 10 months ago

@jaycyn Makes sense. I hadn't seen that line in the docs previously and I haven't seen any failures with the string-based keyPaths, so I had no reason to suspect they weren't supported.

As for feature requests: that's a black hole of sadness and despair. There are requests more than 8 years old on here and every couple years someone asks for an update and...crickets. Asking Realm for features is like asking Apple for features: pointless.