From 0a88d9ec01c15a249399480138104eee9ec5e6d6 Mon Sep 17 00:00:00 2001 From: James Robinson <1500092+AverageHelper@users.noreply.github.com> Date: Wed, 9 Mar 2022 19:01:25 -0700 Subject: [PATCH] Fix bulk deletion with very-large queues (#63) * Reword responses to some informational commands * Don't claim we cleared the queue if we failed to clear the queue * Respect Discord's bulk-delete limits * v1.3.2 --- package-lock.json | 24 ++++++-- package.json | 4 +- src/actions/messages/deleteMessage.test.ts | 64 ++++++++++++++++++++++ src/actions/messages/deleteMessage.ts | 15 ++++- src/commands/languages.ts | 2 +- src/commands/queue/restart.test.ts | 9 +++ src/commands/queue/restart.ts | 5 +- src/commands/version.ts | 6 +- 8 files changed, 117 insertions(+), 12 deletions(-) create mode 100644 src/actions/messages/deleteMessage.test.ts diff --git a/package-lock.json b/package-lock.json index 1f89d1fd..eaca7169 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "gamgee", - "version": "1.3.1", + "version": "1.3.2", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "gamgee", - "version": "1.3.1", + "version": "1.3.2", "license": "LICENSE", "dependencies": { "@averagehelper/job-queue": "^0.9.5", @@ -14,6 +14,7 @@ "discord.js": "^13.1.0", "dotenv": "^8.2.0", "humanize-duration": "^3.25.1", + "lodash": "^4.17.21", "node-fetch": "^2.6.6", "node-persist": "^3.1.0", "reflect-metadata": "^0.1.13", @@ -29,6 +30,7 @@ "devDependencies": { "@types/humanize-duration": "^3.18.1", "@types/jest": "^27.4.0", + "@types/lodash": "^4.14.179", "@types/node": "^16.6.2", "@types/node-fetch": "^2.5.12", "@types/node-persist": "^3.1.1", @@ -1607,6 +1609,12 @@ "integrity": "sha512-qcUXuemtEu+E5wZSJHNxUXeCZhAfXKQ41D+duX+VYPde7xyEVZci+/oXKJL13tnRs9lR2pr4fod59GT6/X1/yQ==", "dev": true }, + "node_modules/@types/lodash": { + "version": "4.14.179", + "resolved": "https://registry.npmjs.org/@types/lodash/-/lodash-4.14.179.tgz", + "integrity": "sha512-uwc1x90yCKqGcIOAT6DwOSuxnrAbpkdPsUOZtwrXb4D/6wZs+6qG7QnIawDuZWg0sWpxl+ltIKCaLoMlna678w==", + "dev": true + }, "node_modules/@types/node": { "version": "16.6.2", "resolved": "https://registry.npmjs.org/@types/node/-/node-16.6.2.tgz", @@ -7523,8 +7531,7 @@ "node_modules/lodash": { "version": "4.17.21", "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", - "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==", - "dev": true + "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==" }, "node_modules/lodash.clonedeep": { "version": "4.5.0", @@ -11762,6 +11769,12 @@ "integrity": "sha512-qcUXuemtEu+E5wZSJHNxUXeCZhAfXKQ41D+duX+VYPde7xyEVZci+/oXKJL13tnRs9lR2pr4fod59GT6/X1/yQ==", "dev": true }, + "@types/lodash": { + "version": "4.14.179", + "resolved": "https://registry.npmjs.org/@types/lodash/-/lodash-4.14.179.tgz", + "integrity": "sha512-uwc1x90yCKqGcIOAT6DwOSuxnrAbpkdPsUOZtwrXb4D/6wZs+6qG7QnIawDuZWg0sWpxl+ltIKCaLoMlna678w==", + "dev": true + }, "@types/node": { "version": "16.6.2", "resolved": "https://registry.npmjs.org/@types/node/-/node-16.6.2.tgz", @@ -16174,8 +16187,7 @@ "lodash": { "version": "4.17.21", "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", - "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==", - "dev": true + "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==" }, "lodash.clonedeep": { "version": "4.5.0", diff --git a/package.json b/package.json index 507d8277..18237c14 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gamgee", - "version": "1.3.1", + "version": "1.3.2", "description": "A Discord bot for managing a song request queue.", "private": true, "scripts": { @@ -47,6 +47,7 @@ "discord.js": "^13.1.0", "dotenv": "^8.2.0", "humanize-duration": "^3.25.1", + "lodash": "^4.17.21", "node-fetch": "^2.6.6", "node-persist": "^3.1.0", "reflect-metadata": "^0.1.13", @@ -62,6 +63,7 @@ "devDependencies": { "@types/humanize-duration": "^3.18.1", "@types/jest": "^27.4.0", + "@types/lodash": "^4.14.179", "@types/node": "^16.6.2", "@types/node-fetch": "^2.5.12", "@types/node-persist": "^3.1.1", diff --git a/src/actions/messages/deleteMessage.test.ts b/src/actions/messages/deleteMessage.test.ts new file mode 100644 index 00000000..39f95ef6 --- /dev/null +++ b/src/actions/messages/deleteMessage.test.ts @@ -0,0 +1,64 @@ +import type Discord from "discord.js"; +import { bulkDeleteMessagesWithIds } from "./deleteMessage.js"; + +const mockBulkDelete = jest.fn(); +const mockSingleDelete = jest.fn(); + +describe("Bulk Message Delete", () => { + let channel: Discord.TextChannel; + + beforeEach(() => { + channel = ({ + bulkDelete: mockBulkDelete, + messages: { + delete: mockSingleDelete + } + } as unknown) as Discord.TextChannel; + + mockBulkDelete.mockImplementation((ids: Array) => { + // Discord balks when we try <2 || >100 IDs + if (ids.length < 2 || ids.length > 100) { + return Promise.reject(new Error("Can't delete fewer than 2 or more than 100")); + } + return Promise.resolve(); + }); + }); + + test.each` + count | multiples | singles + ${2} | ${1} | ${0} + ${3} | ${1} | ${0} + ${45} | ${1} | ${0} + ${99} | ${1} | ${0} + ${100} | ${1} | ${0} + ${101} | ${1} | ${1} + ${800} | ${8} | ${0} + `( + "Clears when the queue has $count items in it", + async (params: { count: number; multiples: number; singles: number }) => { + const { count, multiples, singles } = params; + // Use an array of `count` instances of "a" + const messageIds = new Array(count).fill("a"); + await expect(bulkDeleteMessagesWithIds(messageIds, channel)).resolves.toBeTrue(); + + expect(mockBulkDelete).toHaveBeenCalledTimes(multiples); + expect(mockSingleDelete).toHaveBeenCalledTimes(singles); + } + ); + + test("Deletes one message individually", async () => { + const messageIds: Array = ["a"]; + await expect(bulkDeleteMessagesWithIds(messageIds, channel)).resolves.toBeTrue(); + + expect(mockBulkDelete).not.toHaveBeenCalled(); + expect(mockSingleDelete).toHaveBeenCalledTimes(1); + expect(mockSingleDelete).toHaveBeenCalledWith("a"); + }); + + test("Does nothing when the queue has 0 items in it", async () => { + const messageIds = new Array(); + await expect(bulkDeleteMessagesWithIds(messageIds, channel)).resolves.toBeTrue(); + + expect(mockBulkDelete).not.toHaveBeenCalled(); + }); +}); diff --git a/src/actions/messages/deleteMessage.ts b/src/actions/messages/deleteMessage.ts index 3a4c831a..82d5cec3 100644 --- a/src/actions/messages/deleteMessage.ts +++ b/src/actions/messages/deleteMessage.ts @@ -1,5 +1,6 @@ import type Discord from "discord.js"; import type { Snowflake } from "discord.js"; +import chunk from "lodash/chunk.js"; import isError from "../../helpers/isError.js"; import richErrorMessage from "../../helpers/richErrorMessage.js"; import { useLogger } from "../../logger.js"; @@ -68,8 +69,20 @@ export async function bulkDeleteMessagesWithIds( messageIds: Array, channel: Discord.TextChannel ): Promise { + if (messageIds.length === 0) return true; + try { - await channel.bulkDelete(messageIds); + // Batch up to 100 IDs at a time + for (const ids of chunk(messageIds, 100)) { + if (ids.length === 1) { + // Given one ID, just delete individually + const messageId = ids[0] as string; + await channel.messages.delete(messageId); + } else { + await channel.bulkDelete(ids); + } + } + return true; } catch (error: unknown) { if (isError(error) && error.code === "50034") { diff --git a/src/commands/languages.ts b/src/commands/languages.ts index e8ba174d..609eef91 100644 --- a/src/commands/languages.ts +++ b/src/commands/languages.ts @@ -51,7 +51,7 @@ const languages: GlobalCommand = { const stats = Object.entries(languages).map(([languageName, languageUse]) => { const use = (languageUse ?? 0) / totalUse; - return `${languageName} (${(use * 100).toFixed(1)}%)`; + return `${(use * 100).toFixed(1)}% ${languageName}`; }); logger.debug(`Gamgee is made of ${totalLanguages} languages: ${stats.join(", ")}`); diff --git a/src/commands/queue/restart.test.ts b/src/commands/queue/restart.test.ts index 223af5e6..66606c3b 100644 --- a/src/commands/queue/restart.test.ts +++ b/src/commands/queue/restart.test.ts @@ -37,6 +37,7 @@ describe("Clear queue contents", () => { mockGetAllEntries.mockResolvedValue([]); mockQueueClear.mockResolvedValue(undefined); + mockBulkDeleteMessagesWithIds.mockResolvedValue(true); }); test("does nothing when no queue is set up", async () => { @@ -87,4 +88,12 @@ describe("Clear queue contents", () => { expect(mockQueueClear).toHaveBeenCalledTimes(1); } ); + + test("Does not clear the database when clearing messages fails", async () => { + mockBulkDeleteMessagesWithIds.mockResolvedValueOnce(false); // something goes wrong + + await expect(restart.execute(context)).resolves.toBeUndefined(); // don't throw + + expect(mockQueueClear).not.toHaveBeenCalled(); // don't clear + }); }); diff --git a/src/commands/queue/restart.ts b/src/commands/queue/restart.ts index 02705728..1133511d 100644 --- a/src/commands/queue/restart.ts +++ b/src/commands/queue/restart.ts @@ -19,7 +19,10 @@ const restart: Subcommand = { await prepareForLongRunningTasks(); const toBeDeleted = (await fetchAllEntries(queueChannel)).map(entry => entry.queueMessageId); - await bulkDeleteMessagesWithIds(toBeDeleted, queueChannel); + const didDelete = await bulkDeleteMessagesWithIds(toBeDeleted, queueChannel); + if (!didDelete) { + return reply("Something went wrong. I couldn't get that queue cleared, sorry."); + } await clearEntries(queueChannel); return reply("The queue has restarted."); diff --git a/src/commands/version.ts b/src/commands/version.ts index 5de0afd7..e4c6e75c 100644 --- a/src/commands/version.ts +++ b/src/commands/version.ts @@ -18,9 +18,11 @@ const version: Command = { if (type === "interaction") { // Discord lets bots link stuff in Markdown syntax, but it'll also embed by default. Use `` to prevent the embed. - const repo = `https://github.com/AverageHelper/Gamgee/tree/v${gamgeeVersion}`; + const repo = "https://github.com/AverageHelper/Gamgee"; + const source = `${repo}/tree/v${gamgeeVersion}`; + const changelog = `${repo}/releases/tag/v${gamgeeVersion}`; return reply( - `I'm currently running Gamgee Core [v${gamgeeVersion}](<${repo}>) ${celebration}` + `I'm currently running [Gamgee Core v${gamgeeVersion}](<${source}>) ${celebration}\nYou can [read the changelog](<${changelog}>) on GitHub!` ); }