Repository navigation
feat: jmap support - part 2 - #12800
Conversation
f3ed4a9 to
c56f384
Compare
3b35d0b to
ee47083
Compare
|
Hey, 12800 nice pr number ;) Something I've overlooked in the first jmap pr: Several methods in IMessageConnector take the message list as a variadic ( Both implementations treat At the call sites we already hold an array in most cases and just spread it ( |
|
Hey
Sure I can change this... back to arrays (I had arrays initially)... buy I decided on using variadic notation because it gives actual type safety not just while using psalm.
This was a dumb over site on my part... from the original array iteration. Any chance we can make this change after this is merged? As it make is messy right now with multiple PR's based on the already merged code to change and alter all the tests again. I can make this change while I wait for the front end changes to be approved |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a protocol abstraction layer so the Mail app can operate against IMAP and JMAP via shared connector interfaces, while updating existing services/controllers/jobs to route operations through ProtocolFactory and connectors.
Changes:
- Introduces
ProtocolFactoryrouting and connector interfaces for mailbox/message operations (IMAP + JMAP). - Refactors multiple services/jobs/controllers to use
ProtocolFactory/MailManagerAPIs instead of direct IMAP client usage. - Adds new unit/integration tests and adjusts CI/integration configuration for JMAP (Stalwart) testing.
Reviewed changes
Copilot reviewed 109 out of 109 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/SetupChecks/MailConnectionPerformanceTest.php | Updates setup-check unit tests to use ProtocolFactory instead of IMAPClientFactory. |
| tests/Unit/Service/Sync/SyncServiceTest.php | Refactors sync unit test expectations to use mailbox/message connectors. |
| tests/Unit/Service/Sync/ImapToDbSynchronizerTest.php | Switches synchronizer tests to ProtocolFactory and MailManager. |
| tests/Unit/Service/SetupServiceTest.php | Updates setup service tests to use ProtocolFactory->imapClient(). |
| tests/Unit/Service/Search/MailSearchTest.php | Adjusts search tests to use MailManager->findMessages() rather than IMAP search provider. |
| tests/Unit/Service/OutboxServiceTest.php | Refactors outbox tests for ProtocolFactory + MailManager. |
| tests/Unit/Service/MailTransmissionTest.php | Refactors transmission tests to use ProtocolFactory + MailManager. |
| tests/Unit/Service/ItineraryServiceTest.php | Updates itinerary tests to new ItineraryService::extract() signature using Message. |
| tests/Unit/Service/IMipServiceTest.php | Updates iMIP service tests for new MailManager->getImapMessages() signature. |
| tests/Unit/Service/DraftsServiceTest.php | Refactors drafts tests to use ProtocolFactory + MailManager. |
| tests/Unit/Service/DkimServiceTest.php | Updates DKIM tests to use ProtocolFactory + IMessageConnector and Message param. |
| tests/Unit/Service/Attachment/AttachmentServiceTest.php | Updates attachment service tests to remove explicit IMAP client parameter. |
| tests/Unit/Service/AntiSpamServiceTest.php | Refactors anti-spam tests to use ProtocolFactory->imapClient(). |
| tests/Unit/Service/AiIntegrationsServiceTest.php | Removes IMAP client factory usage from AI integration unit tests. |
| tests/Unit/Service/AccountServiceTest.php | Refactors account connection tests to use ProtocolFactory->testConnection(). |
| tests/Unit/Send/ChainTest.php | Updates send chain tests to use ProtocolFactory->imapClient(). |
| tests/Unit/Listener/MoveJunkListenerTest.php | Adapts listener tests to new MoveJunkListener deps (mailbox mapper + message lookup). |
| tests/Unit/Listener/DeleteDraftListenerTest.php | Updates delete-draft listener test to use ProtocolFactory. |
| tests/Unit/Job/TrashRetentionJobTest.php | Refactors retention job tests to delete via MailManager->deleteMessage(). |
| tests/Unit/Job/FollowUpClassifierJobTest.php | Updates follow-up job test to use MailManager batch APIs. |
| tests/Unit/JMAP/JmapMessageAdapterTest.php | Adds new unit tests covering JMAP message-to-DB mapping behavior. |
| tests/Unit/JMAP/JmapMailboxAdapterTest.php | Adds new unit tests covering JMAP mailbox-to-DB mapping behavior. |
| tests/Unit/IMAP/PreviewEnhancerTest.php | Updates preview enhancer unit tests for new attachment API signature. |
| tests/Unit/IMAP/MailboxSyncTest.php | Refactors mailbox sync unit tests to use ProtocolFactory. |
| tests/Unit/Controller/ThreadControllerTest.php | Updates controller test wiring to use MailManager class directly. |
| tests/Unit/Controller/MessageApiControllerTest.php | Removes IMAP client factory from controller tests; uses getRawMessage style APIs. |
| tests/Unit/Controller/MailboxesControllerTest.php | Updates patch behavior tests to assert mailbox mapper update. |
| tests/Unit/Controller/MailboxesApiControllerTest.php | Adjusts API controller test DI to use MailManager. |
| tests/Unit/Controller/AccountsControllerTest.php | Updates accounts controller tests to use MailManager class directly. |
| tests/Unit/BackgroundJob/SyncJobTest.php | Updates background job tests to validate connector-based sync execution paths. |
| tests/Unit/BackgroundJob/ContextChat/SubmitContentJobTest.php | Removes explicit IMAP client creation/close expectations from context chat job tests. |
| tests/Integration/Sync/ImapToDbSynchronizerTest.php | Updates integration test service retrieval to MailManager. |
| tests/Integration/Service/OutboxServiceIntegrationTest.php | Switches integration wiring from IMAPClientFactory/IMailManager to ProtocolFactory/MailManager. |
| tests/Integration/Service/MailTransmissionIntegrationTest.php | Refactors integration wiring to use ProtocolFactory and MailManager. |
| tests/Integration/Service/DraftServiceIntegrationTest.php | Refactors draft integration test wiring to ProtocolFactory and MailManager. |
| tests/Integration/Service/AntiSpamServiceIntegrationTest.php | Updates integration test calls to flagMessages(...) and uses MailManager. |
| tests/Integration/MailboxSynchronizationTest.php | Updates integration test wiring and raw-message fetch API usage. |
| tests/Integration/IMAP/MessageMapperTest.php | Switches integration test service retrieval to MailManager. |
| tests/Integration/Framework/JmapTestAccount.php | Adjusts JMAP integration account host/port for stable Stalwart addressing. |
| tests/Integration/Db/TagMapperTest.php | Adds integration test for userId persistence when inserting tags via tagMessage(). |
| tests/Integration/Connector/JmapMailboxConnectorTest.php | Adds JMAP mailbox connector contract test implementation. |
| tests/Integration/Connector/ImapMailboxConnectorTest.php | Adds IMAP mailbox connector contract test implementation. |
| tests/Integration/Connector/AbstractMailboxConnectorTest.php | Adds shared contract test suite for IMailboxConnector behavior across protocols. |
| lib/SetupChecks/MailConnectionPerformance.php | Uses ProtocolFactory->imapClient() inside performance check. |
| lib/Service/Sync/SyncService.php | Reworks sync flow to call mailbox/message connectors instead of direct IMAP client + synchronizer calls. |
| lib/Service/Sync/ImapToDbSynchronizer.php | Migrates IMAP client creation to ProtocolFactory and updates permflags check API. |
| lib/Service/SnoozeService.php | Migrates move/flag operations to mailbox/message object-based MailManager APIs; removes explicit IMAP client usage in wake flow. |
| lib/Service/SetupService.php | Switches connectivity checks to ProtocolFactory->imapClient(). |
| lib/Service/Search/MailSearch.php | Switches remote search to MailManager->findMessages(). |
| lib/Service/OutboxService.php | Uses ProtocolFactory->imapClient() for attachment handling paths. |
| lib/Service/MailTransmission.php | Switches IMAP client creation to ProtocolFactory->imapClient() and migrates manager dependency to MailManager. |
| lib/Service/ItineraryService.php | Refactors itinerary extraction to fetch message/attachments via MailManager and take Message parameter. |
| lib/Service/IMipService.php | Switches schedule processing fetch to variadic MailManager->getImapMessages(...). |
| lib/Service/DraftsService.php | Uses ProtocolFactory->imapClient() for attachment upload paths. |
| lib/Service/DkimService.php | Refactors DKIM validation to use ProtocolFactory->messageConnector()->fetchMessageRaw() and accept Message. |
| lib/Service/Classification/NewMessagesClassifier.php | Migrates flag/tag calls to batch-style MailManager APIs using Mailbox + Message. |
| lib/Service/Attachment/AttachmentService.php | Removes explicit IMAP client argument from getAttachmentNames(); fetches via MailManager. |
| lib/Service/AntiSpamService.php | Uses ProtocolFactory->imapClient() for raw message access and sent-copy store logic. |
| lib/Service/AiIntegrations/AiIntegrationsService.php | Removes explicit IMAP client lifecycle; fetches messages via MailManager->getImapMessage(...). |
| lib/Service/AccountService.php | Uses ProtocolFactory->testConnection() for connection tests. |
| lib/Send/Chain.php | Switches IMAP client creation to ProtocolFactory->imapClient(). |
| lib/Protocol/ProtocolFactory.php | Implements connector resolution map and adds testConnection() helper. |
| lib/Model/IMAPMessage.php | Adds hasDkimSignature() accessor. |
| lib/Listener/MoveJunkListener.php | Refactors junk move logic to fetch Message by uid and use mailbox objects (incl. inbox special-use lookup). |
| lib/Listener/DeleteDraftListener.php | Switches IMAP client creation to ProtocolFactory->imapClient(). |
| lib/JMAP/JmapMailboxAdapter.php | Adds adapter mapping JMAP mailbox responses to DB mailbox entities and back. |
| lib/JMAP/JmapClientFactory.php | Uses transport mode constants and simplifies host:port composition. |
| lib/JMAP/Exception/JmapUnknownMethod.php | Adds specific exception type for unknown JMAP methods. |
| lib/IMAP/Search/Provider.php | Removes legacy IMAP-only search provider in favor of manager-based search. |
| lib/IMAP/PreviewEnhancer.php | Removes unconditional IMAP client usage; adds non-IMAP fast-path and updated attachment API usage. |
| lib/IMAP/MailboxSync.php | Uses ProtocolFactory->imapClient() and shortens sync skip window. |
| lib/IMAP/ImapMailboxConnector.php | Adds IMAP implementation of IMailboxConnector. |
| lib/Db/TagMapper.php | Fixes tag insert path to set userId before insert. |
| lib/Db/MessageMapper.php | Adds remote_id support in bulk insert and new find/delete-by-remote-id APIs. |
| lib/Db/Message.php | Updates setReferences docblock type to allow null. |
| lib/Db/MailboxMapper.php | Adds findByName() and helper findSpecialUseMailbox(). |
| lib/Db/Mailbox.php | Expands cached-state detection to include non-null state. |
| lib/Controller/ThreadController.php | Updates dependency type from interface to MailManager. |
| lib/Controller/TagsController.php | Updates dependency type from interface to MailManager. |
| lib/Controller/PageController.php | Updates dependency type from interface to MailManager. |
| lib/Controller/MessagesController.php | Removes IMAP client factory lifecycle; uses mailbox/message object APIs and renames source endpoint handler. |
| lib/Controller/MessageApiController.php | Removes IMAP client factory lifecycle; uses getRawMessage(...) API. |
| lib/Controller/MailboxesController.php | Switches background sync toggle persistence to MailboxMapper->update(). |
| lib/Controller/MailboxesApiController.php | Updates dependency type from interface to MailManager. |
| lib/Controller/ListController.php | Removes direct IMAP client usage and relies on MailManager->getImapMessage(...). |
| lib/Controller/AccountsController.php | Updates dependency type from interface to MailManager. |
| lib/Contracts/IMessageConnector.php | Fixes variadic parameter name for deleteMessages(...) to plural. |
| lib/Contracts/IMailManager.php | Removes legacy interface (replaced by concrete MailManager usage). |
| lib/Contracts/IDkimService.php | Updates DKIM API to accept Message instead of int uid/id. |
| lib/Command/SyncAccount.php | Switches CLI sync to connector-based sync for mailbox + messages. |
| lib/BackgroundJob/TrashRetentionJob.php | Removes explicit IMAP client; deletes messages via MailManager using Message. |
| lib/BackgroundJob/SyncJob.php | Migrates background sync to connector-based sync and gates IMAP auth check by protocol. |
| lib/BackgroundJob/RepairSyncJob.php | Adds mailbox refresh via connector and skips IMAP repair sync for non-IMAP accounts. |
| lib/BackgroundJob/QuotaJob.php | Updates dependency type from interface to MailManager. |
| lib/BackgroundJob/MigrateImportantJob.php | Uses ProtocolFactory->imapClient() and updated permflags check API. |
| lib/BackgroundJob/FollowUpClassifierJob.php | Switches to batch tagging API and message-id lookup API changes. |
| lib/BackgroundJob/ContextChat/SubmitContentJob.php | Removes explicit IMAP client lifecycle; fetches messages via MailManager->getImapMessage(...). |
| lib/AppInfo/Application.php | Removes IMailManager service alias registration (now using MailManager directly). |
| appinfo/routes.php | Renames messages controller route handler to getRawMessage (URL unchanged). |
| .github/workflows/test.yml | Stabilizes Stalwart hostname/port, adds host mapping, updates setup curls for JMAP integration tests. |
Suppressed comments (3)
lib/Protocol/ProtocolFactory.php:1
ProtocolFactory::transmissionConnector()is still present, but theITransmissionConnectormappings are commented out. Any runtime call totransmissionConnector()will now throwServiceExceptioneven for IMAP accounts. Either restore the IMAP mapping (and explicitly fail only for JMAP with a clear error), or remove/guard thetransmissionConnector()method until the sending PR lands.
lib/JMAP/JmapMailboxAdapter.php:1- This treats
null(e.g., property not present in the response) as subscribed and will incorrectly mark mailboxes as\\subscribed. Consider only adding\\subscribedwhensubscribed() === true, and omitting the attribute when the server did not provide that property.
lib/Service/IMipService.php:1 - Fix typo in log message: 'form' should be 'from'.
| } | ||
|
|
||
| foreach ($this->clientFactory->getLoginStats() as $host => $count) { | ||
| $consoleLogger->debug(sprintf('%d IMAP connection(s) to %s', $count, $host)); |
There was a problem hiding this comment.
Is that coming back for imap? ;)
There was a problem hiding this comment.
This will have to be reworked for both protocols. We can't really have protocol specific things in any commands or controllers. I've added it to my list of follow up PR's
| bool $force = false, | ||
| ?Horde_Imap_Client_Socket $client = null): void { | ||
| if (!$force && $account->getMailAccount()->getLastMailboxSync() >= ($this->timeFactory->getTime() - 7200)) { | ||
| if (!$force && $account->getMailAccount()->getLastMailboxSync() >= ($this->timeFactory->getTime() - 900)) { |
There was a problem hiding this comment.
I can revert this, I lowered it for testing originally to 5min then when putting it back, I was like "we only sync mailboxes every two hours?" so made it 15min
kesselb
left a comment
kesselb
left a comment
There was a problem hiding this comment.
Made it till JmapMailboxConnector
|
|
||
| $this->protocolFactory | ||
| ->mailboxConnector($account) | ||
| ->syncAll($account, true); |
| } catch (DoesNotExistException $e) { | ||
| throw new ServiceException('No trash folder', 0, $e); | ||
| } | ||
| $operation = $sourceMailbox->getId() === $trashMailbox->getId() ? 'delete' : 'move'; |
There was a problem hiding this comment.
Why? the values are only used this one function, and are just a way to identify which operation we are doing. they have no actual meaning in jmap, this can be "m" and "d"
| return 'important'; | ||
| } | ||
|
|
||
| $allowed = ['all', 'archive', 'drafts', 'important', 'inbox', 'junk', 'sent', 'trash']; |
| return $role === 'important' ? 'flagged' : $role; | ||
| } | ||
|
|
||
| private function convertToAcl(MailboxParametersResponse $response): ?string { |
There was a problem hiding this comment.
| private function convertToAcl(MailboxParametersResponse $response): ?string { | |
| private function convertToAcl(?MailboxPermission $rights): ?string { |
| class JmapMailboxConnector implements IMailboxConnector { | ||
| use TTransactional; | ||
|
|
||
| private const MAILBOX_SYNC_TTL = 300; |
There was a problem hiding this comment.
Move const to the interface or would we use different ttl for jamp/imap?
There was a problem hiding this comment.
I want to use independent mailbox sync values, because jmap has a delta sync for mailboxes. that means we can do a very quick check, very often, unlike imap which is slow due to the need to pull the entire tree.
Some context in the description please109 files, 5467 additions, ~2498 deletions. That's a lot to take in, and right now the PR doesn't say why any of it looks the way it does. I don't need a walkthrough of every change, the diff already tells me what happened. What I'm missing is the reasoning behind the structural decisions: which boundaries you drew, and what the alternatives were that you rejected. Without that I can't tell an intentional design from an accident, which is what I'd be approving here. As Christoph already mentioned, dropping IMailManager is the prime example. Lost annotationsWe're losing a lot of helpful annotations, the thrown exceptions in particular. getImapMessage is the clearest example, see the usage in SubmitContentJob. On main you can see that IMailManager.getImapMessage may throw a ServiceException or a SmimeDecryptException, and the implementation in MailManager enforces that by wrapping DoesNotExistException, Horde_Mime_Exception and Horde_Imap_Client_Exception into a ServiceException. On this branch:
Independent of the IMailManager question, the new interfaces should carry proper throw annotations. I'm quite sure we have tons of missing or incomplete docblocks on main, and I'm not asking you to fix those, but we should maintain the current level for the code you touch here. getImapMessages still documents Variadics madnessI'd like to understand the reasoning, because passing lists of objects around seems cleaner to me.
The usage in MessagesController.mdn, setFlags and setTag is another example why I'm absolutely not a fan of the variadic overuse here. Passing the list of messages last just reads weird:
In fairness, updateBulk and deleteByUid already use variadics on main, so you didn't invent this. But this PR extends the pattern much further, which makes it a good moment to decide deliberately. Clients and loopsIn a couple of places, TrashRetentionJob for example, we previously re-used one client for a whole list of messages. I understand why the refactoring lost that, and for a background job the trade-off is acceptable, but please log a technical debt issue for it. I assume we can dispatch a single deleteMessages call, since all those messages are in the same mailbox, and optimize the retention mapper code path in the same go. Also, in flagMessages, tagMessages, moveMessages and deleteMessages the $client->logout() sits after the try/catch instead of inside a finally. Unrelated changesFollowUpClassifierJob currently carries two changes:
Might be that getMessagesByMessageId is the better name, might be that a single tagMessages is less to maintain. But both are unrelated to this PR and are just noise while reviewing. The second one is only half unrelated, since it also swaps string $mailboxName for a Mailbox, which the abstraction genuinely needs. Please say so in the description. In the same go, keeping MailManager.deleteMessage as a wrapper for the batch function is a bit inconsistent. Renaming MessagesController.getSource to getRawMessage also seems unrelated to introducing JMAP, and it changes the route id mail.messages.getSource even though the URL stays the same. Two things that look like bugs
|
Not just right now, it was also a lot to de-spagettie, 10 years of code, and to implement a new protocol in 4 weeks.
Well, did he really thought? Looks like it was Claude code that made the comments... But I will response never the less... Before responding point by point, I want to clarify how I treated #12610 and #12611. They were a high-level Ai generated breakdown of an earlier Ai generated working plan, intended to define the outcomes and divide the work, rather than freeze the final class structure. #12610 asks for state-based JMAP mailbox and message sync using the same domain models as IMAP. #12611 asks for flag, move, copy, and delete operations, and explicitly says to reuse or extend the existing service-layer abstractions through a dispatcher so that the service layer remains protocol-agnostic. Neither ticket says that The implementation did drift from the class layout suggested in #12610. Instead of separate Message identity was the most important issue. The existing request path passed an integer IMAP UID from the controller through the service layer to the client. An IMAP UID is a native, mailbox-scoped remote identifier, whereas a JMAP Email ID is an opaque string that remains stable when the message moves between mailboxes. Preserving the old UID-based boundary for JMAP would have required synthetic integer UIDs to be kept aligned through every operation, including moves, even though those integers have no JMAP meaning. It would also require an additional database lookup for each operation to translate the supplied UID into the JMAP Because of that mismatch, dispatching only client construction would not have made the service layer protocol-agnostic. It would have retained IMAP identity semantics and moved the protocol branches and ID translation into downstream services. Dispatching complete protocol operations through connectors keeps callers independent of the concrete client and its identifier model. This is how I interpreted #12611's requirement that the service layer remain protocol-agnostic. In hindsight, the tickets' assumption that the existing abstractions could simply be reused through a client dispatcher was incomplete because it did not account for this identity-model difference. I agree that this reasoning and the differences from the suggested class layout should have been explained in the PR description earlier. This context also does not make the incidental renames, the
Agreed. The connector interfaces should document their exception contracts, and the implementations should consistently translate protocol-specific failures into the service-level exceptions expected by callers. I will also correct the stale
The variadics were a deliberate choice: they keep each argument typed as
Agreed on the
These changes came from making
Agreed; both are bugs rather than intentional design changes. I will pass |
Thank you, for getting that far |
|
I have addressed / changed all requested via comments... will clean up CI once code is approved. |
|
I moved the psalm-types to the class level docblock, otherwise the import from other classes wont work. |
Saw that thank you. I address the last comment you made |
|
I have addressed / changed all requested in your most recent comments |
|
@DerDreschner since you tested this positively, could you please review and approve? Thanks |
ChristophWurst
left a comment
ChristophWurst
left a comment
There was a problem hiding this comment.
As far as I can see you removed the explicit try-finally handling of IMAP connections as resource.
There is no replacement mechanism to manage them.
Doesn't this mean that we might end up with a pile of open connections by the end of a cron execution but possibly also during initial sync when we need to hop between caching and non-caching clients?
As a reminder, there is a common per-IP connection limit on mail servers. IIRC it was five connections per IP for Dovecot. That would bite us rather quickly.
The last commit addresses the open connections
As for this connection reuse/caching is on my list of after this is merged follow ups, |
|
Findings from our co-working session: Bugs:
Ideas:
|
Fixed
Fixed
Fixed |
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
|
Bypassed codecov requirement... will add more tests in separate PR |
Summary
Adds a protocol abstraction so Mail can talk to both IMAP and JMAP servers through a
common set of interfaces, and reworks the existing IMAP code to implement them.
ProtocolFactory— selects the right connector for an account's protocolIMailboxConnector(+ImapMailboxConnector,JmapMailboxConnector)IMessageConnector(+ImapMessageConnector,JmapMessageConnector)Scope
Sending operations are in feat: jmap support - part 2b #12871.
Automated tests
composer psalmcleancomposer test:unitgreencomposer test:integrationgreencomposer cs:checkcleanTesting
Every operation that talks to an external server must be tested twice — once against an IMAP account/server and once against a JMAP account/server.
Copy a block below into a comment and tick the boxes for the server you tested against.
Each operation has one checkbox per protocol.
Mailbox operations
Message operations
Console operations
mail:account:create— alias ofmail:account:create-imapmail:account:create-imapmail:account:create-jmapmail:account:debugmail:account:deletemail:account:diagnose— alias ofmail:account:testmail:account:exportmail:account:export-threadsmail:account:run-meta-estimatormail:account:syncmail:account:testmail:account:trainmail:account:updatemail:clean-upmail:predict-importancemail:repair:system-autorespondersmail:repair:tagsmail:tags:migration-jobsmail:thread