diff --git a/src/Entity/Mail/Message.php b/src/Entity/Mail/Message.php index d0f3c542..55a29df3 100644 --- a/src/Entity/Mail/Message.php +++ b/src/Entity/Mail/Message.php @@ -19,7 +19,7 @@ use Doctrine\ORM\Mapping as ORM; // batch handlers dedup in PHP before inserting, but that check is a read on // stale data — this is the guard that actually holds when batches overlap // across runs or retries. Provider id leads the column list so the indexes -// also serve the id-only lookups (findOneBy(['gmailId'|'graphId'])). +// also serve the provider-id lookups (graph by account, gmail by owning user). #[ORM\UniqueConstraint(name: 'uniq_message_gmail_id_account', columns: ['gmail_id', 'account_id'])] #[ORM\UniqueConstraint(name: 'uniq_message_graph_id_account', columns: ['graph_id', 'account_id'])] // IMAP UIDs are unique within a mailbox, so this is the guard for the sync diff --git a/src/Infrastructure/Messaging/Handler/SyncGmailMessageBatchHandler.php b/src/Infrastructure/Messaging/Handler/SyncGmailMessageBatchHandler.php index 003e3578..852d6940 100644 --- a/src/Infrastructure/Messaging/Handler/SyncGmailMessageBatchHandler.php +++ b/src/Infrastructure/Messaging/Handler/SyncGmailMessageBatchHandler.php @@ -183,7 +183,7 @@ final readonly class SyncGmailMessageBatchHandler // redelivered batch — a worker restart, a retry after a partial // failure — used to build a second row for every message in it. if ('' !== $gmailId) { - $known = $this->messageRepository->findOneBy(['gmailId' => $gmailId]); + $known = $this->messageRepository->findOneByGmailIdForUser($account->usr, $gmailId); if (null !== $known) { // A native Gmail row: Gmail is the authority on its flags, diff --git a/src/Repository/Mail/MessageRepository.php b/src/Repository/Mail/MessageRepository.php index a9133088..0f3b74ac 100644 --- a/src/Repository/Mail/MessageRepository.php +++ b/src/Repository/Mail/MessageRepository.php @@ -1537,6 +1537,30 @@ class MessageRepository extends ServiceEntityRepository ->getResult(); } + /** + * The one row for a Gmail id among the owner's accounts, for the batch + * handler's "already stored?" check. + * + * Scoped through the user and not the account, for the reason given on + * findByGmailIdsForUser(): a message fetched by one Gmail account may be + * attributed to a sibling. It used to be a bare findOneBy(['gmailId']), + * which searched every user on the install — a gmailId is only unique per + * account, so another user's row could be found and enriched with this + * user's labels. QueryBuilder because the owner is behind an association. + */ + public function findOneByGmailIdForUser(User $user, string $gmailId): ?Message + { + return $this->createQueryBuilder('m') + ->innerJoin('m.account', 'a') + ->where('a.usr = :usr') + ->andWhere('m.gmailId = :gmailId') + ->setParameter('usr', $user) + ->setParameter('gmailId', $gmailId) + ->setMaxResults(1) + ->getQuery() + ->getOneOrNullResult(); + } + /** * QueryBuilder on two counts: the owner is reached through the account * association, which findBy() cannot traverse, and only one column comes diff --git a/src/Service/Graph/GraphApiSyncer.php b/src/Service/Graph/GraphApiSyncer.php index cddf53b6..cbd1c88e 100644 --- a/src/Service/Graph/GraphApiSyncer.php +++ b/src/Service/Graph/GraphApiSyncer.php @@ -217,7 +217,7 @@ final class GraphApiSyncer // Cast for the same reason, and here the symptom would be silent: // an int compared against a stored string id matches nothing, so // the message is simply not erased and nothing is raised. - $message = $this->messageRepository->findOneBy(['graphId' => (string) $graphId]); + $message = $this->messageRepository->findOneBy(['graphId' => (string) $graphId, 'account' => $account]); if (null === $message) { continue; @@ -294,7 +294,7 @@ final class GraphApiSyncer // phone stayed unread here, exactly as it did on IMAP, and for the // same reason — nothing ever re-read the state of a row it already // had. - $state = $this->flagStateOf($graphId, $item); + $state = $this->flagStateOf($account, $graphId, $item); if (null !== $state) { $flagStates[] = $state; @@ -323,7 +323,7 @@ final class GraphApiSyncer * * @param array $item */ - private function flagStateOf(string $graphId, array $item): ?RemoteFlagState + private function flagStateOf(Account $account, string $graphId, array $item): ?RemoteFlagState { if (false === array_key_exists('isRead', $item)) { // A delta entry that does not mention read state is not asserting @@ -332,7 +332,7 @@ final class GraphApiSyncer return null; } - $message = $this->messageRepository->findOneBy(['graphId' => $graphId]); + $message = $this->messageRepository->findOneBy(['graphId' => $graphId, 'account' => $account]); if (null === $message) { return null; @@ -355,7 +355,7 @@ final class GraphApiSyncer private function attachFolderLabel(Account $account, string $graphId, string $folderId): void { - $message = $this->messageRepository->findOneBy(['graphId' => $graphId]); + $message = $this->messageRepository->findOneBy(['graphId' => $graphId, 'account' => $account]); if (null === $message) { return; @@ -404,7 +404,7 @@ final class GraphApiSyncer private function detachFolderLabel(Account $account, string $graphId, string $folderId): void { - $message = $this->messageRepository->findOneBy(['graphId' => $graphId]); + $message = $this->messageRepository->findOneBy(['graphId' => $graphId, 'account' => $account]); if (null === $message) { return; diff --git a/tests/Service/Gmail/GmailInboundReadStateTest.php b/tests/Service/Gmail/GmailInboundReadStateTest.php index f16196bb..35052eba 100644 --- a/tests/Service/Gmail/GmailInboundReadStateTest.php +++ b/tests/Service/Gmail/GmailInboundReadStateTest.php @@ -170,6 +170,31 @@ final class GmailInboundReadStateTest extends KernelTestCase self::assertNotNull($message->seenAt, 'the local change is still in flight'); } + /** + * A gmailId is only unique per account. The "already stored?" lookup was + * a bare findOneBy(['gmailId']) across the whole install, so another + * user's row with the same id was found and rewritten with this user's + * labels, and this user's message was never stored at all. + */ + public function testAnotherUsersRowWithTheSameGmailIdIsLeftAlone(): void + { + $mine = $this->account; + $this->user = $this->seedUser(); + $this->account = $this->seedAccount(); + $theirs = $this->seedMessage(seen: false); + $this->account = $mine; + + $this->handle(['INBOX']); + + $this->em->refresh($theirs); + + self::assertNull($theirs->seenAt, 'the other user\'s row must not be touched'); + self::assertNotNull( + $this->em->getRepository(Message::class)->findOneBy(['gmailId' => self::GMAIL_ID, 'account' => $mine]), + 'the message is stored for the account that fetched it', + ); + } + // ── helpers ─────────────────────────────────────────────────────────── /**