realm / realm-swift

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

Ensure Migration.enumerateObjects(ofType:_:) consistently uses property mappings #8659

Open AnthonyWharton opened 2 months ago

AnthonyWharton commented 2 months ago

Problem

When performing a Migration.enumerateObjects(ofType:_:), within the executed block (MigrationObjectEnumerateBlock) the old object and new object inconsistently use the property mappings for the given object.

Suppose the example snippet below:

class Example: Object {
    // ...
    @Persisted var someField: String = "default value"
    // ...

    override class public func propertiesMapping() -> [String : String] {
        [
            "someField": "some_field"
        ]
    }
}

Inside of a migration, when running enumerateObjects(ofType:_:), I observe the following behaviour within the MigrationObjectEnumerateBlock:

I understand this is probably to do with something under the hood when Realm actually maps the properties, and that changing this might be not worth the difficulty.

Even if this is decided to be the intended behaviour, this should be documented as I had to work this out through trial and error.

Solution

No response

Alternatives

No response

How important is this improvement for you?

I would like to have it but have a workaround

Feature would mainly be used with

Local Database only

sync-by-unito[bot] commented 2 months ago

➤ PM Bot commented:

Jira ticket: RCOCOA-2415

tgoyne commented 2 months ago

I don't think this was a specifically intended outcome, but I think it may be necessary? Supporting using the mapped name on oldObject can't work, since one of the points of the mapping is that it isn't persisted and if you're accessing a property which you removed the mapped name just doesn't exist anywhere. Supporting both the mapped and unmapped names in the new object would be very brittle, since you're allowed to have collisions there (e.g. column named "foo" mapped to property "oldFoo" and column named "newFoo" mapped to property "foo"). I think in retrospect we probably shouldn't have applied the mapping to the new object at all to be consistent, but changing that now is tricky.

You're absolutely right that at the minimum this really needs to be documented.

AnthonyWharton commented 2 months ago

Makes sense to me regarding the old objects, and yeah its a difficult one to remedy as your points are all very valid. I don't think being able to access both mapped and unmapped is what I was asking for, as collisions are possible as you say.

Given that changing the behaviour will result in legacy code being broken, I think documentation would be sufficient in this case.

I don't know what Realm's attitude to breaking changes is, but it could be staged for a future major release if still applicable?

tgoyne commented 2 months ago

It's the sort of breaking change that's really spooky because it has the potential for a developer to not notice the change and ship an update to their app which corrupts user data. In the worst case it leads to a migration block completing successfully but reading the wrong field, so even if the developer is testing old migrations they could possibly miss it. We could maybe do something like have a config option to opt into the correct behavior and then log a warning if property mappings are read in a migration without the option set?

AnthonyWharton commented 2 months ago

Seems pretty reasonable to me! Not a huge priority now that I know what is going on, but I think this could be a nice improvement for increasing consistency and lowering cognitive load when dealing with codebases that make heavy use of mappings!