Avoid overwriting local db column values in cases were CKRecord has no value.

[hishnash]

it appears that if a db record is created (or modified) while the sync engine is not active (such as running a data migration at the same time as the db migration as is common) then when the sync engine starts the SyncEngine.updateLocalFromSchemaChange method is called and this in turn courses the updateQuery method and here any new columns you may have added are included, however since there is no entry in CKRecord for these columns the query is constructed to set this value to NULL on conflict.

This change filters the fields used in the ON CONFLICT to only update fields were the CKRecord has some values. I wander if an additional change that might be worth having here is a call that updates the local CKRecord to keep it in sync with the local db so that data migrations are then correctly tracked?

I have also updated the method that is called when a cloud sync comes in over the wire to ensure that it also does not end up setting columns to NULL within the update block in cases were there is an existing value and the CKRecord has not indication of that column existing at all.

This change assumes that CKRecord always persists null against a columnName rather than skipping the column. I believe this is the correct assumption but maybe for columns with attachments it is not?

[mbrandonw]

Hi @hishnash, before we can consider a change like this we would need to see a test that demonstrates what it is fixing. The test should fail without these changes.

[hishnash]

Yer I am writing a test now that exposes this.

[hishnash]

@mbrandonw the added test fails without these changes applied, with the change the new column is skipped leaving its value in place.

I will try to figure out how to write a test that tests the other side of this when a remote change comes over the wire.

[hishnash]

Also I am not sure about how the system stores columns that have CKAsset types but are optional and there is no value set?

Should still expect a CKAsset object or should I like with encryptedValues be tracking the list of keys.

[mbrandonw]

Hi @hishnash, your test is not testing something that the SyncEngine is meant to handle. In particular, these lines here.

The sync engine must be initialized and connected to your database at all times (this is done in the app entry point). The SyncEngine doesn't necessarily need to be running. It is fine to call stop() on it at anytime, or even initialize it with startsImmediately: false. But it does need to be initialized and set up because we need the triggers to be running to see changes to your tables in order to enqueue changes to send to iCloud. Otherwise changes to your database are not noticed by the sync engine.

[hishnash]

I get that it is better for all changes to run within the runtime of the SyncEngine I'm not sure you can assume that will always be the case.

I think it would be prudent for the engine to not overwrite values that it does not have any knowledge of when it is not forced to do so. This block of code that runs after the schema change is intended to let a device copy in values it has from the remote server for new files from another device the user has that has already migrated not remove local values that are in the db that it does not have any record of.

For example if we were to track what rows we updated/inserted within a schema migration and then as you suggested in the other PR tapped the engine on the back by doing a update later these would not help as by that time the sync engine would have run updateLocalFromSchemaChange and nuked any local changed before it picks up new ones. Also we do need to consider that at any point other system extensions that also use the sync engine could fire up an instance.

[mbrandonw]

I get that it is better for all changes to run within the runtime of the SyncEngine I'm not sure you can assume that will always be the case.

Yes we can assume it because it's what we require. What you are asking for is analogous to asking that SwiftData be usable without a model container set up. It's just not meant to be how things are done. We require that a sync engine be set up as soon as the app launches before any writes to the database are made.

For example if we were to track what rows we updated/inserted within a schema migration and then as you suggested in the other PR tapped the engine on the back by doing a update later these would not help as by that time the sync engine would have run updateLocalFromSchemaChange and nuked any local changed before it picks up new ones.

My suggestion was off the cuff and I didn't think it through, and I now take it back. I do not think that is a viable way to handle things.

It would be worth the time to consider alternatives to "data migrations" that do not involve writing to the database while the sync engine is running. For example, if the data you need to seed are just constant values that should be available to all devices, you could probably get away with putting those values in a dedicated SQLite table that is not synchronized to iCloud.

Also we do need to consider that at any point other system extensions that also use the sync engine could fire up an instance.

I don't understand this point. Fire up an instance of what? Can you explain more?

---

There is a chance we may take your changes as just an overall safeguard, but even if we did we would still highly, highly recommend to not make changes to the database without the sync engine running. You are only working against the grain and are likely to cause bigger problems down the road.

[hishnash]

@mbrandonw I have this branch to be ontop of the latest changes.