Skip to content

Commit d33fc21

Browse files
kesselbkesselb
committed
fix(sync): apply the search filter as a restriction
SyncService filtered its candidates with a parameter that widens the search instead of narrowing it. With a subject term in the query the filter stopped restricting, so a search returned every thread newer than the oldest match. Add a separate parameter that always narrows. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
1 parent 5dcf82b commit d33fc21

3 files changed

Lines changed: 101 additions & 21 deletions

File tree

‎lib/Db/MessageMapper.php‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,14 @@ class MessageMapper extends QBMapper {
5050

5151
use TTransactional;
5252

53+
private const PARAM_UIDS = 'uids';
54+
private const PARAM_IDS = 'ids';
55+
56+
/**
57+
* TODO: replace with IQueryBuilder::MAX_IN_PARAMETERS once the minimum server version is 35
58+
*/
59+
private const MAX_IN_PARAMETERS = 1000;
60+
5361
/** @var ITimeFactory */
5462
private $timeFactory;
5563

@@ -791,14 +799,12 @@ public function findByMessageId(Account $account, string $messageId): array {
791799
}
792800

793801
/**
794-
* @param Mailbox $mailbox
795-
* @param SearchQuery $query
796-
* @param int|null $limit
797-
* @param int[]|null $uids
802+
* @param int[]|null $uids IMAP body search matches, combined with the text conditions via OR
803+
* @param int[]|null $ids restricts the result to these message ids
798804
*
799805
* @return int[]
800806
*/
801-
public function findIdsByQuery(Mailbox $mailbox, SearchQuery $query, string $sortOrder, ?int $limit, ?array $uids = null): array {
807+
public function findIdsByQuery(Mailbox $mailbox, SearchQuery $query, string $sortOrder, ?int $limit, ?array $uids = null, ?array $ids = null): array {
802808
$qb = $this->db->getQueryBuilder();
803809

804810
if ($this->needDistinct($query)) {
@@ -925,17 +931,23 @@ public function findIdsByQuery(Mailbox $mailbox, SearchQuery $query, string $sor
925931
// In the case of body+subject search we need a combination of both results,
926932
// thus the orWhere in every other case andWhere should do the job.
927933
if (!empty($query->getSubjects())) {
928-
$textOrs[] = $qb->expr()->in('m.uid', $qb->createParameter('uids'));
934+
$textOrs[] = $qb->expr()->in('m.uid', $qb->createParameter(self::PARAM_UIDS));
929935
} else {
930936
$select->andWhere(
931-
$qb->expr()->in('m.uid', $qb->createParameter('uids'))
937+
$qb->expr()->in('m.uid', $qb->createParameter(self::PARAM_UIDS))
932938
);
933939
}
934940
}
935941
if (!empty($textOrs)) {
936942
$select->andWhere($qb->expr()->orX(...$textOrs));
937943
}
938944

945+
if ($ids !== null) {
946+
$select->andWhere(
947+
$qb->expr()->in('m.id', $qb->createParameter(self::PARAM_IDS), IQueryBuilder::PARAM_INT_ARRAY)
948+
);
949+
}
950+
939951
if (!empty($query->getStart())) {
940952
$select->andWhere(
941953
$qb->expr()->gte('m.sent_at', $qb->createNamedParameter($query->getStart()), IQueryBuilder::PARAM_INT)
@@ -994,16 +1006,31 @@ public function findIdsByQuery(Mailbox $mailbox, SearchQuery $query, string $sor
9941006
}
9951007

9961008
if ($uids !== null) {
997-
return array_flat_map(function (array $chunk) use ($qb, $select) {
998-
$qb->setParameter('uids', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
999-
return array_map(static fn (Message $message) => $message->getId(), $this->findEntities($select));
1000-
}, array_chunk($uids, 1000));
1009+
return $this->findIdsByChunkedParameter($select, self::PARAM_UIDS, $uids);
1010+
}
1011+
1012+
if ($ids !== null) {
1013+
return $this->findIdsByChunkedParameter($select, self::PARAM_IDS, $ids);
10011014
}
10021015

10031016
$result = array_map(static fn (Message $message) => $message->getId(), $this->findEntities($select));
10041017
return $result;
10051018
}
10061019

1020+
/**
1021+
* Run $select once per chunk of $values, binding each chunk to $parameter.
1022+
*
1023+
* @param int[] $values
1024+
*
1025+
* @return int[]
1026+
*/
1027+
private function findIdsByChunkedParameter(IQueryBuilder $select, string $parameter, array $values): array {
1028+
return array_flat_map(function (array $chunk) use ($select, $parameter) {
1029+
$select->setParameter($parameter, $chunk, IQueryBuilder::PARAM_INT_ARRAY);
1030+
return array_map(static fn (Message $message) => $message->getId(), $this->findEntities($select));
1031+
}, array_chunk($values, self::MAX_IN_PARAMETERS));
1032+
}
1033+
10071034
public function findIdsGloballyByQuery(IUser $user, SearchQuery $query, ?int $limit, ?array $uids = null): array {
10081035
$qb = $this->db->getQueryBuilder();
10091036
$qbMailboxes = $this->db->getQueryBuilder();
@@ -1112,7 +1139,7 @@ public function findIdsGloballyByQuery(IUser $user, SearchQuery $query, ?int $li
11121139
}
11131140
if ($uids !== null) {
11141141
$select->andWhere(
1115-
$qb->expr()->in('m.uid', $qb->createParameter('uids'))
1142+
$qb->expr()->in('m.uid', $qb->createParameter(self::PARAM_UIDS))
11161143
);
11171144
}
11181145
foreach ($query->getFlags() as $flag) {
@@ -1133,10 +1160,7 @@ public function findIdsGloballyByQuery(IUser $user, SearchQuery $query, ?int $li
11331160
}
11341161

11351162
if ($uids !== null) {
1136-
return array_flat_map(function (array $chunk) use ($select) {
1137-
$select->setParameter('uids', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
1138-
return array_map(static fn (Message $message) => $message->getId(), $this->findEntities($select));
1139-
}, array_chunk($uids, 1000));
1163+
return $this->findIdsByChunkedParameter($select, self::PARAM_UIDS, $uids);
11401164
}
11411165

11421166
return array_map(static fn (Message $message) => $message->getId(), $this->findEntities($select));

‎lib/Service/Sync/SyncService.php‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -139,16 +139,13 @@ private function getDatabaseSyncChanges(Account $account,
139139
}
140140
$order = $sortOrder === 'oldest' ? IMailSearch::ORDER_OLDEST_FIRST : IMailSearch::ORDER_NEWEST_FIRST;
141141
if ($query !== null) {
142-
// Filter new messages to those that also match the current filter
143-
$newUids = $this->messageMapper->findUidsForIds($mailbox, $newIds);
144-
$newIds = $this->messageMapper->findIdsByQuery($mailbox, $query, $order, null, $newUids);
142+
$newIds = $this->messageMapper->findIdsByQuery($mailbox, $query, $order, null, null, $newIds);
145143
}
146144
$new = $this->messageMapper->findByMailboxAndIds($mailbox, $account->getUserId(), $newIds);
147145

148146
// TODO: $changed = $this->messageMapper->findChanged($account, $mailbox, $uids);
149147
if ($query !== null) {
150-
$changedUids = $this->messageMapper->findUidsForIds($mailbox, $knownIds);
151-
$changedIds = $this->messageMapper->findIdsByQuery($mailbox, $query, $order, null, $changedUids);
148+
$changedIds = $this->messageMapper->findIdsByQuery($mailbox, $query, $order, null, null, $knownIds);
152149
} else {
153150
$changedIds = $knownIds;
154151
}

‎tests/Integration/Db/MessageMapperTest.php‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,65 @@ public function testFindIdsByQuery(): void {
208208
self::assertEquals([3,2,1], $result);
209209
}
210210

211+
public function testFindIdsByQueryRestrictedToIds(): void {
212+
$mailbox = new Mailbox();
213+
$mailbox->setId(1);
214+
$qb = $this->db->getQueryBuilder();
215+
216+
$values = [
217+
[
218+
'id' => 1,
219+
'uid' => $qb->createNamedParameter(267, IQueryBuilder::PARAM_INT),
220+
'message_id' => $qb->createNamedParameter('<abc@123.com>'),
221+
'mailbox_id' => $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT),
222+
'subject' => $qb->createNamedParameter('needle'),
223+
'sent_at' => $qb->createNamedParameter(1641216000, IQueryBuilder::PARAM_INT),
224+
],
225+
[
226+
'id' => 2,
227+
'uid' => $qb->createNamedParameter(268, IQueryBuilder::PARAM_INT),
228+
'message_id' => $qb->createNamedParameter('<def@456.com>'),
229+
'mailbox_id' => $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT),
230+
'subject' => $qb->createNamedParameter('needle'),
231+
'sent_at' => $qb->createNamedParameter(1641216001, IQueryBuilder::PARAM_INT),
232+
],
233+
[
234+
'id' => 3,
235+
'uid' => $qb->createNamedParameter(269, IQueryBuilder::PARAM_INT),
236+
'message_id' => $qb->createNamedParameter('<ghi@789.com>'),
237+
'mailbox_id' => $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT),
238+
'subject' => $qb->createNamedParameter('unrelated'),
239+
'sent_at' => $qb->createNamedParameter(1641216003, IQueryBuilder::PARAM_INT),
240+
],
241+
];
242+
243+
foreach ($values as $value) {
244+
$qb->insert($this->mapper->getTableName())->values($value)->executeStatement();
245+
}
246+
247+
$searchQuery = new SearchQuery();
248+
$searchQuery->setMatch('anyof');
249+
$searchQuery->addSubject('needle');
250+
251+
$result = $this->mapper->findIdsByQuery($mailbox, $searchQuery, 'DESC', null, null, [2, 3]);
252+
253+
self::assertEquals([2], $result);
254+
}
255+
256+
public function testFindIdsByQueryRestrictedToNoIds(): void {
257+
$mailbox = new Mailbox();
258+
$mailbox->setId(1);
259+
$this->insertMessage(267, 1);
260+
261+
$searchQuery = new SearchQuery();
262+
$searchQuery->setMatch('anyof');
263+
$searchQuery->addSubject('TEST');
264+
265+
$result = $this->mapper->findIdsByQuery($mailbox, $searchQuery, 'DESC', null, null, []);
266+
267+
self::assertEquals([], $result);
268+
}
269+
211270
public function testDeleteByUid(): void {
212271
$mailbox = new Mailbox();
213272
$mailbox->setId(1);

0 commit comments

Comments
 (0)