From 541ba6c9deb8c05e80da963a0f88c6033f480a19 Mon Sep 17 00:00:00 2001 From: trevor-signal <131492920+trevor-signal@users.noreply.github.com> Date: Tue, 12 Nov 2024 17:00:46 -0500 Subject: [PATCH] Update quote behavior in backups --- ts/jobs/helpers/sendNormalMessage.ts | 2 +- ts/messages/copyQuote.ts | 1 - ts/messages/helpers.ts | 2 +- ts/model-types.d.ts | 8 +- ts/services/backups/export.ts | 5 +- ts/services/backups/import.ts | 96 ++++---------- .../backups/util/CircularMessageCache.ts | 80 ----------- .../backup/CircularMessageCache_test.ts | 74 ----------- ts/test-electron/backup/attachments_test.ts | 6 - ts/test-electron/backup/bubble_test.ts | 124 ++++++++++++++---- ts/util/getQuoteBodyText.ts | 14 +- ts/util/makeQuote.ts | 10 +- 12 files changed, 142 insertions(+), 280 deletions(-) delete mode 100644 ts/services/backups/util/CircularMessageCache.ts delete mode 100644 ts/test-electron/backup/CircularMessageCache_test.ts diff --git a/ts/jobs/helpers/sendNormalMessage.ts b/ts/jobs/helpers/sendNormalMessage.ts index 9abb5cea873f..51cc76cbc84b 100644 --- a/ts/jobs/helpers/sendNormalMessage.ts +++ b/ts/jobs/helpers/sendNormalMessage.ts @@ -905,7 +905,7 @@ async function uploadMessageQuote({ return { isGiftBadge: loadedQuote.isGiftBadge, - id: loadedQuote.id, + id: loadedQuote.id ?? undefined, authorAci: loadedQuote.authorAci ? normalizeAci(loadedQuote.authorAci, 'sendNormalMessage.quote.authorAci') : undefined, diff --git a/ts/messages/copyQuote.ts b/ts/messages/copyQuote.ts index ccc7d357d0a1..0e22ba7b2d36 100644 --- a/ts/messages/copyQuote.ts +++ b/ts/messages/copyQuote.ts @@ -55,7 +55,6 @@ export const copyFromQuotedMessage = async ( referencedMessageNotFound: false, isGiftBadge: quote.type === SignalService.DataMessage.Quote.Type.GIFT_BADGE, isViewOnce: false, - messageId: '', }; const queryMessage = await messageCache.findBySentAt(id, attributes => diff --git a/ts/messages/helpers.ts b/ts/messages/helpers.ts index 3460ad0ad52d..f1b9907ae439 100644 --- a/ts/messages/helpers.ts +++ b/ts/messages/helpers.ts @@ -117,7 +117,7 @@ export function getPaymentEventDescription( export function isQuoteAMatch( message: ReadonlyMessageAttributesType | null | undefined, conversationId: string, - quote: ReadonlyDeep> + quote: ReadonlyDeep> ): message is ReadonlyMessageAttributesType { if (!message) { return false; diff --git a/ts/model-types.d.ts b/ts/model-types.d.ts index f0d9cd02abb0..d3614750a6ff 100644 --- a/ts/model-types.d.ts +++ b/ts/model-types.d.ts @@ -85,7 +85,6 @@ export type QuotedAttachmentType = { }; export type QuotedMessageType = { - // TODO DESKTOP-3826 attachments: ReadonlyArray; payment?: AnyPaymentEvent; // `author` is an old attribute that holds the author's E164. We shouldn't use it for @@ -93,10 +92,13 @@ export type QuotedMessageType = { author?: string; authorAci?: AciString; bodyRanges?: ReadonlyArray; - id: number; + // id can be null if the referenced message was not found and we imported this quote + // from backup + id: number | null; isGiftBadge?: boolean; isViewOnce: boolean; - messageId: string; + // `messageId` is deprecated + messageId?: string; referencedMessageNotFound: boolean; text?: string; }; diff --git a/ts/services/backups/export.ts b/ts/services/backups/export.ts index 4ded3ec18592..242e277c20b5 100644 --- a/ts/services/backups/export.ts +++ b/ts/services/backups/export.ts @@ -2126,7 +2126,10 @@ export class BackupExportStream extends Readable { } return { - targetSentTimestamp: Long.fromNumber(quote.id), + targetSentTimestamp: + quote.referencedMessageNotFound || quote.id == null + ? null + : Long.fromNumber(quote.id), authorId, text: quote.text != null diff --git a/ts/services/backups/import.ts b/ts/services/backups/import.ts index fd12a69d73ac..26fba0656c96 100644 --- a/ts/services/backups/import.ts +++ b/ts/services/backups/import.ts @@ -89,10 +89,8 @@ import { convertBackupMessageAttachmentToAttachment, convertFilePointerToAttachment, } from './util/filePointers'; -import { CircularMessageCache } from './util/CircularMessageCache'; import { filterAndClean } from '../../types/BodyRange'; import { APPLICATION_OCTET_STREAM, stringToMIMEType } from '../../types/MIME'; -import { copyFromQuotedMessage } from '../../messages/copyQuote'; import { groupAvatarJobQueue } from '../../jobs/groupAvatarJobQueue'; import { AttachmentDownloadManager } from '../../jobs/AttachmentDownloadManager'; import { @@ -119,9 +117,6 @@ const MAX_CONCURRENCY = 10; const SAVE_MESSAGE_BATCH_SIZE = 10000; -// Keep 1000 recent messages in memory to speed up quote lookup. -const RECENT_MESSAGES_CACHE_SIZE = 1000; - type ChatItemParseResult = { message: Partial; additionalMessages: Array>; @@ -219,10 +214,6 @@ export class BackupImportStream extends Writable { private releaseNotesRecipientId: Long | undefined; private releaseNotesChatId: Long | undefined; private pendingGroupAvatars = new Map(); - private recentMessages = new CircularMessageCache({ - size: RECENT_MESSAGES_CACHE_SIZE, - flush: () => this.flushMessages(), - }); private constructor(private readonly backupType: BackupType) { super({ objectMode: true }); @@ -470,7 +461,6 @@ export class BackupImportStream extends Writable { } private async saveMessage(attributes: MessageAttributesType): Promise { - this.recentMessages.push(attributes); this.saveMessageBatch.add(attributes); if (this.saveMessageBatch.size >= SAVE_MESSAGE_BATCH_SIZE) { return this.flushMessages(); @@ -1327,7 +1317,7 @@ export class BackupImportStream extends Writable { if (item.standardMessage) { attributes = { ...attributes, - ...(await this.fromStandardMessage(item.standardMessage, chatConvo.id)), + ...(await this.fromStandardMessage(item.standardMessage)), }; } else if (item.viewOnceMessage) { attributes = { @@ -1561,8 +1551,7 @@ export class BackupImportStream extends Writable { } private async fromStandardMessage( - data: Backups.IStandardMessage, - conversationId: string + data: Backups.IStandardMessage ): Promise> { return { body: data.text?.body || undefined, @@ -1591,9 +1580,7 @@ export class BackupImportStream extends Writable { }) : undefined, reactions: this.fromReactions(data.reactions), - quote: data.quote - ? await this.fromQuote(data.quote, conversationId) - : undefined, + quote: data.quote ? await this.fromQuote(data.quote) : undefined, }; } @@ -1644,10 +1631,7 @@ export class BackupImportStream extends Writable { } = this.fromDirectionDetails(rev, timestamp); return { - ...(await this.fromStandardMessage( - rev.standardMessage, - mainMessage.conversationId - )), + ...(await this.fromStandardMessage(rev.standardMessage)), timestamp, received_at: incrementMessageCounter(), sendStateByConversationId, @@ -1685,29 +1669,7 @@ export class BackupImportStream extends Writable { return result; } - private convertQuoteType( - type: Backups.Quote.Type | null | undefined - ): SignalService.DataMessage.Quote.Type { - switch (type) { - case Backups.Quote.Type.GIFT_BADGE: - return SignalService.DataMessage.Quote.Type.GIFT_BADGE; - case Backups.Quote.Type.VIEW_ONCE: - // No special treatment, we'll compute it once we find the message - return SignalService.DataMessage.Quote.Type.NORMAL; - case Backups.Quote.Type.NORMAL: - case Backups.Quote.Type.UNKNOWN: - case null: - case undefined: - return SignalService.DataMessage.Quote.Type.NORMAL; - default: - throw missingCaseError(type); - } - } - - private async fromQuote( - quote: Backups.IQuote, - conversationId: string - ): Promise { + private async fromQuote(quote: Backups.IQuote): Promise { strictAssert(quote.authorId != null, 'quote must have an authorId'); const authorConvo = this.recipientIdToConvo.get(quote.authorId.toNumber()); @@ -1717,32 +1679,28 @@ export class BackupImportStream extends Writable { 'must have ACI for authorId in quote' ); - return copyFromQuotedMessage( - { - id: getTimestampFromLong(quote.targetSentTimestamp), - authorAci: authorConvo.serviceId, - text: dropNull(quote.text?.body), - bodyRanges: this.fromBodyRanges(quote.text), - attachments: - quote.attachments?.map(quotedAttachment => { - const { fileName, contentType, thumbnail } = quotedAttachment; - return { - fileName: dropNull(fileName), - contentType: contentType - ? stringToMIMEType(contentType) - : APPLICATION_OCTET_STREAM, - thumbnail: thumbnail?.pointer - ? convertFilePointerToAttachment(thumbnail.pointer) - : undefined, - }; - }) ?? [], - type: this.convertQuoteType(quote.type), - }, - conversationId, - { - messageCache: this.recentMessages, - } - ); + return { + id: getTimestampFromLong(quote.targetSentTimestamp) || null, + referencedMessageNotFound: quote.targetSentTimestamp == null, + authorAci: authorConvo.serviceId, + text: dropNull(quote.text?.body), + bodyRanges: this.fromBodyRanges(quote.text), + isGiftBadge: quote.type === Backups.Quote.Type.GIFT_BADGE, + isViewOnce: quote.type === Backups.Quote.Type.VIEW_ONCE, + attachments: + quote.attachments?.map(quotedAttachment => { + const { fileName, contentType, thumbnail } = quotedAttachment; + return { + fileName: dropNull(fileName), + contentType: contentType + ? stringToMIMEType(contentType) + : APPLICATION_OCTET_STREAM, + thumbnail: thumbnail?.pointer + ? convertFilePointerToAttachment(thumbnail.pointer) + : undefined, + }; + }) ?? [], + }; } private fromBodyRanges( diff --git a/ts/services/backups/util/CircularMessageCache.ts b/ts/services/backups/util/CircularMessageCache.ts deleted file mode 100644 index 7bff2f77e487..000000000000 --- a/ts/services/backups/util/CircularMessageCache.ts +++ /dev/null @@ -1,80 +0,0 @@ -// Copyright 2024 Signal Messenger, LLC -// SPDX-License-Identifier: AGPL-3.0-only - -import type { - ReadonlyMessageAttributesType, - MessageAttributesType, -} from '../../../model-types.d'; -import { find } from '../../../util/iterables'; -import { DataReader } from '../../../sql/Client'; - -export type CircularMessageCacheOptionsType = Readonly<{ - size: number; - flush: () => Promise; -}>; - -export class CircularMessageCache { - private readonly flush: () => Promise; - private readonly buffer: Array; - private readonly sentAtToMessages = new Map< - number, - Set - >(); - private offset = 0; - - constructor({ size, flush }: CircularMessageCacheOptionsType) { - this.flush = flush; - this.buffer = new Array(size); - } - - public push(attributes: MessageAttributesType): void { - const stale = this.buffer[this.offset]; - this.buffer[this.offset] = attributes; - this.offset = (this.offset + 1) % this.buffer.length; - - let addedSet = this.sentAtToMessages.get(attributes.sent_at); - if (addedSet === undefined) { - addedSet = new Set(); - this.sentAtToMessages.set(attributes.sent_at, addedSet); - } - addedSet.add(attributes); - - if (stale === undefined) { - return; - } - - const staleSet = this.sentAtToMessages.get(stale.sent_at); - if (staleSet === undefined) { - return; - } - staleSet.delete(stale); - if (staleSet.size === 0) { - this.sentAtToMessages.delete(stale.sent_at); - } - } - - public async findBySentAt( - sentAt: number, - predicate: (attributes: ReadonlyMessageAttributesType) => boolean - ): Promise { - const set = this.sentAtToMessages.get(sentAt); - if (set !== undefined) { - const cached = find(set.values(), predicate); - if (cached != null) { - return cached; - } - } - - await this.flush(); - - const onDisk = await DataReader.getMessagesBySentAt(sentAt); - return onDisk.find(predicate); - } - - // Just a stub to conform with the interface - public async upgradeSchema( - attributes: MessageAttributesType - ): Promise { - return attributes; - } -} diff --git a/ts/test-electron/backup/CircularMessageCache_test.ts b/ts/test-electron/backup/CircularMessageCache_test.ts deleted file mode 100644 index c151074741c3..000000000000 --- a/ts/test-electron/backup/CircularMessageCache_test.ts +++ /dev/null @@ -1,74 +0,0 @@ -// Copyright 2024 Signal Messenger, LLC -// SPDX-License-Identifier: AGPL-3.0-only - -import { assert } from 'chai'; -import * as sinon from 'sinon'; - -import { generateAci } from '../../types/ServiceId'; -import { type MessageAttributesType } from '../../model-types.d'; -import { CircularMessageCache } from '../../services/backups/util/CircularMessageCache'; -import { DataWriter } from '../../sql/Client'; - -const OUR_ACI = generateAci(); - -function createMessage(sentAt: number): MessageAttributesType { - return { - sent_at: sentAt, - received_at: sentAt, - timestamp: sentAt, - - id: 'abc', - type: 'incoming' as const, - conversationId: 'cid', - }; -} - -describe('backup/attachments', () => { - let messageCache: CircularMessageCache; - let flush: sinon.SinonStub; - - beforeEach(async () => { - await DataWriter.removeAll(); - flush = sinon.stub(); - messageCache = new CircularMessageCache({ - size: 2, - flush, - }); - }); - - afterEach(async () => { - await DataWriter.removeAll(); - }); - - it('should return a cached message', async () => { - const message = createMessage(123); - messageCache.push(message); - - const found = await messageCache.findBySentAt(123, () => true); - sinon.assert.notCalled(flush); - assert.strictEqual(found, message); - }); - - it('should purge message from cache on overflow', async () => { - messageCache.push(createMessage(123)); - messageCache.push(createMessage(124)); - messageCache.push(createMessage(125)); - - const found = await messageCache.findBySentAt(123, () => true); - sinon.assert.calledOnce(flush); - assert.isUndefined(found); - }); - - it('should find message in the database', async () => { - const message = createMessage(123); - - await DataWriter.saveMessage(message, { - ourAci: OUR_ACI, - forceSave: true, - }); - - const found = await messageCache.findBySentAt(123, () => true); - sinon.assert.calledOnce(flush); - assert.deepStrictEqual(found, message); - }); -}); diff --git a/ts/test-electron/backup/attachments_test.ts b/ts/test-electron/backup/attachments_test.ts index 0d218f2bc1e5..61005a3a4eee 100644 --- a/ts/test-electron/backup/attachments_test.ts +++ b/ts/test-electron/backup/attachments_test.ts @@ -486,7 +486,6 @@ describe('backup/attachments', () => { isViewOnce: false, id: Date.now(), referencedMessageNotFound: false, - messageId: '', isGiftBadge: true, attachments: [{ thumbnail: attachment, contentType: VIDEO_MP4 }], }; @@ -502,7 +501,6 @@ describe('backup/attachments', () => { composeMessage(1, { quote: { ...quotedMessage, - referencedMessageNotFound: true, attachments: [ { thumbnail: omit(attachment, NON_ROUNDTRIPPED_FIELDS), @@ -524,7 +522,6 @@ describe('backup/attachments', () => { isViewOnce: false, id: Date.now(), referencedMessageNotFound: false, - messageId: '', isGiftBadge: true, attachments: [{ thumbnail: attachment, contentType: VIDEO_MP4 }], }; @@ -539,7 +536,6 @@ describe('backup/attachments', () => { composeMessage(1, { quote: { ...quotedMessage, - referencedMessageNotFound: true, attachments: [ { thumbnail: { @@ -575,7 +571,6 @@ describe('backup/attachments', () => { isViewOnce: false, id: existingMessageTimestamp, referencedMessageNotFound: false, - messageId: '', isGiftBadge: false, attachments: [{ thumbnail: quoteAttachment, contentType: VIDEO_MP4 }], }; @@ -636,7 +631,6 @@ describe('backup/attachments', () => { isViewOnce: false, id: originalMessage.timestamp, referencedMessageNotFound: false, - messageId: '', isGiftBadge: false, attachments: [ { diff --git a/ts/test-electron/backup/bubble_test.ts b/ts/test-electron/backup/bubble_test.ts index a87f6f59912d..a195b3475873 100644 --- a/ts/test-electron/backup/bubble_test.ts +++ b/ts/test-electron/backup/bubble_test.ts @@ -229,9 +229,80 @@ describe('backup/bubble messages', () => { ]); }); - it('roundtrips gift badge quote', async () => { - await symmetricRoundtripHarness([ - { + describe('quotes', () => { + it('roundtrips gift badge quote', async () => { + await symmetricRoundtripHarness([ + { + conversationId: contactA.id, + id: generateGuid(), + type: 'incoming', + received_at: 3, + received_at_ms: 3, + sent_at: 3, + sourceServiceId: CONTACT_A, + readStatus: ReadStatus.Unread, + seenStatus: SeenStatus.Unseen, + unidentifiedDeliveryReceived: true, + timestamp: 3, + giftBadge: { + id: undefined, + level: 100, + expiration: 1723248000000, + receiptCredentialPresentation: BADGE_RECEIPT, + state: GiftBadgeStates.Opened, + }, + }, + { + conversationId: contactA.id, + id: generateGuid(), + type: 'incoming', + received_at: 4, + received_at_ms: 4, + sent_at: 4, + sourceServiceId: CONTACT_A, + readStatus: ReadStatus.Unread, + seenStatus: SeenStatus.Unseen, + unidentifiedDeliveryReceived: true, + timestamp: 4, + quote: { + authorAci: CONTACT_A, + attachments: [], + id: 3, + isViewOnce: false, + isGiftBadge: true, + referencedMessageNotFound: false, + }, + }, + ]); + }); + it('roundtrips quote with referenced message found', async () => { + await symmetricRoundtripHarness([ + { + conversationId: contactA.id, + id: generateGuid(), + type: 'incoming', + received_at: 3, + received_at_ms: 3, + sent_at: 3, + sourceServiceId: CONTACT_A, + readStatus: ReadStatus.Unread, + seenStatus: SeenStatus.Unseen, + unidentifiedDeliveryReceived: true, + timestamp: 3, + quote: { + authorAci: CONTACT_A, + attachments: [], + id: 42, + isGiftBadge: false, + text: 'quote text', + isViewOnce: false, + referencedMessageNotFound: false, + }, + }, + ]); + }); + it('roundtrips quote without referenced message found', async () => { + const message = { conversationId: contactA.id, id: generateGuid(), type: 'incoming', @@ -243,37 +314,32 @@ describe('backup/bubble messages', () => { seenStatus: SeenStatus.Unseen, unidentifiedDeliveryReceived: true, timestamp: 3, - giftBadge: { - id: undefined, - level: 100, - expiration: 1723248000000, - receiptCredentialPresentation: BADGE_RECEIPT, - state: GiftBadgeStates.Opened, - }, - }, - { - conversationId: contactA.id, - id: generateGuid(), - type: 'incoming', - received_at: 4, - received_at_ms: 4, - sent_at: 4, - sourceServiceId: CONTACT_A, - readStatus: ReadStatus.Unread, - seenStatus: SeenStatus.Unseen, - unidentifiedDeliveryReceived: true, - timestamp: 4, quote: { authorAci: CONTACT_A, attachments: [], - id: 3, + id: 42, + text: 'quote text', isViewOnce: false, - isGiftBadge: true, - messageId: '', - referencedMessageNotFound: false, + isGiftBadge: false, + referencedMessageNotFound: true, }, - }, - ]); + } as const; + + await asymmetricRoundtripHarness( + [message], + [ + { + ...message, + quote: { + ...message.quote, + // id is removed during roundtrip + id: null, + }, + }, + ] + ); + }); + // TODO (DESKTOP-7899): Roundtrip view-once quotes }); it('roundtrips sealed/unsealed incoming message', async () => { diff --git a/ts/util/getQuoteBodyText.ts b/ts/util/getQuoteBodyText.ts index 7b9db2a13a72..6407bf6977ef 100644 --- a/ts/util/getQuoteBodyText.ts +++ b/ts/util/getQuoteBodyText.ts @@ -6,16 +6,18 @@ import * as EmbeddedContact from '../types/EmbeddedContact'; export function getQuoteBodyText( messageAttributes: ReadonlyMessageAttributesType, - id: number + id: number | null ): string | undefined { const storyReactionEmoji = messageAttributes.storyReaction?.emoji; - const { editHistory } = messageAttributes; - const editedMessage = - editHistory && editHistory.find(edit => edit.timestamp === id); + if (id != null) { + const { editHistory } = messageAttributes; + const editedMessage = + editHistory && editHistory.find(edit => edit.timestamp === id); - if (editedMessage && editedMessage.body) { - return editedMessage.body; + if (editedMessage && editedMessage.body) { + return editedMessage.body; + } } const { body, contact: embeddedContact } = messageAttributes; diff --git a/ts/util/makeQuote.ts b/ts/util/makeQuote.ts index f25733b3b5ba..6630d6b4506b 100644 --- a/ts/util/makeQuote.ts +++ b/ts/util/makeQuote.ts @@ -27,14 +27,7 @@ export async function makeQuote( strictAssert(contact, 'makeQuote: no contact'); - const { - attachments, - bodyRanges, - id: messageId, - payment, - preview, - sticker, - } = quotedMessage; + const { attachments, bodyRanges, payment, preview, sticker } = quotedMessage; const quoteId = getMessageSentTimestamp(quotedMessage, { log }); @@ -48,7 +41,6 @@ export async function makeQuote( id: quoteId, isViewOnce: isTapToView(quotedMessage), isGiftBadge: isGiftBadge(quotedMessage), - messageId, referencedMessageNotFound: false, text: getQuoteBodyText(quotedMessage, quoteId), };