From 661d5cb5b0d6c3aebb9a83ac4c5115d0411b3f39 Mon Sep 17 00:00:00 2001 From: Reto Brunner Date: Fri, 30 Dec 2022 16:06:46 +0100 Subject: [PATCH 1/3] messagestorage: remove implementation details from interface The interface should not contain things that aren't the API of the storage interface. Further, rename ISqliteMessageStorage to SearchableMessageStorage, as that's also an implementation detail. We'll never have a second sqlite backend, so the name seems strange. --- server/plugins/messageStorage/sqlite.ts | 10 +++------- server/plugins/messageStorage/types.d.ts | 6 ++---- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/server/plugins/messageStorage/sqlite.ts b/server/plugins/messageStorage/sqlite.ts index fb7d9c3e..698f7c82 100644 --- a/server/plugins/messageStorage/sqlite.ts +++ b/server/plugins/messageStorage/sqlite.ts @@ -8,11 +8,7 @@ import Msg, {Message} from "../../models/msg"; import Client from "../../client"; import Chan, {Channel} from "../../models/chan"; import Helper from "../../helper"; -import type { - SearchResponse, - SearchQuery, - SqliteMessageStorage as ISqliteMessageStorage, -} from "./types"; +import type {SearchResponse, SearchQuery, SearchableMessageStorage} from "./types"; import Network from "../../models/network"; // TODO; type @@ -49,11 +45,11 @@ class Deferred { } } -class SqliteMessageStorage implements ISqliteMessageStorage { - client: Client; +class SqliteMessageStorage implements SearchableMessageStorage { isEnabled: boolean; database!: Database; initDone: Deferred; + client: Client; constructor(client: Client) { this.client = client; diff --git a/server/plugins/messageStorage/types.d.ts b/server/plugins/messageStorage/types.d.ts index 6a038822..acaa041a 100644 --- a/server/plugins/messageStorage/types.d.ts +++ b/server/plugins/messageStorage/types.d.ts @@ -6,7 +6,6 @@ import {Network} from "../../models/network"; import Client from "../../client"; interface MessageStorage { - client: Client; isEnabled: boolean; enable(): Promise; @@ -38,7 +37,6 @@ export type SearchResponse = type SearchFunction = (query: SearchQuery) => Promise; -export interface SqliteMessageStorage extends MessageStorage { - database: Database; - search: SearchFunction | []; +export interface SearchableMessageStorage extends MessageStorage { + search: SearchFunction; } From 52b8a2a78e62dfdcdd2313e8c7e81a7b07f383e2 Mon Sep 17 00:00:00 2001 From: Reto Brunner Date: Fri, 30 Dec 2022 16:16:22 +0100 Subject: [PATCH 2/3] textStorage: rip out client instance We don't need the client, so there's no need to accept it. --- server/client.ts | 2 +- server/plugins/messageStorage/text.ts | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/server/client.ts b/server/client.ts index cac35532..1af16751 100644 --- a/server/client.ts +++ b/server/client.ts @@ -143,7 +143,7 @@ class Client { } if (Config.values.messageStorage.includes("text")) { - client.messageStorage.push(new TextFileMessageStorage(client)); + client.messageStorage.push(new TextFileMessageStorage(client.name)); } for (const messageStorage of client.messageStorage) { diff --git a/server/plugins/messageStorage/text.ts b/server/plugins/messageStorage/text.ts index 3190890c..51214347 100644 --- a/server/plugins/messageStorage/text.ts +++ b/server/plugins/messageStorage/text.ts @@ -5,17 +5,16 @@ import filenamify from "filenamify"; import Config from "../../config"; import {MessageStorage} from "./types"; -import Client from "../../client"; import Channel from "../../models/chan"; import {Message, MessageType} from "../../models/msg"; import Network from "../../models/network"; class TextFileMessageStorage implements MessageStorage { - client: Client; isEnabled: boolean; + username: string; - constructor(client: Client) { - this.client = client; + constructor(username: string) { + this.username = username; this.isEnabled = false; } @@ -36,7 +35,7 @@ class TextFileMessageStorage implements MessageStorage { const logPath = path.join( Config.getUserLogsPath(), - this.client.name, + this.username, TextFileMessageStorage.getNetworkFolderName(network) ); From 958a948456d1a0c3c97bb60e8759e8f9f5578ac8 Mon Sep 17 00:00:00 2001 From: Reto Brunner Date: Fri, 30 Dec 2022 16:40:12 +0100 Subject: [PATCH 3/3] sqlite: Remove client from sqlitestorage The only reason we accepted a client was that so we have access to the next message id when we need it. So let's accept an id provider function instead. --- server/client.ts | 2 +- server/models/chan.ts | 2 +- server/plugins/messageStorage/sqlite.ts | 23 ++++++++++------------- server/plugins/messageStorage/types.d.ts | 2 +- test/plugins/sqlite.ts | 16 ++++++++-------- 5 files changed, 21 insertions(+), 24 deletions(-) diff --git a/server/client.ts b/server/client.ts index 1af16751..33abf2ab 100644 --- a/server/client.ts +++ b/server/client.ts @@ -138,7 +138,7 @@ class Client { if (!Config.values.public && client.config.log) { if (Config.values.messageStorage.includes("sqlite")) { - client.messageProvider = new SqliteMessageStorage(client); + client.messageProvider = new SqliteMessageStorage(client.name); client.messageStorage.push(client.messageProvider); } diff --git a/server/models/chan.ts b/server/models/chan.ts index 1298353c..8fd68ed7 100644 --- a/server/models/chan.ts +++ b/server/models/chan.ts @@ -287,7 +287,7 @@ class Chan { } client.messageProvider - .getMessages(network, this) + .getMessages(network, this, () => client.idMsg++) .then((messages) => { if (messages.length === 0) { if (network.irc!.network.cap.isEnabled("znc.in/playback")) { diff --git a/server/plugins/messageStorage/sqlite.ts b/server/plugins/messageStorage/sqlite.ts index 698f7c82..7538da8e 100644 --- a/server/plugins/messageStorage/sqlite.ts +++ b/server/plugins/messageStorage/sqlite.ts @@ -5,7 +5,6 @@ import path from "path"; import fs from "fs/promises"; import Config from "../../config"; import Msg, {Message} from "../../models/msg"; -import Client from "../../client"; import Chan, {Channel} from "../../models/chan"; import Helper from "../../helper"; import type {SearchResponse, SearchQuery, SearchableMessageStorage} from "./types"; @@ -49,17 +48,17 @@ class SqliteMessageStorage implements SearchableMessageStorage { isEnabled: boolean; database!: Database; initDone: Deferred; - client: Client; + userName: string; - constructor(client: Client) { - this.client = client; + constructor(userName: string) { + this.userName = userName; this.isEnabled = false; this.initDone = new Deferred(); } async _enable() { const logsPath = Config.getUserLogsPath(); - const sqlitePath = path.join(logsPath, `${this.client.name}.sqlite3`); + const sqlitePath = path.join(logsPath, `${this.userName}.sqlite3`); try { await fs.mkdir(logsPath, {recursive: true}); @@ -186,13 +185,11 @@ class SqliteMessageStorage implements SearchableMessageStorage { ]); } - /** - * Load messages for given channel on a given network and resolve a promise with loaded messages. - * - * @param network Network - Network object where the channel is - * @param channel Channel - Channel object for which to load messages for - */ - async getMessages(network: Network, channel: Channel): Promise { + async getMessages( + network: Network, + channel: Channel, + nextID: () => number + ): Promise { await this.initDone.promise; if (!this.isEnabled || Config.values.maxHistory === 0) { @@ -215,7 +212,7 @@ class SqliteMessageStorage implements SearchableMessageStorage { msg.type = row.type; const newMsg = new Msg(msg); - newMsg.id = this.client.idMsg++; + newMsg.id = nextID(); return newMsg; }); diff --git a/server/plugins/messageStorage/types.d.ts b/server/plugins/messageStorage/types.d.ts index acaa041a..2ead5575 100644 --- a/server/plugins/messageStorage/types.d.ts +++ b/server/plugins/messageStorage/types.d.ts @@ -16,7 +16,7 @@ interface MessageStorage { deleteChannel(network: Network, channel: Channel): Promise; - getMessages(network: Network, channel: Channel): Promise; + getMessages(network: Network, channel: Channel, nextID: () => number): Promise; canProvideMessages(): boolean; } diff --git a/test/plugins/sqlite.ts b/test/plugins/sqlite.ts index 4476faf0..16c832f1 100644 --- a/test/plugins/sqlite.ts +++ b/test/plugins/sqlite.ts @@ -6,7 +6,6 @@ import util from "../util"; import Msg, {MessageType} from "../../server/models/msg"; import Config from "../../server/config"; import MessageStorage from "../../server/plugins/messageStorage/sqlite"; -import Client from "../../server/client"; describe("SQLite Message Storage", function () { // Increase timeout due to unpredictable I/O on CI services @@ -17,10 +16,7 @@ describe("SQLite Message Storage", function () { let store: MessageStorage; before(function (done) { - store = new MessageStorage({ - name: "testUser", - idMsg: 1, - } as Client); + store = new MessageStorage("testUser"); // Delete database file from previous test run if (fs.existsSync(expectedPath)) { @@ -47,7 +43,7 @@ describe("SQLite Message Storage", function () { it("should resolve an empty array when disabled", async function () { store.isEnabled = false; - const messages = await store.getMessages(null as any, null as any); + const messages = await store.getMessages(null as any, null as any, null as any); expect(messages).to.be.empty; store.isEnabled = true; }); @@ -106,13 +102,15 @@ describe("SQLite Message Storage", function () { }); it("should retrieve previously stored message", async function () { + let msgid = 0; const messages = await store.getMessages( { uuid: "this-is-a-network-guid", } as any, { name: "#thisisaCHANNEL", - } as any + } as any, + () => msgid++ ); expect(messages).to.have.lengthOf(1); const msg = messages[0]; @@ -138,9 +136,11 @@ describe("SQLite Message Storage", function () { ); } + let msgId = 0; const messages = await store.getMessages( {uuid: "retrieval-order-test-network"} as any, - {name: "#channel"} as any + {name: "#channel"} as any, + () => msgId++ ); expect(messages).to.have.lengthOf(2); expect(messages.map((i_1) => i_1.text)).to.deep.equal(["msg 198", "msg 199"]);