Skip to content

Commit

Permalink
Delete reactions when their parent message is deleted
Browse files Browse the repository at this point in the history 
Co-authored-by: Scott Nonnenberg <scott@signal.org>
  • Loading branch information
@automated-signal @scottnonnenberg-signal
automated-signal and scottnonnenberg-signal committed Oct 15, 2021
1 parent 5cf9bba commit a2fe191
Show file tree
Hide file tree
Showing 4 changed files with 185 additions and 8 deletions.
5 changes: 5 additions & 0 deletions ts/sql/Client.ts
View file
Expand Up @@ -221,6 +221,7 @@ const dataInterface: ClientInterface = {
markReactionAsRead,
removeReactionFromConversation,
addReaction,
_getAllReactions,

getMessageBySender,
getMessageById,
Expand Down Expand Up @@ -1258,6 +1259,10 @@ async function addReaction(reactionObj: ReactionType) {
return channels.addReaction(reactionObj);
}

async function _getAllReactions() {
return channels._getAllReactions();
}

function handleMessageJSON(messages: Array<MessageTypeUnhydrated>) {
return messages.map(message => JSON.parse(message.json));
}
Expand Down
1 change: 1 addition & 0 deletions ts/sql/Interface.ts
View file
Expand Up @@ -389,6 +389,7 @@ export type DataInterface = {
targetTimestamp: number;
}) => Promise<void>;
addReaction: (reactionObj: ReactionType) => Promise<void>;
_getAllReactions: () => Promise<Array<ReactionType>>;

getUnprocessedCount: () => Promise<number>;
getAllUnprocessed: () => Promise<Array<UnprocessedType>>;
Expand Down
83 changes: 79 additions & 4 deletions ts/sql/Server.ts
View file
Expand Up @@ -203,6 +203,7 @@ const dataInterface: ServerInterface = {
markReactionAsRead,
addReaction,
removeReactionFromConversation,
_getAllReactions,
getMessageBySender,
getMessageById,
getMessagesById,
Expand Down Expand Up @@ -2530,6 +2531,71 @@ function updateToSchemaVersion41(currentVersion: number, db: Database) {
logger.info('updateToSchemaVersion41: success!');
}

function updateToSchemaVersion42(currentVersion: number, db: Database) {
if (currentVersion >= 42) {
return;
}

db.transaction(() => {
// First, recreate messages table delete trigger with reaction support

db.exec(`
DROP TRIGGER messages_on_delete;
CREATE TRIGGER messages_on_delete AFTER DELETE ON messages BEGIN
DELETE FROM messages_fts WHERE rowid = old.rowid;
DELETE FROM sendLogPayloads WHERE id IN (
SELECT payloadId FROM sendLogMessageIds
WHERE messageId = old.id
);
DELETE FROM reactions WHERE rowid IN (
SELECT rowid FROM reactions
WHERE messageId = old.id
);
END;
`);

// Then, delete previously-orphaned reactions

// Note: we use `pluck` here to fetch only the first column of
// returned row.
const messageIdList: Array<string> = db
.prepare('SELECT id FROM messages ORDER BY id ASC;')
.pluck()
.all();
const allReactions: Array<{
rowid: number;
messageId: string;
}> = db.prepare('SELECT rowid, messageId FROM reactions;').all();

const messageIds = new Set(messageIdList);
const reactionsToDelete: Array<number> = [];

allReactions.forEach(reaction => {
if (!messageIds.has(reaction.messageId)) {
reactionsToDelete.push(reaction.rowid);
}
});

function deleteReactions(rowids: Array<number>) {
db.prepare<ArrayQuery>(
`
DELETE FROM reactions
WHERE rowid IN ( ${rowids.map(() => '?').join(', ')} );
`
).run(rowids);
}

if (reactionsToDelete.length > 0) {
logger.info(`Deleting ${reactionsToDelete.length} orphaned reactions`);
batchMultiVarQuery(reactionsToDelete, deleteReactions, db);
}

db.pragma('user_version = 42');
})();
logger.info('updateToSchemaVersion42: success!');
}

export const SCHEMA_VERSIONS = [
updateToSchemaVersion1,
updateToSchemaVersion2,
Expand Down Expand Up @@ -2572,6 +2638,7 @@ export const SCHEMA_VERSIONS = [
updateToSchemaVersion39,
updateToSchemaVersion40,
updateToSchemaVersion41,
updateToSchemaVersion42,
];

export function updateSchema(db: Database) {
Expand Down Expand Up @@ -2809,19 +2876,22 @@ function getInstance(): Database {

function batchMultiVarQuery<ValueT>(
values: Array<ValueT>,
query: (batch: Array<ValueT>) => void
query: (batch: Array<ValueT>) => void,
providedDatabase?: Database
): [];
function batchMultiVarQuery<ValueT, ResultT>(
values: Array<ValueT>,
query: (batch: Array<ValueT>) => Array<ResultT>
query: (batch: Array<ValueT>) => Array<ResultT>,
providedDatabase?: Database
): Array<ResultT>;
function batchMultiVarQuery<ValueT, ResultT>(
values: Array<ValueT>,
query:
| ((batch: Array<ValueT>) => void)
| ((batch: Array<ValueT>) => Array<ResultT>)
| ((batch: Array<ValueT>) => Array<ResultT>),
providedDatabase?: Database
): Array<ResultT> {
const db = getInstance();
const db = providedDatabase || getInstance();
if (values.length > MAX_VARIABLE_COUNT) {
const result: Array<ResultT> = [];
db.transaction(() => {
Expand Down Expand Up @@ -4608,6 +4678,11 @@ async function removeReactionFromConversation({
});
}

async function _getAllReactions(): Promise<Array<ReactionType>> {
const db = getInstance();
return db.prepare<EmptyQuery>('SELECT * from reactions;').all();
}

async function getOlderMessagesByConversation(
conversationId: string,
{
Expand Down
104 changes: 100 additions & 4 deletions ts/test-node/sql_migrations_test.ts
View file
Expand Up @@ -7,10 +7,6 @@ import { v4 as generateGuid } from 'uuid';

import { SCHEMA_VERSIONS } from '../sql/Server';

const THEIR_UUID = generateGuid();
const THEIR_CONVO = generateGuid();
const ANOTHER_CONVO = generateGuid();
const THIRD_CONVO = generateGuid();
const OUR_UUID = generateGuid();

describe('SQL migrations test', () => {
Expand Down Expand Up @@ -87,6 +83,11 @@ describe('SQL migrations test', () => {
});

describe('updateToSchemaVersion41', () => {
const THEIR_UUID = generateGuid();
const THEIR_CONVO = generateGuid();
const ANOTHER_CONVO = generateGuid();
const THIRD_CONVO = generateGuid();

it('clears sessions and keys if UUID is not available', () => {
updateToVersion(40);

Expand Down Expand Up @@ -519,4 +520,99 @@ describe('SQL migrations test', () => {
);
});
});

describe('updateToSchemaVersion42', () => {
const MESSAGE_ID_1 = generateGuid();
const MESSAGE_ID_2 = generateGuid();
const MESSAGE_ID_3 = generateGuid();
const MESSAGE_ID_4 = generateGuid();
const CONVERSATION_ID = generateGuid();

it('deletes orphaned reactions', () => {
updateToVersion(41);

db.exec(
`
INSERT INTO messages
(id, conversationId, body)
VALUES
('${MESSAGE_ID_1}', '${CONVERSATION_ID}', 'message number 1'),
('${MESSAGE_ID_2}', '${CONVERSATION_ID}', 'message number 2');
INSERT INTO reactions (messageId, conversationId) VALUES
('${MESSAGE_ID_1}', '${CONVERSATION_ID}'),
('${MESSAGE_ID_2}', '${CONVERSATION_ID}'),
('${MESSAGE_ID_3}', '${CONVERSATION_ID}'),
('${MESSAGE_ID_4}', '${CONVERSATION_ID}');
`
);

const reactionCount = db
.prepare('SELECT COUNT(*) FROM reactions;')
.pluck();
const messageCount = db.prepare('SELECT COUNT(*) FROM messages;').pluck();

assert.strictEqual(reactionCount.get(), 4);
assert.strictEqual(messageCount.get(), 2);

updateToVersion(42);

assert.strictEqual(reactionCount.get(), 2);
assert.strictEqual(messageCount.get(), 2);

const reactionMessageIds = db
.prepare('SELECT messageId FROM reactions;')
.pluck()
.all();

assert.sameDeepMembers(reactionMessageIds, [MESSAGE_ID_1, MESSAGE_ID_2]);
});

it('new message delete trigger deletes reactions as well', () => {
updateToVersion(41);

db.exec(
`
INSERT INTO messages
(id, conversationId, body)
VALUES
('${MESSAGE_ID_1}', '${CONVERSATION_ID}', 'message number 1'),
('${MESSAGE_ID_2}', '${CONVERSATION_ID}', 'message number 2'),
('${MESSAGE_ID_3}', '${CONVERSATION_ID}', 'message number 3');
INSERT INTO reactions (messageId, conversationId) VALUES
('${MESSAGE_ID_1}', '${CONVERSATION_ID}'),
('${MESSAGE_ID_2}', '${CONVERSATION_ID}'),
('${MESSAGE_ID_3}', '${CONVERSATION_ID}');
`
);

const reactionCount = db
.prepare('SELECT COUNT(*) FROM reactions;')
.pluck();
const messageCount = db.prepare('SELECT COUNT(*) FROM messages;').pluck();

assert.strictEqual(reactionCount.get(), 3);
assert.strictEqual(messageCount.get(), 3);

updateToVersion(42);

assert.strictEqual(reactionCount.get(), 3);
assert.strictEqual(messageCount.get(), 3);

db.exec(
`
DELETE FROM messages WHERE id = '${MESSAGE_ID_1}';
`
);

assert.strictEqual(reactionCount.get(), 2);
assert.strictEqual(messageCount.get(), 2);

const reactionMessageIds = db
.prepare('SELECT messageId FROM reactions;')
.pluck()
.all();

assert.sameDeepMembers(reactionMessageIds, [MESSAGE_ID_2, MESSAGE_ID_3]);
});
});
});

0 comments on commit a2fe191

Please sign in to comment.