Skip to content

Deliver latest record state in ModifyRecordsCallback test helper#510

Open
lukaskubanek wants to merge 1 commit into
pointfreeco:mainfrom
structuredpath:fresh-records-in-modify-callback
Open

Deliver latest record state in ModifyRecordsCallback test helper#510
lukaskubanek wants to merge 1 commit into
pointfreeco:mainfrom
structuredpath:fresh-records-in-modify-callback

Conversation

@lukaskubanek

Copy link
Copy Markdown
Contributor

Motivation

The modifyRecords(…) test helper returns a ModifyRecordsCallback, which allows tests to notify the sync engine about server-side changes at a later point. This is useful for testing merge conflict scenarios that depend on ordering, such as fetch-before-retry and retry-before-fetch.

However, the callback currently captures the saved records at modification time. If the same record is modified again in the mock database before the callback is invoked, the callback replays stale record versions, so an earlier edit is delivered to the sync engine even though the mock database already holds a newer one. The deletion side has the equivalent problem, as a deferred deletion is still delivered even if the record has been re-created in the meantime. This deviates from CloudKit's behavior, where a fetch always reflects the latest server state. Production code is not involved, but tests relying on deferred callbacks currently exercise scenarios that real CloudKit can never produce.

The issue would become even more relevant with mocked CKRecord.recordChangeTag (a follow-up to #411 I have lined up), where stale change tags would break the conflict detection required for supporting customizable conflict resolution in the future.

This was originally discussed in this comment on #412, where filing it as a separate PR was suggested. I had planned to wait for #507, but the change turned out to be independent of it. It also doesn't reintroduce the shared reference issue addressed there, since record(for:) already returns copies of the stored records on main.

Implementation

The callback now reads the latest state from the mock database at invocation time instead of using the captured save results:

  • For modifications, each successfully saved record is re-read via record(for:), so the sync engine receives the current server state.
  • For deletions, the event is skipped if the record has been re-saved between the original delete and the callback invocation, mirroring how a real fetch would no longer report such a record as deleted.

Both behaviors are covered by regression tests that fail on main and pass with this fix.


Disclaimer: AI assistance (Claude Code with Fable model) was used to prepare this PR. All changes and verifications were reviewed by me.

@mbrandonw

Copy link
Copy Markdown
Member

Hi @lukaskubanek, thanks for this!

It mostly looks good, but I have a few suggestions. I'm not able to push directly to your branch for some reason, but I think we should:

  • Drop the ReturnValue generic from ModifyRecordsCallback. It's not actually used, and if we keep it we have to answer the question of what do we return? The saved/deleted results or the fresh results (as of now you have not updated it to return fresh results).
  • Batch all of the record fetching into a single records(for:) so that it is all done in a single transaction instead of a bunch of transactions.

I have pushed these changes to a branch of mine: e5d1563, 8b40128. Feel free to cherry pick them or re-implement them from scratch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants