From 372844ef5c787fdb5017999792e540f5ef11b50a Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Mon, 27 Jul 2026 09:40:19 +0200 Subject: [PATCH] fix(sync): apply all InstanceId removes for a recurring series Store EAS 16 occurrence deletes as uid => [InstanceId, ...] so a Sync batch that removes several instances of the same series no longer keeps only the last one. Resolve Removes that omit ServerEntryId against a single co-batched Add (iOS pattern) so the EXDATE is applied in the same request instead of requiring a second delete. --- lib/Horde/ActiveSync/Connector/Importer.php | 2 +- lib/Horde/ActiveSync/Request/Sync.php | 128 +++++++++++- .../ActiveSync/Request/SyncStreamingTest.php | 190 ++++++++++++++++++ 3 files changed, 312 insertions(+), 8 deletions(-) diff --git a/lib/Horde/ActiveSync/Connector/Importer.php b/lib/Horde/ActiveSync/Connector/Importer.php index dc2ccb6c..4414b962 100644 --- a/lib/Horde/ActiveSync/Connector/Importer.php +++ b/lib/Horde/ActiveSync/Connector/Importer.php @@ -411,7 +411,7 @@ protected function _draftModifyStat( * @param array $ids Server message uids to delete * @param string $class The server collection class. * @param boolean $instanceids If true, $ids is a hash of - * instanceids => uids. @since 2.31.0 + * uid => instanceid. @since 2.31.0 * * @return array An array containing ids of successfully deleted messages. */ diff --git a/lib/Horde/ActiveSync/Request/Sync.php b/lib/Horde/ActiveSync/Request/Sync.php index 3ebe7a6a..d989d152 100644 --- a/lib/Horde/ActiveSync/Request/Sync.php +++ b/lib/Horde/ActiveSync/Request/Sync.php @@ -1125,18 +1125,100 @@ protected function _importRemoves( * * @param Horde_ActiveSync_Connector_Importer $importer The importer. * @param array $collection The collection array. - * @param array $instanceidRemoves Hash of uid => instanceid. + * @param array $instanceidRemoves Hash of uid => instanceid, or + * uid => list of instanceids (when the + * client deletes several occurrences of + * the same series in one Sync). */ protected function _importInstanceIdRemoves( $importer, array &$collection, array $instanceidRemoves ) { - foreach ($instanceidRemoves as $uid => $instanceid) { - $importer->importMessageDeletion([$uid => $instanceid], $collection['class'], true); + foreach ($instanceidRemoves as $uid => $instanceids) { + if (!is_array($instanceids)) { + $instanceids = [$instanceids]; + } + foreach ($instanceids as $instanceid) { + $importer->importMessageDeletion( + [$uid => $instanceid], + $collection['class'], + true + ); + } + } + } + + /** + * Count instance-delete entries, including multiple InstanceIds per uid. + * + * @param array $instanceidRemoves Hash of uid => instanceid|instanceids. + * + * @return integer + */ + protected function _countInstanceIdRemoves(array $instanceidRemoves) + { + $count = 0; + foreach ($instanceidRemoves as $instanceids) { + $count += is_array($instanceids) ? count($instanceids) : 1; + } + + return $count; + } + + /** + * Resolve EAS 16 instance Removes that arrived without a ServerEntryId. + * + * iOS often pairs a Remove(InstanceId) that omits ServerEntryId with an + * Add of the same recurring series in one Commands block. After the Add + * has produced a server uid via clientids, attach the orphaned InstanceIds + * to that uid so the occurrence is deleted (EXDATE) in the same sync. + * + * @param array $collection The collection array, updated in place. + */ + protected function _resolveOrphanInstanceIdRemoves(array &$collection) + { + if (empty($collection['orphan_instanceid_removes'])) { + return; + } + $orphans = $collection['orphan_instanceid_removes']; + unset($collection['orphan_instanceid_removes']); + + $newUids = []; + if (!empty($collection['clientids'])) { + foreach ($collection['clientids'] as $serverid) { + if ($serverid) { + $newUids[] = $serverid; + } + } + } + $newUids = array_values(array_unique($newUids)); + + if (count($newUids) !== 1) { + $this->_logger->warn(sprintf( + 'Dropping %d instance Remove(s) with empty ServerEntryId; cannot resolve to a single co-batched Add (found %d).', + count($orphans), + count($newUids) + )); + return; + } + + $uid = $newUids[0]; + if (!isset($collection['instanceid_removes'][$uid]) + || !is_array($collection['instanceid_removes'][$uid])) { + $collection['instanceid_removes'][$uid] = []; + } + foreach ($orphans as $instanceid) { + $collection['instanceid_removes'][$uid][] = $instanceid; } + $this->_logger->info(sprintf( + 'Resolved %d instance Remove(s) with empty ServerEntryId to ServerEntryId %s from co-batched Add.', + count($orphans), + $uid + )); } + /** * Import client-sent Sync commands that were queued during request * parsing (streaming only). @@ -1218,12 +1300,26 @@ protected function _runDeferredSyncCommands(array &$collection) $count += count($deferred['removes']); $keepAlives += $this->_emitKeepAlive(); } + // Orphans may have been queued without a ServerEntryId; resolve them + // against a single successful co-batched Add before importing. + if (!empty($deferred['orphan_instanceid_removes'])) { + $collection['orphan_instanceid_removes'] + = $deferred['orphan_instanceid_removes']; + } if (!empty($deferred['instanceid_removes'])) { + $collection['instanceid_removes'] = $deferred['instanceid_removes']; + } + $this->_resolveOrphanInstanceIdRemoves($collection); + + if (!empty($collection['instanceid_removes'])) { + $instanceCount = $this->_countInstanceIdRemoves( + $collection['instanceid_removes'] + ); try { $this->_importInstanceIdRemoves( $importer, $collection, - $deferred['instanceid_removes'] + $collection['instanceid_removes'] ); } catch (Horde_Exception $e) { $this->_logger->err(sprintf( @@ -1232,7 +1328,8 @@ protected function _runDeferredSyncCommands(array &$collection) $e->getMessage() )); } - $count += count($deferred['instanceid_removes']); + unset($collection['instanceid_removes']); + $count += $instanceCount; $keepAlives += $this->_emitKeepAlive(); } @@ -1715,7 +1812,17 @@ protected function _parseSyncCommands(&$collection) case Horde_ActiveSync::SYNC_REMOVE: if ($instanceid) { - $collection['instanceid_removes'][$serverid] = $instanceid; + if ($serverid) { + // Several occurrence deletes for one series + // share the same ServerEntryId; keep a list. + $collection['instanceid_removes'][$serverid][] + = $instanceid; + } else { + // iOS may omit ServerEntryId when pairing the + // Remove with an Add of the same series. + $collection['orphan_instanceid_removes'][] + = $instanceid; + } } elseif ($serverid) { // Work around broken clients that send empty $serverid. $collection['removes'][] = $serverid; @@ -1750,6 +1857,11 @@ protected function _parseSyncCommands(&$collection) = $collection['instanceid_removes']; unset($collection['instanceid_removes']); } + if (!empty($collection['orphan_instanceid_removes'])) { + $this->_deferredCommands[$collection['id']]['orphan_instanceid_removes'] + = $collection['orphan_instanceid_removes']; + unset($collection['orphan_instanceid_removes']); + } $this->_logger->info(sprintf( 'Queued %d incoming changes for deferred import (streaming).', $nchanges @@ -1766,7 +1878,9 @@ protected function _parseSyncCommands(&$collection) ); unset($collection['removes']); } - // EAS 16.0 instance deletions. + // Resolve InstanceId Removes that omitted ServerEntryId against + // a co-batched Add, then import all EAS 16.0 instance deletions. + $this->_resolveOrphanInstanceIdRemoves($collection); if (!empty($collection['instanceid_removes']) && !empty($collection['synckey'])) { $this->_importInstanceIdRemoves( diff --git a/test/unit/Horde/ActiveSync/Request/SyncStreamingTest.php b/test/unit/Horde/ActiveSync/Request/SyncStreamingTest.php index 7effdb76..b0de77da 100644 --- a/test/unit/Horde/ActiveSync/Request/SyncStreamingTest.php +++ b/test/unit/Horde/ActiveSync/Request/SyncStreamingTest.php @@ -403,6 +403,196 @@ public function testRunDeferredSyncCommandsNoopWithoutQueue() $this->assertSame(['id' => 'F1'], $collection); } + public function testImportInstanceIdRemovesAcceptsMultiplePerUid() + { + $sync = $this->_syncRequestWithoutConstructor(); + + $importer = $this->createMock(Horde_ActiveSync_Connector_Importer::class); + $importer->expects($this->exactly(3)) + ->method('importMessageDeletion') + ->willReturnCallback(function ($ids, $class, $instanceids) { + $this->assertTrue($instanceids); + $this->assertSame(Horde_ActiveSync::CLASS_CALENDAR, $class); + $this->assertCount(1, $ids); + return $ids; + }); + + $collection = [ + 'id' => 'A1', + 'class' => Horde_ActiveSync::CLASS_CALENDAR, + ]; + + $this->_invokeSyncMethod( + $sync, + '_importInstanceIdRemoves', + [ + $importer, + &$collection, + [ + 'series-uid' => [ + '20250801T120000Z', + '20250808T120000Z', + ], + // Legacy single-string form still supported. + 'other-uid' => '20250901T120000Z', + ], + ] + ); + } + + public function testResolveOrphanInstanceIdRemovesMapsToSingleAdd() + { + $sync = $this->_syncRequestWithoutConstructor(); + $ref = new ReflectionClass($sync); + $loggerProp = $ref->getProperty('_logger'); + $loggerProp->setAccessible(true); + $loggerProp->setValue( + $sync, + new Horde_ActiveSync_Log_Logger(new Horde_Log_Handler_Null()) + ); + + $collection = [ + 'orphan_instanceid_removes' => [ + '20260803T110000Z', + '20260810T110000Z', + ], + 'clientids' => [ + 'client-1' => 'server-uid-42', + ], + 'instanceid_removes' => [ + 'server-uid-42' => ['20260817T110000Z'], + ], + ]; + + $this->_invokeSyncMethod( + $sync, + '_resolveOrphanInstanceIdRemoves', + [&$collection] + ); + + $this->assertArrayNotHasKey('orphan_instanceid_removes', $collection); + $this->assertSame( + [ + 'server-uid-42' => [ + '20260817T110000Z', + '20260803T110000Z', + '20260810T110000Z', + ], + ], + $collection['instanceid_removes'] + ); + } + + public function testResolveOrphanInstanceIdRemovesDropsWhenAmbiguous() + { + $sync = $this->_syncRequestWithoutConstructor(); + $ref = new ReflectionClass($sync); + $loggerProp = $ref->getProperty('_logger'); + $loggerProp->setAccessible(true); + $loggerProp->setValue( + $sync, + new Horde_ActiveSync_Log_Logger(new Horde_Log_Handler_Null()) + ); + + $collection = [ + 'orphan_instanceid_removes' => ['20260803T110000Z'], + 'clientids' => [ + 'client-1' => 'server-a', + 'client-2' => 'server-b', + ], + ]; + + $this->_invokeSyncMethod( + $sync, + '_resolveOrphanInstanceIdRemoves', + [&$collection] + ); + + $this->assertArrayNotHasKey('orphan_instanceid_removes', $collection); + $this->assertArrayNotHasKey('instanceid_removes', $collection); + } + + public function testRunDeferredSyncCommandsImportsAllInstanceRemoves() + { + $sync = $this->_syncRequestWithoutConstructor(); + $ref = new ReflectionClass($sync); + + $appdata = $this->createMock(Horde_ActiveSync_Message_Base::class); + + $importer = $this->createMock(Horde_ActiveSync_Connector_Importer::class); + $importer->expects($this->once())->method('init'); + $importer->expects($this->once()) + ->method('importMessageChange') + ->willReturn(['id' => 'series-uid', 'mod' => 1]); + $importer->expects($this->exactly(3)) + ->method('importMessageDeletion') + ->willReturnCallback(function ($ids, $class, $instanceids) { + $this->assertTrue($instanceids); + $this->assertSame(['series-uid'], array_keys($ids)); + return $ids; + }); + + $as = $this->createMock(Horde_ActiveSync::class); + $as->method('getImporter')->willReturn($importer); + + $main = fopen('php://memory', 'wb+'); + $encoder = $this->_encoder($main); + + foreach ([ + '_activeSync' => $as, + '_device' => $this->createMock(\Horde_ActiveSync_Device::class), + '_state' => $this->createMock(Horde_ActiveSync_State_Base::class), + '_encoder' => $encoder, + '_logger' => new Horde_ActiveSync_Log_Logger(new Horde_Log_Handler_Null()), + '_keepAliveInterval' => 15, + '_deferredCommands' => [ + 'A1' => [ + 'commands' => [ + [ + 'type' => Horde_ActiveSync::SYNC_ADD, + 'serverid' => false, + 'clientid' => 'client-cal', + 'appdata' => $appdata, + ], + ], + // Multiple deletes for the known series uid. + 'instanceid_removes' => [ + 'series-uid' => [ + '20250801T120000Z', + '20250808T120000Z', + ], + ], + // Orphan Remove from iOS (empty ServerEntryId), resolved + // against the co-batched Add above. + 'orphan_instanceid_removes' => [ + '20250815T120000Z', + ], + ], + ], + ] as $property => $value) { + $prop = $ref->getProperty($property); + $prop->setAccessible(true); + $prop->setValue($sync, $value); + } + + $collection = [ + 'id' => 'A1', + 'class' => Horde_ActiveSync::CLASS_CALENDAR, + 'synckey' => '{uuid}5', + 'conflict' => Horde_ActiveSync::CONFLICT_OVERWRITE_PIM, + 'clientids' => [], + ]; + + $method = $ref->getMethod('_runDeferredSyncCommands'); + $method->setAccessible(true); + $collectionArgs = [&$collection]; + $method->invokeArgs($sync, $collectionArgs); + + $this->assertSame(['client-cal' => 'series-uid'], $collection['clientids']); + $this->assertArrayNotHasKey('instanceid_removes', $collection); + $this->assertArrayNotHasKey('orphan_instanceid_removes', $collection); + } + protected function _encoder($stream) { $encoder = new Horde_ActiveSync_Wbxml_Encoder(