From 48175a9a9aed6cc69c2bb8b87f3d6a8390d28887 Mon Sep 17 00:00:00 2001 From: mrjarnould Date: Fri, 4 Sep 2026 21:18:29 +0200 Subject: [PATCH] fix(cloudkit): surface per-record modify errors instead of discarding them CloudKit answers 200 for a modify whose records were rejected, and puts the reason in the record entry. `CKModifyResponse.records` is already typed to include `CKErrorItem`, but nothing looked: every caller scanned for a `CKRecord` with a matching name, did not find one, and reported something of its own invention. So a rejected write surfaced as "modify response missing record", which reads like a response-shape problem, and the only useful sentence in the response was thrown away. Photos filters errors out with an isinstance check; reminders does not look at all. modify() now raises CloudKitApiError naming every rejected record with Apple's code and reason, and carries the response as the payload. Live proof of what this recovers. `InvitesService.rsvp()` reported "RSVP modify response missing record". What Apple actually said was: CONFLICT (record to insert already exists) which is a different problem entirely, and points at its real cause. Closes #353 Co-Authored-By: Claude Opus 5 --- pyicloud/common/cloudkit/client.py | 33 +++++++++++- tests/test_cloudkit_client.py | 85 +++++++++++++++++++++++++++++- 2 files changed, 116 insertions(+), 2 deletions(-) diff --git a/pyicloud/common/cloudkit/client.py b/pyicloud/common/cloudkit/client.py index 632d2ccc..93365e83 100644 --- a/pyicloud/common/cloudkit/client.py +++ b/pyicloud/common/cloudkit/client.py @@ -16,6 +16,7 @@ from .base import CloudKitExtraMode, resolve_cloudkit_validation_extra from .models import ( CKDatabaseChangesResponse, + CKErrorItem, CKLookupDescriptor, CKLookupRequest, CKLookupResponse, @@ -265,6 +266,34 @@ def get_stream(self, url: str, *, chunk_size: int = 65536) -> Iterator[bytes]: close() +def _raise_for_record_errors(response: CKModifyResponse) -> None: + """Raise when CloudKit reported a per-record failure inside a 200 response. + + CloudKit answers ``200`` for a modify whose records were rejected, and puts + the reason in the record entry. Callers looking for their record by name + find a ``CKErrorItem`` instead, and every one of them silently skipped it: + a rejected write surfaced as a missing record, or as nothing at all. + + Raising here means the reason Apple gave -- "Attempt to save encrypted data + in non encrypted field type", say -- reaches the caller instead of being + replaced by a guess about the response shape. + """ + + errors = [record for record in response.records if isinstance(record, CKErrorItem)] + if not errors: + return + + detail = "; ".join( + f"{error.recordName or ''}: {error.serverErrorCode}" + + (f" ({error.reason})" if error.reason else "") + for error in errors + ) + raise CloudKitApiError( + f"CloudKit rejected {len(errors)} record(s) -- {detail}", + payload=response.model_dump(mode="json", exclude_none=True), + ) + + class CloudKitContainerClient: """Typed CloudKit client for a single container/environment/scope.""" @@ -436,12 +465,14 @@ def modify( ).model_dump(mode="json", exclude_none=True) data = self._http.post("/records/modify", payload) try: - return self._validate_response(CKModifyResponse, data) + response = self._validate_response(CKModifyResponse, data) except ValidationError as exc: raise CloudKitApiError( "Modify response validation failed", payload=data, ) from exc + _raise_for_record_errors(response) + return response def zones_list(self) -> CKZoneListResponse: """List the container's zones and return the parsed response.""" diff --git a/tests/test_cloudkit_client.py b/tests/test_cloudkit_client.py index c60a55bf..329875be 100644 --- a/tests/test_cloudkit_client.py +++ b/tests/test_cloudkit_client.py @@ -7,7 +7,12 @@ import pytest -from pyicloud.common.cloudkit import CKQueryObject, CKZoneIDReq +from pyicloud.common.cloudkit import ( + CKModifyOperation, + CKQueryObject, + CKWriteRecord, + CKZoneIDReq, +) from pyicloud.common.cloudkit.client import ( CloudKitApiError, CloudKitContainerClient, @@ -176,3 +181,81 @@ def test_cloudkit_client_download_asset_stream_closes_on_error() -> None: list(client.download_asset_stream("https://example.com/asset")) response.close.assert_called_once() + + +def _modify_op() -> CKModifyOperation: + """One trivial update operation, enough to send a modify.""" + + return CKModifyOperation( + operationType="update", + record=CKWriteRecord(recordName="R1", recordType="Thing"), + ) + + +def test_a_rejected_record_raises_with_apples_reason() -> None: + """CloudKit reports write failures per record, inside a 200 response. + + Every caller looked for its record by name, found a CKErrorItem instead of + a CKRecord, and skipped it -- so a rejected write surfaced as a missing + record or as nothing at all. The reason Apple gave has to reach the caller. + """ + + session = MagicMock() + session.post.return_value = _json_response({ + "records": [ + { + "recordName": "EventDetails:E1", + "serverErrorCode": "BAD_REQUEST", + "reason": "Attempt to save encrypted data in non encrypted field type", + } + ] + }) + client = CloudKitContainerClient("https://example.com/database", session, {}) + + with pytest.raises(CloudKitApiError) as exc_info: + client.modify(operations=[_modify_op()], zone_id=CKZoneIDReq(zoneName="Z")) + + message = str(exc_info.value) + assert "BAD_REQUEST" in message + assert "encrypted data in non encrypted field type" in message + assert "EventDetails:E1" in message + + +def test_every_rejected_record_is_named() -> None: + """A batch that fails in more than one way should say so once.""" + + session = MagicMock() + session.post.return_value = _json_response({ + "records": [ + {"recordName": "A", "serverErrorCode": "BAD_REQUEST", "reason": "bad"}, + {"recordName": "B", "serverErrorCode": "CONFLICT"}, + ] + }) + client = CloudKitContainerClient("https://example.com/database", session, {}) + + with pytest.raises(CloudKitApiError) as exc_info: + client.modify(operations=[_modify_op()], zone_id=CKZoneIDReq(zoneName="Z")) + + message = str(exc_info.value) + assert "2 record(s)" in message + assert "A: BAD_REQUEST (bad)" in message + assert "B: CONFLICT" in message + + +def test_a_successful_modify_still_returns_its_records() -> None: + """The guard must not disturb the path that already worked.""" + + session = MagicMock() + session.post.return_value = _json_response({ + "records": [ + {"recordName": "R1", "recordType": "Thing", "recordChangeTag": "TAG"} + ] + }) + client = CloudKitContainerClient("https://example.com/database", session, {}) + + response = client.modify( + operations=[_modify_op()], zone_id=CKZoneIDReq(zoneName="Z") + ) + + assert len(response.records) == 1 + assert response.records[0].recordName == "R1"