fix(cloudkit): surface per-record modify errors instead of discarding them

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 '<unnamed>'}: {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."""

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"