From f58fe75e4bd1539e9dd4893f4d33b2fef56f9f73 Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Mon, 26 Oct 2026 23:48:46 +0100 Subject: [PATCH] feat(activesync): let backends set the per-item status of a failed change Connector_Importer::importMessageChange() reported every changeMessage() failure as Status 5 (server error), so a client resends an item the server will never accept on every Sync. Allow Driver_Base::changeMessage() to return ['error' => ] in addition to false; the importer propagates the status into the importfailures list and still defaults to Status 5 for false. The core driver uses this to answer Status 6 (STATUS_INVALID) when the backend rejected the client's data (see horde/turba#73). --- doc/architecture.md | 6 + lib/Horde/ActiveSync/Connector/Importer.php | 21 ++- lib/Horde/ActiveSync/Driver/Base.php | 9 +- .../Connector/ImporterChangeStatusTest.php | 137 ++++++++++++++++++ 4 files changed, 165 insertions(+), 8 deletions(-) create mode 100644 test/unit/Horde/ActiveSync/Connector/ImporterChangeStatusTest.php diff --git a/doc/architecture.md b/doc/architecture.md index 45287ebc..8f8395a4 100644 --- a/doc/architecture.md +++ b/doc/architecture.md @@ -163,6 +163,12 @@ EAS code pages defined in `Wbxml.php`. Points worth knowing: detection against the change map, `changeMessage()` / `deleteMessage()` / `moveMessage()` calls, folder operations, and recording of client-origin changes so they are not exported back. + A failed `changeMessage()` returns either `false` — a server error, + reported as `Status 5` so the client may retry — or + `['error' => ]` naming the per-item status; backends use + `Status 6` (`STATUS_INVALID`) when they *rejected the client's data* + (e.g. a Turba attribute over its configured length), so the client stops + resending an item the server will never accept. - `Connector_Exporter_Sync` walks the change list produced by `getServerChanges()` and encodes each change into the response, updating state as it goes. diff --git a/lib/Horde/ActiveSync/Connector/Importer.php b/lib/Horde/ActiveSync/Connector/Importer.php index 48e21a14..d188b16c 100644 --- a/lib/Horde/ActiveSync/Connector/Importer.php +++ b/lib/Horde/ActiveSync/Connector/Importer.php @@ -293,7 +293,8 @@ public function importMessageChange( $uid, $message, $device - ))) { + )) + && empty($stat['error'])) { $stat['serverid'] = $this->_folderId; $this->_state->updateState( Horde_ActiveSync::CHANGE_TYPE_CHANGE, @@ -322,16 +323,22 @@ public function importMessageChange( } // Tell the backend about the change - if (!$stat = $this->_as->driver->changeMessage($this->_folderId, $backendId, $message, $device)) { + $stat = $this->_as->driver->changeMessage($this->_folderId, $backendId, $message, $device); + if (!$stat || !empty($stat['error'])) { + // The backend may name the per-item status to report (e.g. + // STATUS_INVALID for rejected client data, which the client + // must not retry); a plain false is a retryable server error. + $status = !empty($stat['error']) + ? $stat['error'] + : Horde_ActiveSync_Request_Sync::STATUS_SERVERERROR; $this->_logger->err( sprintf( - 'Change message failed when updating %s', - $id + 'Change message failed when updating %s (status %d)', + $id, + $status ) ); - return $id - ? [0 => $id, 'error' => Horde_ActiveSync_Request_Sync::STATUS_SERVERERROR] - : [0 => false, 'error' => Horde_ActiveSync_Request_Sync::STATUS_SERVERERROR]; + return [0 => $id ?: false, 'error' => $status]; } $stat['serverid'] = $this->_folderId; diff --git a/lib/Horde/ActiveSync/Driver/Base.php b/lib/Horde/ActiveSync/Driver/Base.php index 003ed401..c24ec26b 100644 --- a/lib/Horde/ActiveSync/Driver/Base.php +++ b/lib/Horde/ActiveSync/Driver/Base.php @@ -762,7 +762,14 @@ abstract public function getWasteBasket($class); * The activesync message * @param Horde_ActiveSync_Device $device The device information * - * @return array|boolean A stat array if successful, otherwise false. + * @return array|boolean A stat array if successful. On failure either + * false (server error, the client may retry + * the item) or an array with only an 'error' + * key holding the + * Horde_ActiveSync_Request_Sync::STATUS_* code + * to report for the item - STATUS_INVALID when + * the backend rejected the client's data, so + * the client stops resending it. */ abstract public function changeMessage($folderid, $id, Horde_ActiveSync_Message_Base $message, $device); diff --git a/test/unit/Horde/ActiveSync/Connector/ImporterChangeStatusTest.php b/test/unit/Horde/ActiveSync/Connector/ImporterChangeStatusTest.php new file mode 100644 index 00000000..a836a4f7 --- /dev/null +++ b/test/unit/Horde/ActiveSync/Connector/ImporterChangeStatusTest.php @@ -0,0 +1,137 @@ + + * @category Horde + * @copyright 2026 The Horde Project + * @license http://www.horde.org/licenses/gpl GPLv2 + * @package ActiveSync + * @subpackage UnitTests + */ + +use PHPUnit\Framework\TestCase; + +/** + * Unit tests for the per-item status a backend may attach to a failed + * changeMessage(): rejected client data must surface as STATUS_INVALID so + * the client stops resending the item, while a plain false stays a + * retryable STATUS_SERVERERROR. + */ +class Horde_ActiveSync_Connector_ImporterChangeStatusTest extends TestCase +{ + public function testBackendErrorStatusIsReportedForModify(): void + { + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('changeMessage') + ->willReturn(['error' => Horde_ActiveSync_Request_Sync::STATUS_INVALID]); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('isConflict')->willReturn(false); + $state->expects($this->never())->method('updateState'); + + $result = $this->_importer($state, $driver)->importMessageChange( + 'contact-1', + new Horde_ActiveSync_Message_Contact(), + $this->_device(), + false, + Horde_ActiveSync::CLASS_CONTACTS + ); + + $this->assertSame('contact-1', $result[0]); + $this->assertSame(Horde_ActiveSync_Request_Sync::STATUS_INVALID, $result['error']); + } + + public function testBackendErrorStatusIsReportedForAdd(): void + { + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('changeMessage') + ->willReturn(['error' => Horde_ActiveSync_Request_Sync::STATUS_INVALID]); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('isDuplicatePIMAddition')->willReturn(false); + $state->expects($this->never())->method('updateState'); + + $result = $this->_importer($state, $driver)->importMessageChange( + false, + new Horde_ActiveSync_Message_Contact(), + $this->_device(), + 'client-1', + Horde_ActiveSync::CLASS_CONTACTS + ); + + $this->assertFalse($result[0]); + $this->assertSame(Horde_ActiveSync_Request_Sync::STATUS_INVALID, $result['error']); + } + + public function testPlainFalseStaysServerError(): void + { + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('changeMessage')->willReturn(false); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('isConflict')->willReturn(false); + + $result = $this->_importer($state, $driver)->importMessageChange( + 'contact-1', + new Horde_ActiveSync_Message_Contact(), + $this->_device(), + false, + Horde_ActiveSync::CLASS_CONTACTS + ); + + $this->assertSame('contact-1', $result[0]); + $this->assertSame(Horde_ActiveSync_Request_Sync::STATUS_SERVERERROR, $result['error']); + } + + public function testSuccessfulChangeIsUnaffected(): void + { + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('changeMessage') + ->willReturn(['id' => 'contact-1', 'mod' => 5, 'flags' => 1]); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('isConflict')->willReturn(false); + $state->expects($this->once())->method('updateState'); + + $result = $this->_importer($state, $driver)->importMessageChange( + 'contact-1', + new Horde_ActiveSync_Message_Contact(), + $this->_device(), + false, + Horde_ActiveSync::CLASS_CONTACTS + ); + + $this->assertSame('contact-1', $result['id']); + $this->assertArrayNotHasKey('error', $result); + } + + protected function _device(): Horde_ActiveSync_Device + { + $device = $this->createMock(Horde_ActiveSync_Device::class); + $device->supported = []; + return $device; + } + + protected function _importer($state, $driver): Horde_ActiveSync_Connector_Importer + { + $collections = $this->createMock(Horde_ActiveSync_Collections::class); + $collections->method('getBackendIdForFolderUid')->willReturn('@Contacts@'); + + $as = $this->createMock(Horde_ActiveSync::class); + $as->driver = $driver; + $as->method('getCollectionsObject')->willReturn($collections); + + $importer = new Horde_ActiveSync_Connector_Importer($as); + $importer->setLogger(new Horde_Log_Logger(new Horde_Log_Handler_Null())); + $importer->init($state, 'folder-uid', 0); + + return $importer; + } +}