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"