[mstilkerich]

CardDAV: sync-collection with a limit loses changes

Problem

When a client sends a sync-collection REPORT with a previous sync-token and a DAV:limit, Sabre\CardDAV\Backend\PDO::getChangesForAddressBook() adds a LIMIT to the changes query, but:

• always returns the address book's current sync-token, and
• never sets result_truncated, so no 507 response is added for the collection.

The client therefore gets at most limit changes plus the latest token, and believes it is fully up to date. The remaining changes are never reported. With nresults=1 and several pending deletes, the client keeps the deleted contacts.

The CalDAV backend already handles this case.

Fix

This follows the approach in Sabre\CalDAV\Backend\PDO::getChangesForCalendar():

• Fetch limit + 1 change rows to detect truncation.
• If there are more changes than the limit, set result_truncated and return a sync-token that only covers the reported changes.
• Otherwise, the behaviour is unchanged: all changes are reported together with the address book's current token.

Different from the CalDAV backend, the code counts change rows for the limit, not merged results. In case of multiple changes to the same object, it will return fewer results than allows by the limit parameter, but will still be able to report result truncation correctly (which is required by RFC 6578).

Tests

New tests in tests/Sabre/CardDAV/Backend/AbstractPDOTestCase.php:

• testGetChangesWithLimit: three deletes paged with limit=1. Each page reports one delete and is truncated, and the final page returns the current token without truncation.
• testGetChangesWithLimitAndDuplicates: two changes to the same card merge into one result on a truncated page, followed by the remaining change.

Both tests fail without the fix. The existing testGetChanges is unchanged and passes.

Other noticed issue (not fixed)

Both CalDAV and CardDAV ignore the limit parameter when no sync-token is given (initial sync). However, this is not conform with RFC 6578. Contrary to the docblock comment above getChangesForAddressBook(), the limit is not suggestive, and the server is not free to ignore it. The server has two conforming options:

1. Honor the limit, return at most limit results, and indicate result truncation to tell the client there are more changes beyond those reported.
2. Fail reporting a DAV:number-of-matches-within-limits postcondition.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.23%. Comparing base (e755174) to head (55cda9f).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1682      +/-   ##
============================================
+ Coverage     97.22%   97.23%   +0.01%     
- Complexity     2845     2847       +2     
============================================
  Files           175      175              
  Lines          8887     8893       +6     
============================================
+ Hits           8640     8647       +7     
+ Misses          247      246       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.