From 72396cfca3e2d96bd28924057aee8e47c0844283 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Tue, 19 Mar 2024 10:30:08 +1100 Subject: [PATCH] feat: fix message attachment cleanup and handle group attach+msg delete --- ts/data/data.ts | 10 +-- ts/data/dataInit.ts | 2 +- ts/models/message.ts | 9 ++- ts/node/sql.ts | 31 +++------ .../SwarmPollingGroupConfig.ts | 24 ++++++- ts/types/MessageAttachment.ts | 34 ++++++++-- ts/types/attachments/migrations.ts | 67 +++++++++---------- 7 files changed, 102 insertions(+), 75 deletions(-) diff --git a/ts/data/data.ts b/ts/data/data.ts index e2fea1e85..403b9c72e 100644 --- a/ts/data/data.ts +++ b/ts/data/data.ts @@ -292,11 +292,13 @@ async function removeAllMessagesInConversationSentBefore(args: { return channels.removeAllMessagesInConversationSentBefore(args); } -async function removeAllAttachmentsInConversationSentBefore(args: { +async function getAllMessagesWithAttachmentsInConversationSentBefore(args: { deleteAttachBeforeSeconds: number; conversationId: GroupPubkeyType; -}): Promise> { - return channels.removeAllAttachmentsInConversationSentBefore(args); +}): Promise> { + const msgAttrs = await channels.getAllMessagesWithAttachmentsInConversationSentBefore(args); + + return msgAttrs.map((msg: any) => new MessageModel(msg)); } async function getMessageIdsFromServerIds( @@ -839,7 +841,7 @@ export const Data = { removeMessage, removeMessagesByIds, removeAllMessagesInConversationSentBefore, - removeAllAttachmentsInConversationSentBefore, + getAllMessagesWithAttachmentsInConversationSentBefore, cleanUpExpirationTimerUpdateHistory, getMessageIdsFromServerIds, getMessageById, diff --git a/ts/data/dataInit.ts b/ts/data/dataInit.ts index 9194af553..8e14cf548 100644 --- a/ts/data/dataInit.ts +++ b/ts/data/dataInit.ts @@ -41,7 +41,7 @@ const channelsToMake = new Set([ 'saveMessages', 'removeMessage', 'removeMessagesByIds', - 'removeAllAttachmentsInConversationSentBefore', + 'getAllMessagesWithAttachmentsInConversationSentBefore', 'removeAllMessagesInConversationSentBefore', 'cleanUpExpirationTimerUpdateHistory', 'getUnreadByConversation', diff --git a/ts/models/message.ts b/ts/models/message.ts index 8362c9aea..af565014b 100644 --- a/ts/models/message.ts +++ b/ts/models/message.ts @@ -301,12 +301,11 @@ export class MessageModel extends Backbone.Model { return ''; } - public onDestroy() { - void this.cleanup(); - } - public async cleanup() { - await deleteExternalMessageFiles(this.attributes); + const changed = await deleteExternalMessageFiles(this.attributes); + if (changed) { + await this.commit(); + } } public getPropsForExpiringMessage(): PropsForExpiringMessage { diff --git a/ts/node/sql.ts b/ts/node/sql.ts index fef4d20a7..51b8c74cc 100644 --- a/ts/node/sql.ts +++ b/ts/node/sql.ts @@ -1003,7 +1003,7 @@ function removeAllMessagesInConversationSentBefore( .prepare( `SELECT id FROM ${MESSAGES_TABLE} WHERE conversationId = $conversationId AND sent_at <= $beforeMs;` ) - .all({ conversationId, beforeMs: deleteBeforeSeconds * 1000 }) as Array; + .all({ conversationId, beforeMs: deleteBeforeSeconds * 1000 }); assertGlobalInstanceOrInstance(instance) .prepare( @@ -1011,10 +1011,10 @@ function removeAllMessagesInConversationSentBefore( ) .run({ conversationId, beforeMs: deleteBeforeSeconds * 1000 }); console.info('removeAllMessagesInConversationSentBefore deleted msgIds:', JSON.stringify(msgIds)); - return msgIds; + return msgIds.map(m => m.id); } -async function removeAllAttachmentsInConversationSentBefore( +async function getAllMessagesWithAttachmentsInConversationSentBefore( { deleteAttachBeforeSeconds, conversationId, @@ -1023,29 +1023,14 @@ async function removeAllAttachmentsInConversationSentBefore( ) { const rows = assertGlobalInstanceOrInstance(instance) .prepare( - `SELECT json FROM ${MESSAGES_TABLE} WHERE conversationId = $conversationId AND sent_at <= $beforeMs` + `SELECT json FROM ${MESSAGES_TABLE} WHERE conversationId = $conversationId AND sent_at <= $beforeMs;` ) .all({ conversationId, beforeMs: deleteAttachBeforeSeconds * 1000 }); const messages = map(rows, row => jsonToObject(row.json)); - - const externalFiles: Array = []; - const msgIdsWithChanges: Array = []; - forEach(messages, message => { - const externalFilesMsg = getExternalFilesForMessage(message); - if (externalFilesMsg.length) { - externalFiles.push(...externalFilesMsg); - msgIdsWithChanges.push(); - } + const messagesWithAttachments = messages.filter(m => { + return getExternalFilesForMessage(m).some(a => !isEmpty(a) && isString(a)); // when we remove an attachment, we set the path to '' so it should be excluded here }); - const uniqPathsToRemove = uniq(externalFiles); - console.info('removeAllAttachmentsInConversationSentBefore removing attachments:', externalFiles); - console.info( - 'removeAllAttachmentsInConversationSentBefore impacted msgIds:', - JSON.stringify(msgIdsWithChanges) - ); - - const userDataPath = app.getPath('userData'); - await deleteAll({ userDataPath, attachments: uniqPathsToRemove }); + return messagesWithAttachments; } function removeAllMessagesInConversation( @@ -2553,7 +2538,7 @@ export const sqlNode = { removeMessage, removeMessagesByIds, removeAllMessagesInConversationSentBefore, - removeAllAttachmentsInConversationSentBefore, + getAllMessagesWithAttachmentsInConversationSentBefore, cleanUpExpirationTimerUpdateHistory, removeAllMessagesInConversation, getUnreadByConversation, diff --git a/ts/session/apis/snode_api/swarm_polling_config/SwarmPollingGroupConfig.ts b/ts/session/apis/snode_api/swarm_polling_config/SwarmPollingGroupConfig.ts index 487e9a48c..f3fb572db 100644 --- a/ts/session/apis/snode_api/swarm_polling_config/SwarmPollingGroupConfig.ts +++ b/ts/session/apis/snode_api/swarm_polling_config/SwarmPollingGroupConfig.ts @@ -1,6 +1,7 @@ import { GroupPubkeyType } from 'libsession_util_nodejs'; import { isFinite, isNumber } from 'lodash'; import { Data } from '../../../../data/data'; +import { messagesExpired } from '../../../../state/ducks/conversations'; import { groupInfoActions } from '../../../../state/ducks/metaGroups'; import { MetaGroupWrapperActions } from '../../../../webworker/workers/browser/libsession_worker_interface'; import { ed25519Str } from '../../../onions/onionPath'; @@ -22,7 +23,13 @@ async function handleMetaMergeResults(groupPk: GroupPubkeyType) { deleteBeforeSeconds: infos.deleteBeforeSeconds, conversationId: groupPk, }); - console.warn('deletedMsgIds', deletedMsgIds); + window.log.info( + `removeAllMessagesInConversationSentBefore of ${ed25519Str(groupPk)} before ${infos.deleteBeforeSeconds}: `, + deletedMsgIds + ); + await window.inboxStore.dispatch( + messagesExpired(deletedMsgIds.map(messageId => ({ conversationKey: groupPk, messageId }))) + ); } if ( @@ -32,11 +39,22 @@ async function handleMetaMergeResults(groupPk: GroupPubkeyType) { infos.deleteAttachBeforeSeconds > 0 ) { // delete any attachments in this conversation sent before that timestamp (in seconds) - const impactedMsgids = await Data.removeAllAttachmentsInConversationSentBefore({ + const impactedMsgModels = await Data.getAllMessagesWithAttachmentsInConversationSentBefore({ deleteAttachBeforeSeconds: infos.deleteAttachBeforeSeconds, conversationId: groupPk, }); - console.warn('impactedMsgids', impactedMsgids); + window.log.info( + `getAllMessagesWithAttachmentsInConversationSentBefore of ${ed25519Str(groupPk)} before ${infos.deleteAttachBeforeSeconds}: impactedMsgModelsIds `, + impactedMsgModels.map(m => m.id) + ); + + for (let index = 0; index < impactedMsgModels.length; index++) { + const msg = impactedMsgModels[index]; + + // eslint-disable-next-line no-await-in-loop + // eslint-disable-next-line no-await-in-loop + await msg?.cleanup(); + } } } diff --git a/ts/types/MessageAttachment.ts b/ts/types/MessageAttachment.ts index bff81b474..3b2e2cf5c 100644 --- a/ts/types/MessageAttachment.ts +++ b/ts/types/MessageAttachment.ts @@ -11,20 +11,39 @@ import { autoOrientJPEGAttachment, captureDimensionsAndScreenshot, deleteData, + deleteDataSuccessful, loadData, replaceUnicodeV2, } from './attachments/migrations'; // NOTE I think this is only used on the renderer side, but how?! -export const deleteExternalMessageFiles = async (message: { - attachments: any; - quote: any; - preview: any; +export const deleteExternalMessageFiles = async (messageAttributes: { + attachments: Array | undefined; + quote: { attachments: Array | undefined }; + preview: Array | undefined; }) => { - const { attachments, quote, preview } = message; + let anyChanges = false; + const { attachments, quote, preview } = messageAttributes; if (attachments && attachments.length) { await Promise.all(attachments.map(deleteData)); + anyChanges = true; + + // test that the files were deleted successfully + try { + let results = await Promise.allSettled(attachments.map(deleteDataSuccessful)); + results = results.filter(result => result.status === 'rejected'); + + if (results.length) { + throw Error; + } + } catch (err) { + // eslint-disable-next-line no-console + console.warn( + '[deleteExternalMessageFiles]: Failed to delete attachments for', + messageAttributes + ); + } } if (quote && quote.attachments && quote.attachments.length) { @@ -41,6 +60,8 @@ export const deleteExternalMessageFiles = async (message: { } attachment.thumbnail = undefined; + anyChanges = true; + return attachment; }) ); @@ -57,10 +78,13 @@ export const deleteExternalMessageFiles = async (message: { } item.image = undefined; + anyChanges = true; + return image; }) ); } + return anyChanges; }; let attachmentsPath: string | undefined; diff --git a/ts/types/attachments/migrations.ts b/ts/types/attachments/migrations.ts index c42ec185d..d35bcf689 100644 --- a/ts/types/attachments/migrations.ts +++ b/ts/types/attachments/migrations.ts @@ -1,8 +1,8 @@ +/* eslint-disable no-param-reassign */ import { arrayBufferToBlob, blobToArrayBuffer } from 'blob-util'; -import { pathExists } from 'fs-extra'; +import fse from 'fs-extra'; import { isString } from 'lodash'; - import * as GoogleChrome from '../../util/GoogleChrome'; import * as MIME from '../MIME'; import { toLogFormat } from './Errors'; @@ -145,26 +145,6 @@ export const loadData = async (attachment: any) => { return { ...attachment, data }; }; -const handleDiskDeletion = async (path: string) => { - await deleteOnDisk(path); - try { - const exists = await pathExists(path); - - // NOTE we want to confirm the path no longer exists - if (exists) { - throw Error('Error: File path still exists.'); - } - - window.log.debug(`deleteDataSuccessful: Deletion succeeded for attachment ${path}`); - return undefined; - } catch (err) { - window.log.warn( - `deleteDataSuccessful: Deletion failed for attachment ${path} ${err.message || err}` - ); - return path; - } -}; - // deleteData :: (RelativePath -> IO Unit) // Attachment -> // IO Unit @@ -177,24 +157,43 @@ export const deleteData = async (attachment: { throw new TypeError('deleteData: attachment is not valid'); } - let { path, thumbnail, screenshot } = attachment; - - if (path && isString(path)) { - const pathAfterDelete = await handleDiskDeletion(path); - path = isString(pathAfterDelete) ? pathAfterDelete : undefined; + const { path, thumbnail, screenshot } = attachment; + if (isString(path)) { + await deleteOnDisk(path); + attachment.path = ''; } - if (thumbnail && isString(thumbnail.path)) { - const pathAfterDelete = await handleDiskDeletion(thumbnail.path); - thumbnail = isString(pathAfterDelete) ? pathAfterDelete : undefined; + await deleteOnDisk(thumbnail.path); + attachment.thumbnail = undefined; } - if (screenshot && isString(screenshot.path)) { - const pathAfterDelete = await handleDiskDeletion(screenshot.path); - screenshot = isString(pathAfterDelete) ? pathAfterDelete : undefined; + await deleteOnDisk(screenshot.path); + attachment.screenshot = undefined; } - return { path, thumbnail, screenshot }; + return attachment; +}; + +export const deleteDataSuccessful = async (attachment: { + path: string; + thumbnail: any; + screenshot: any; +}) => { + const errorMessage = `deleteDataSuccessful: Deletion failed for attachment ${attachment.path}`; + // eslint-disable-next-line @typescript-eslint/no-misused-promises + return fse.pathExists(attachment.path, (err, exists) => { + if (err) { + return Promise.reject(new Error(`${errorMessage} ${err}`)); + } + + // Note we want to confirm the path no longer exists + if (exists) { + return Promise.reject(errorMessage); + } + + window.log.debug(`deleteDataSuccessful: Deletion succeeded for attachment ${attachment.path}`); + return true; + }); }; type CaptureDimensionType = { contentType: string; path: string };