Skip to content

Return copied CKRecords when modifying database.#507

Merged
mbrandonw merged 5 commits into
mainfrom
mock-db-fix
Jul 24, 2026
Merged

Return copied CKRecords when modifying database.#507
mbrandonw merged 5 commits into
mainfrom
mock-db-fix

Conversation

@mbrandonw

Copy link
Copy Markdown
Member

We have a few places we are accidentally returning references from the mock database that exist in the DB's storage. That allows the outside to make edits to the references in the DB. I've put in a few extra copy()s to fix this.

@mbrandonw
mbrandonw requested a review from stephencelis July 23, 2026 18:23
Comment thread Tests/SQLiteDataTests/CloudKitTests/SyncEngineDelegateTests.swift Outdated
Co-authored-by: Stephen Celis <stephen@stephencelis.com>
@lukaskubanek

Copy link
Copy Markdown
Contributor

I didn't have time to check in detail, but it looks like a duplicate of #412, which I've filed a while ago.

@mbrandonw

Copy link
Copy Markdown
Member Author

Hi @lukaskubanek, yeah good catch, these PRs are closely related. Mine chose to fix the problem by forcing the copy()s to be in the mock database so that we never return references to stored data (value types would fix this, but I think we're stuck with references). So I think we can close your PR and I've added you as a co-author to this PR (hopefully GH picks up the attribution!). Thanks for investigating the problem and sorry we didn't get to it earlier.

And I know there's a lot more we need to get back into, so hopefully sometime soon :)

Co-Authored-By: Lukas Kubanek <533299+lukaskubanek@users.noreply.github.com>
@lukaskubanek

Copy link
Copy Markdown
Contributor

Hi @mbrandonw, I just looked at the diffs and your approach in this PR is way more robust and handles all the cases related to shared CKRecord references in the MockCloudDatabase. In #412, I only addressed the minimal change for the purpose of getting to customizable conflict resolution. So yeah, this PR supersedes mine, so I’ll go ahead and close it. And thanks for the attribution!

@mbrandonw
mbrandonw merged commit 63a2ff6 into main Jul 24, 2026
5 checks passed
@mbrandonw
mbrandonw deleted the mock-db-fix branch July 24, 2026 17:08
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.

3 participants