Set CKRecord.modificationDate in MockCloudDatabase

[lukaskubanek]

Motivation

SQLiteData’s tests leverage MockCloudDatabase, which currently doesn’t populate the modificationDate on saved records. Since this value is checked by the sync machinery, the tests effectively exercise different behavior than in production.

This has been first noticed while documenting the current sync behavior while elaborating on custom conflict resolution support (search for modificationDate) and further confirmed as a known issue here.

This is another step toward bringing MockCloudDatabase closer to CloudKit’s actual behavior, and an intermediate step toward adding support for configurable conflict resolution.

Implementation

Similar to record change tags, MockCloudDatabase now assigns incrementing dates to records saved in modifyRecords(…), mirroring CloudKit’s server-side behavior. However, since the codebase checks the actual CKRecord.modificationDate, this PR, unlike the current handling of change tags, swizzles the Objective-C getter to return the value stored in CKRecord._modificationDate via associated object storage. This is safe in production, as the global swizzle is only triggered when setting CKRecord._modificationDate, which is limited to MockCloudDatabase, used exclusively in tests. See the next section for alternative approaches that were considered.

Similar to the change tag, the injected modification date must survive the SQLite roundtrips, which is why both CKRecord.SystemFieldsRepresentation and CKRecord._AllFieldsRepresentation were extended to persist _modificationDate alongside the existing _recordChangeTag using NSKeyedArchiver/NSKeyedUnarchiver.

My assumption is that when saving a record, CloudKit only sets the modificationDate on that record and it doesn’t “touch” the parent to modify its modificationDate, as it apparently does with the recordChangeTag. I’m not 100% sure about this behavior, though.

Alternative Approaches

One way to circumvent swizzling would be to subclass CKRecord, override modificationDate, and return a mocked record from MockCloudDatabase.modifyRecords(…). Not only is it tricky to change the class of the copied record due to its internal implementation, but CloudKit now logs the following message when attempting to subclass CKRecord:

BUG IN CLIENT OF CLOUDKIT: CKRecord should not be subclassed (`…MockRecord`). If it is, Sendable may no longer apply. This will become an error in a near future release.

Another idea would be to use a facade property that returns the mock version and falls back to the real value if not set. Something like CKRecord.effectiveModificationDate. However, this would pollute the API and require all call sites in the library to go through this property, which is quite error-prone.

I’ve come to the conclusion that swizzling is the right tool for the job here, but I’m open to discussing further.

Also, I initially started with using @Dependency(\.date.now) for providing dates set to the modificationDate in MockCloudDatabase. However, giving the mock database control over the values felt more natural and better aligned with how record change tags are handled, so I ended up using the same pattern.

Impact on Test

When running the test suite with this change, there is one test failing, MergeConflictTests.clientRecordUpdatedBeforeServerRecord():

@@ −10,9 +10,9 @@
         dueDate🗓️: 0,
         id: 1,
         id🗓️: 0,
         isCompleted: 1,
-        isCompleted🗓️: 30,
+        isCompleted🗓️: 60,
         priority🗓️: 0,
         remindersListID: 1,
         remindersListID🗓️: 0,
         title: "Buy milk",

A single timestamp diff in the last assertion might seem minor, but there is an important conceptual decision to be made. This flow might not be immediately obvious, so let me break it down:

Step	Event	SyncMetadata.userModificationTime	CKRecord.🗓️	CKRecord.title🗓️	CKRecord.isCompleted🗓️
1	Initial setup	0	0	0	0
2	Simulate server-side change of title="Buy milk"	n/a	60	60	0
3	Update locally isCompleted=true	30	60	60	0
4	Process local changes & hit .serverRecordChanged error	60	60	60	0
5	Process local changes	60	60	60	60
6	Fetch record zone changes	60	60	60	60

Note

The CKRecord timestamps in the table refer to the server state, i.e. the MockCloudDatabase, not the last-known server record stored in the SyncMetadata.

In step 4, when hitting the .serverRecordChanged error, the server record’s CKRecord.userModificationTime is promoted to be the new SyncMetadata.userModificationTime following this logic: max(SyncMetadata.userModificationTime, CKRecord.userModificationTime), max(30, 60), 60.

This means that in step 5, when processing the local changes again after resolving the conflict, the isCompleted change is now considered to happen at t=60. This means that the original timestamp of the change is lost and I’m wondering if this behavior, which already occurs in production (!), is desired.

I’d say yes, because with support for configurable conflict resolution via a three-way merge, an entirely new value may emerge rather than selecting either the local or server value. And in that case it should be marked as changed at the current SyncMetadata.userModificationTime.

Do you agree? Can we accept the change in the test?

Follow-up Ideas

I was thinking of two follow-up PRs:

1. I’d like to refactor the merge conflict tests to make the flow clearer. I’ve previously done some work on that in #353, but I’ve abandoned that one, and would start from scratch and base it on this change.
2. I’d extend the swizzling pattern in the MockCloudDatabase to CKRecord._recordChangeTag to align it with CKRecord._modificationDate. Currently, the library doesn’t seem to check CKRecord.recordChangeTag directly, but it is desired to use for detecting conflicts with support for configurable conflict resolution.

[mbrandonw]

Hey @lukaskubanek, thanks so much for exploring this! At first glance this seems like a good approach, and the fact that a test needs to be updated is fine and probably to be expected. We will look in more depth very soon.

[lukaskubanek]

Here’s a follow-up after reviewing the comments in #386 starting here.

If I understand this correctly, aligning the test and production environments means in-flight records are no longer eagerly stored as last-known server records from nextRecordZoneChangeBatch(…). When records are prepared for sending, the modification dates of the current last-known server record (lastKnownDate) and the passed-in record (recordDate) are now either nil or equal.

This is how the reminder record from the affected test evolves in refreshLastKnownServerRecord(…):

Source	Effect	lastKnownDate	recordDate
nextRecordZoneChangeBatch	update (initial)	nil	nil
handleSentRecordZoneChanges	update (initial)	nil	1
nextRecordZoneChangeBatch	no update (dates equal)	1	1
nextRecordZoneChangeBatch	no update (dates equal)	2	2
handleSentRecordZoneChanges	update (dates differ)	2	4

Previously, the modification dates were always nil, which caused refreshLastKnownServerRecord(…) to update on every invocation. The idea of using eagerly stored in-flight records to track timestamps for user modifications may be based on that behavior.

With this change, however, we effectively observe what you, @mbrandonw, described here when completely removing the call that attempts to store the in-flight record in nextRecordZoneChangeBatch(…).

This suggests that the logic for tracking timestamps via eagerly stored in-flight records does not actually hold. It likely never did—except during initial record saves (which is still the case) and previously in tests (which is no longer the case).

I was able to confirm that the updateLastKnownServerRecord() call is never reached from the record modification-date checking branch when refreshLastKnownServerRecord(…) is invoked from the nextRecordZoneChangeBatch(…) source. This holds both in the test suite and in production when running an example app.

This reopens the question of the role of the last-known server record, and whether it can serve as the ancestor in a three-way merge during conflict resolution. But let’s address this separately to keep this PR focused…