Repository navigation
feat(db-repository): replace destroy with destroyAllByServerId and destroyByServerId - #11613
rafaeltonholo wants to merge 5 commits into
Conversation
rafaeltonholo
added
the
report: exclude
rafaeltonholo
added
the
pr: stacked
github-actions
Bot
added
the
tb-team
2625216 to
b308012
Compare
81352d0 to
302be1d
Compare
f30e684 to
6235fb8
Compare
6235fb8 to
ffc9344
Compare
GitHub Actions
Error
ffc9344 to
d1e6bdd
Compare
thunderbird-botmobile
Bot
added
the
pr-sentinel: ready for review
d1e6bdd to
eafbcb7
Compare
eafbcb7 to
48aba89
Compare
48aba89 to
5d294a0
Compare
GitHub Actions
Error
5d294a0 to
384978b
Compare
384978b to
5aa41e7
Compare
There was a problem hiding this comment.
This review took some time because these changes touch core application behavior and change it. While refactoring the lifecycle repository to use MessageLifecycleRepository for CRUD operations is the right move, I noticed some issues regarding message content integrity that could lead to data loss.
MessageLifecycleRepository.update()rebuilds the message from the domain model, which loses data. This affects re-downloaded messages and local drafts.- Move to Drafts saves the draft under the source message server UID, which can lose the message.
- Each save copies its attachments into
cacheDir, but the copies are never used. We also have an attachment store, meaning most attachments are stored twice now. The cache copies stay even when the user deleted the related message and there is no cleanup. - Too verbose logging that prints whole messages
create()/update()throw exceptions where other operations return error outcomes.
| logger.debug { | ||
| "$LOG_ID creating new message in folder '${folderId ?: "<outbox-id>"}' for account '$accountId'" | ||
| } | ||
| logger.verbose { "$LOG_ID message = $message" } |
There was a problem hiding this comment.
Why is this writing the whole message to the logger?
Prints:
- addresses
- subject
- plain-text and HTML body
- headers
Leaks subject, display name and the full plain-text and HTML body.
I think the logger should be used to aid debugging, but not write everything the app does to the logs.
There was a problem hiding this comment.
The message object is an object backed by the PII-safe compiler plugin. The addresses are already marked as PII, except for their label.
For the subject, body and headers, we can mark them also as PII.
There was a problem hiding this comment.
The MessageHeader is also marked as HasPii, except by the extra property.
| logger.debug { | ||
| "$LOG_ID updating message '$messageId' in folder '$folderId' for account '$accountId'" | ||
| } | ||
| logger.verbose { "$LOG_ID message = $message" } |
There was a problem hiding this comment.
Why is this writing the whole message to the logger?
| val domainFolderId = LegacyFolderIdFactory.of(folderId) | ||
| // A message coming from the backend carries no X_DOWNLOADED_* flags yet, so the mapper would | ||
| // infer ENVELOPE. The backend tells us explicitly how much was downloaded; that must win. | ||
| val domainMessage = mapper.toDomain(message).copy(downloadState = downloadState.toDomainDownloadState()) |
There was a problem hiding this comment.
mapper.toDomain(message) runs AttachmentResolver.resolveAttachmentFile on every attachment, which copies the decoded content into cacheDir. The message store already keeps the same attachment encoded (in the attachment directory above 16 KB, in the database below). So every synced, sent or saved-draft message now stores its attachments twice and the cache copy isn't even used: create() saves the original message through LegacyMessageSource.
Nothing deletes these copies. deleteStaleVersions only runs for a LocalPart, and deleting or moving a message doesn't touch them. The decoded content can also stay on disk after the user deletes the message.
The same happens in sendMessageCompat and DraftOperations.toDomainMessage.
This also adds extra IO to every message save.
| val messageData = message.toSaveMessageData(plaintextSubject) | ||
|
|
||
| return messageStore.saveLocalMessage(folderId, messageData, existingDraftId) | ||
| val domainMessage = message.toDomainMessage(plaintextSubject, account, folderId) |
There was a problem hiding this comment.
toDomainMessage writes the plaintext subject into envelope.subject, and then update() → toDto writes it to the stored Subject: header. For an encrypted draft, that replaces ... with the real subject, which undoes subject protection. Before this change, the plaintext subject only went into the display-subject column. Please pass it separately rather than through the envelope.
| private suspend fun LegacyMessage.toDomainMessage( | ||
| subject: String?, | ||
| account: LegacyAccountDto, | ||
| folderId: Long, | ||
| ): Message { | ||
| setFlag(Flag.X_DOWNLOADED_FULL, true) | ||
| accountId = account.id | ||
| val domainMessage = messageDataMapper.toDomain(this) | ||
| val envelop = if (subject.isNullOrBlank()) { | ||
| domainMessage.envelope | ||
| } else { | ||
| domainMessage.envelope.copy(subject = subject) | ||
| } | ||
| return domainMessage.copy( | ||
| folderId = LegacyFolderIdFactory.of(folderId), | ||
| envelope = envelop, | ||
| ) |
There was a problem hiding this comment.
moveToDraftsFolderInBackground (MessagingController.java:1995) passes in a LocalMessage that still has its server UID from the source folder. The mapper turns that UID into serverId, so create() calls saveRemoteMessage(draftsFolder, <source UID>). Before this change, saveLocalMessage always generated a local placeholder UID.
What follows:
processPendingAppendskips the upload because the UID has no local prefix
(MessagingController.java:875).- The next Drafts sync deletes the row, because that UID isn't on the server. If the UID happens to match a real draft, the row gets overwritten, or create fails with
MessageAlreadyExists. - The source message has already been destroyed by then, so the content is lost.
Maybe clearing serverId and id here so drafts are always created as local messages.
…sitory - Wire MessageLifecycleRepository into MessagingController for sending messages - Replace direct MessageStore calls with repository.create() in sendMessage() - Add DefaultMessageLifecycleRepository implementing create() with legacy adapter logic - Add messageLifecycleRepository, messageDataMapper, and messageIdLegacyEntityIdFactory dependencies - Convert domain Message to legacy DTO, save to outbox folder, and initialize outbox state - Add MessageLifecycleRepository registration in MailMessageModule - Add feature.mail.message.api dependency to legacy core GG-ID: c-639ad93
…sitory - Add update(message, accountId, folderId) to MessageLifecycleRepository with legacy adapter - Add MessageAlreadyExists error and duplicate detection in create() via MessageQueryRepository - Use create/update in K9BackendFolder.saveMessage, recovering from MessageAlreadyExists - Use create/update in DraftOperations.saveDraftLocally - Derive legacy download state from message flags in the mapper - Add update and duplicate-detection tests to DefaultMessageLifecycleRepositoryTest GG-ID: c-a66bc30 GG-Parent: c-639ad93
…tory - Add move(messageId, destinationFolderId, accountId) and moveAll() to MessageLifecycleRepository - Implement both in DefaultMessageLifecycleRepository with legacy adapter, mapping store exceptions to UnhandledError - Replace MessageStore.moveMessage/moveMessages calls in MessagingController with repository move/moveAll - Add move/moveAll tests to DefaultMessageLifecycleRepositoryTest and sent-message move tests to MessagingControllerTest GG-ID: c-7f7af18 GG-Parent: c-a66bc30
…pository - Add accountId parameter to MessageLifecycleRepository.copy - Add copyAll for copying multiple messages to a destination folder - Implement both operations in DefaultMessageLifecycleRepository via legacy MessageStore - Replace direct messageStore.copyMessages call in MessagingController with repository-backed copyAll - Extract convertLegacyMessageIdsToDomainMessageIds and convertDomainIdMappingToLegacyIdMapping helpers - Add tests for copy and copyAll success, store failure, and unknown account cases GG-ID: c-145c77c GG-Parent: c-7f7af18
…stroyByServerId - implement message destruction in DefaultMessageLifecycleRepository instead of TODO - add MessageQueryRepository.getAllServerIdByFolderId and back K9BackendFolder clearAllMessages/destroyMessages with the new repositories - make BackendFolder.destroyMessages and clearAllMessages suspend - introduce MessageQueryError.AccountNotFound and UnhandledError, making throwable nullable GG-ID: c-99f9e08 GG-Parent: c-145c77c
5aa41e7 to
1475601
Compare
This branch was successfully deployed
3 participants
Stack
Contribution Summary
Linked Issue/Ticket: Closes #11486
RFC / Technical Design (if applicable):
Description
AI Disclosure
Select one of the following (mandatory)
Contribution Checklist
gradlew spotlessCheckto check andgradlew spotlessApplyto format your source code; will be checked by CI).gradlew testDebugUnitTest; will be checked by CI).