From 189ba4c340a910e127345e01b3221b6d1e7eaf77 Mon Sep 17 00:00:00 2001 From: Joe Porpeglia Date: Fri, 3 Dec 2021 15:50:09 -0500 Subject: [PATCH 1/9] Overwrite sqlite filepath per plugin Signed-off-by: Joe Porpeglia --- .../src/database/DatabaseManager.test.ts | 5 ++++- .../src/database/DatabaseManager.ts | 12 +++++++--- .../src/database/connectors/sqlite3.test.ts | 22 ------------------- .../src/database/connectors/sqlite3.ts | 13 ----------- 4 files changed, 13 insertions(+), 39 deletions(-) diff --git a/packages/backend-common/src/database/DatabaseManager.test.ts b/packages/backend-common/src/database/DatabaseManager.test.ts index bd2e7b9f71..d5271bc2b5 100644 --- a/packages/backend-common/src/database/DatabaseManager.test.ts +++ b/packages/backend-common/src/database/DatabaseManager.test.ts @@ -282,7 +282,10 @@ describe('DatabaseManager', () => { expect(baseConfig.get().client).toEqual('sqlite3'); // sqlite3 uses 'filename' instead of 'database' - expect(overrides).toHaveProperty('connection.filename'); + expect(overrides).toHaveProperty( + 'connection.filename', + `plugin_with_different_client/${pluginId}`, + ); }); it('provides database client specific base from plugin connection string when client set under plugin', async () => { diff --git a/packages/backend-common/src/database/DatabaseManager.ts b/packages/backend-common/src/database/DatabaseManager.ts index 8e23a219ec..977516a18b 100644 --- a/packages/backend-common/src/database/DatabaseManager.ts +++ b/packages/backend-common/src/database/DatabaseManager.ts @@ -114,10 +114,16 @@ export class DatabaseManager { const connection = this.getConnectionConfig(pluginId); if (this.getClientType(pluginId).client === 'sqlite3') { + const sqliteFilename = (connection as Knex.Sqlite3ConnectionConfig) + ?.filename; + + // if persisting to a file, create separate files per plugin to avoid db migration issues. + if (sqliteFilename !== ':memory:') { + return `${sqliteFilename}/${pluginId}`; + } + // sqlite database name should fallback to ':memory:' as a special case - return ( - (connection as Knex.Sqlite3ConnectionConfig)?.filename ?? ':memory:' - ); + return ':memory:'; } const databaseName = (connection as Knex.ConnectionConfig)?.database; diff --git a/packages/backend-common/src/database/connectors/sqlite3.test.ts b/packages/backend-common/src/database/connectors/sqlite3.test.ts index b9da19d247..cfc9f61527 100644 --- a/packages/backend-common/src/database/connectors/sqlite3.test.ts +++ b/packages/backend-common/src/database/connectors/sqlite3.test.ts @@ -73,28 +73,6 @@ describe('sqlite3', () => { }); }); - it('builds a persistent connection per database', () => { - expect( - buildSqliteDatabaseConfig( - createConfig({ - filename: path.join('path', 'to', 'foo'), - }), - { - connection: { - database: 'my-database', - }, - }, - ), - ).toEqual({ - client: 'sqlite3', - connection: { - filename: path.join('path', 'to', 'foo', 'my-database.sqlite'), - database: 'my-database', - }, - useNullAsDefault: true, - }); - }); - it('replaces the connection with an override', () => { expect( buildSqliteDatabaseConfig(createConfig(':memory:'), { diff --git a/packages/backend-common/src/database/connectors/sqlite3.ts b/packages/backend-common/src/database/connectors/sqlite3.ts index 3dfecd7e6b..41f80ed293 100644 --- a/packages/backend-common/src/database/connectors/sqlite3.ts +++ b/packages/backend-common/src/database/connectors/sqlite3.ts @@ -86,19 +86,6 @@ export function buildSqliteDatabaseConfig( overrides, ); - // If we don't create an in-memory database, interpret the connection string - // as a directory that contains multiple sqlite files based on the database - // name. - const database = (config.connection as Knex.ConnectionConfig).database; - const sqliteConnection = config.connection as Knex.Sqlite3ConnectionConfig; - - if (database && sqliteConnection.filename !== ':memory:') { - sqliteConnection.filename = path.join( - sqliteConnection.filename, - `${database}.sqlite`, - ); - } - return config; } From e51369952ed91364d217ca560e113fe4b559a993 Mon Sep 17 00:00:00 2001 From: Joe Porpeglia Date: Fri, 3 Dec 2021 16:07:30 -0500 Subject: [PATCH 2/9] Add suffix to plugin sqlite db file Signed-off-by: Joe Porpeglia --- packages/backend-common/src/database/DatabaseManager.test.ts | 2 +- packages/backend-common/src/database/DatabaseManager.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend-common/src/database/DatabaseManager.test.ts b/packages/backend-common/src/database/DatabaseManager.test.ts index d5271bc2b5..2587a70423 100644 --- a/packages/backend-common/src/database/DatabaseManager.test.ts +++ b/packages/backend-common/src/database/DatabaseManager.test.ts @@ -284,7 +284,7 @@ describe('DatabaseManager', () => { // sqlite3 uses 'filename' instead of 'database' expect(overrides).toHaveProperty( 'connection.filename', - `plugin_with_different_client/${pluginId}`, + `plugin_with_different_client/${pluginId}.sqlite`, ); }); diff --git a/packages/backend-common/src/database/DatabaseManager.ts b/packages/backend-common/src/database/DatabaseManager.ts index 977516a18b..aee47aedc1 100644 --- a/packages/backend-common/src/database/DatabaseManager.ts +++ b/packages/backend-common/src/database/DatabaseManager.ts @@ -119,7 +119,7 @@ export class DatabaseManager { // if persisting to a file, create separate files per plugin to avoid db migration issues. if (sqliteFilename !== ':memory:') { - return `${sqliteFilename}/${pluginId}`; + return `${sqliteFilename}/${pluginId}.sqlite`; } // sqlite database name should fallback to ':memory:' as a special case From fe24bc9a323afd69fb44ab410b910ea84a1be057 Mon Sep 17 00:00:00 2001 From: Joe Porpeglia Date: Fri, 3 Dec 2021 16:14:55 -0500 Subject: [PATCH 3/9] Add changeset Signed-off-by: Joe Porpeglia --- .changeset/pink-ladybugs-share.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/pink-ladybugs-share.md diff --git a/.changeset/pink-ladybugs-share.md b/.changeset/pink-ladybugs-share.md new file mode 100644 index 0000000000..c8f2e8f206 --- /dev/null +++ b/.changeset/pink-ladybugs-share.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': minor +--- + +Each plugin now saves to a separate sqlite database file when `connection.filename` is provided in the sqlite config. From ef8392dab249014fe7a51e152bde8d4dc2e42e43 Mon Sep 17 00:00:00 2001 From: Joe Porpeglia Date: Mon, 6 Dec 2021 11:24:09 -0500 Subject: [PATCH 4/9] Use path.join Signed-off-by: Joe Porpeglia --- packages/backend-common/src/database/DatabaseManager.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/backend-common/src/database/DatabaseManager.ts b/packages/backend-common/src/database/DatabaseManager.ts index aee47aedc1..b026e83b4e 100644 --- a/packages/backend-common/src/database/DatabaseManager.ts +++ b/packages/backend-common/src/database/DatabaseManager.ts @@ -28,6 +28,7 @@ import { normalizeConnection, } from './connection'; import { PluginDatabaseManager } from './types'; +import path from 'path'; /** * Provides a config lookup path for a plugin's config block. @@ -119,7 +120,7 @@ export class DatabaseManager { // if persisting to a file, create separate files per plugin to avoid db migration issues. if (sqliteFilename !== ':memory:') { - return `${sqliteFilename}/${pluginId}.sqlite`; + return path.join(sqliteFilename, `${pluginId}.sqlite`); } // sqlite database name should fallback to ':memory:' as a special case From c2a478d7f948c63db8b2e3dc23e64090048eca4b Mon Sep 17 00:00:00 2001 From: Joe Porpeglia Date: Mon, 6 Dec 2021 11:26:10 -0500 Subject: [PATCH 5/9] Update changeset Signed-off-by: Joe Porpeglia --- .changeset/pink-ladybugs-share.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.changeset/pink-ladybugs-share.md b/.changeset/pink-ladybugs-share.md index c8f2e8f206..7edeb12f6a 100644 --- a/.changeset/pink-ladybugs-share.md +++ b/.changeset/pink-ladybugs-share.md @@ -1,5 +1,6 @@ --- -'@backstage/backend-common': minor +'@backstage/backend-common': path --- Each plugin now saves to a separate sqlite database file when `connection.filename` is provided in the sqlite config. +Any existing sqlite database files will be ignored. From e081edc783bd9da4594ae3619d2f5ad50ee0c4da Mon Sep 17 00:00:00 2001 From: Joe Porpeglia Date: Mon, 6 Dec 2021 11:34:31 -0500 Subject: [PATCH 6/9] Fix changset typo Signed-off-by: Joe Porpeglia --- .changeset/pink-ladybugs-share.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/pink-ladybugs-share.md b/.changeset/pink-ladybugs-share.md index 7edeb12f6a..83f5b8b78a 100644 --- a/.changeset/pink-ladybugs-share.md +++ b/.changeset/pink-ladybugs-share.md @@ -1,5 +1,5 @@ --- -'@backstage/backend-common': path +'@backstage/backend-common': patch --- Each plugin now saves to a separate sqlite database file when `connection.filename` is provided in the sqlite config. From cf192e1a724368230f97d4fa1a6b98cac6266638 Mon Sep 17 00:00:00 2001 From: Joe Porpeglia Date: Mon, 6 Dec 2021 12:18:50 -0500 Subject: [PATCH 7/9] Check if sqlite filename was provided before making plugin-specific filename Signed-off-by: Joe Porpeglia --- packages/backend-common/src/database/DatabaseManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-common/src/database/DatabaseManager.ts b/packages/backend-common/src/database/DatabaseManager.ts index b026e83b4e..a00a4991cb 100644 --- a/packages/backend-common/src/database/DatabaseManager.ts +++ b/packages/backend-common/src/database/DatabaseManager.ts @@ -119,7 +119,7 @@ export class DatabaseManager { ?.filename; // if persisting to a file, create separate files per plugin to avoid db migration issues. - if (sqliteFilename !== ':memory:') { + if (sqliteFilename && sqliteFilename !== ':memory:') { return path.join(sqliteFilename, `${pluginId}.sqlite`); } From d9a286bd56e3dd15b27f0b44daa148746ce3eea3 Mon Sep 17 00:00:00 2001 From: Joe Porpeglia Date: Wed, 8 Dec 2021 13:49:26 -0500 Subject: [PATCH 8/9] Use path.join for tests Signed-off-by: Joe Porpeglia --- packages/backend-common/src/database/DatabaseManager.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/backend-common/src/database/DatabaseManager.test.ts b/packages/backend-common/src/database/DatabaseManager.test.ts index 2587a70423..c72c740765 100644 --- a/packages/backend-common/src/database/DatabaseManager.test.ts +++ b/packages/backend-common/src/database/DatabaseManager.test.ts @@ -15,6 +15,7 @@ */ import { ConfigReader } from '@backstage/config'; import { omit } from 'lodash'; +import path from 'path'; import { createDatabaseClient, ensureDatabaseExists, @@ -284,7 +285,7 @@ describe('DatabaseManager', () => { // sqlite3 uses 'filename' instead of 'database' expect(overrides).toHaveProperty( 'connection.filename', - `plugin_with_different_client/${pluginId}.sqlite`, + path.join('plugin_with_different_client', `${pluginId}.sqlite`), ); }); From 63f5bc5f2cf699b0a479237331ad823dadd54ff5 Mon Sep 17 00:00:00 2001 From: Joe Porpeglia Date: Thu, 9 Dec 2021 17:59:56 -0500 Subject: [PATCH 9/9] Add connection.directory support for sqlite Signed-off-by: Joe Porpeglia --- .../src/database/DatabaseManager.test.ts | 128 ++++++++++++++---- .../src/database/DatabaseManager.ts | 27 +++- 2 files changed, 125 insertions(+), 30 deletions(-) diff --git a/packages/backend-common/src/database/DatabaseManager.test.ts b/packages/backend-common/src/database/DatabaseManager.test.ts index c72c740765..a48b1584aa 100644 --- a/packages/backend-common/src/database/DatabaseManager.test.ts +++ b/packages/backend-common/src/database/DatabaseManager.test.ts @@ -171,28 +171,6 @@ describe('DatabaseManager', () => { ); }); - it('uses top level sqlite database filename if plugin config is not present', async () => { - const testManager = DatabaseManager.fromConfig( - new ConfigReader({ - backend: { - database: { - client: 'sqlite3', - connection: 'some-file-path', - }, - }, - }), - ); - - await testManager.forPlugin('pluginwithoutconfig').getClient(); - const mockCalls = mocked(createDatabaseClient).mock.calls.splice(-1); - const [_, overrides] = mockCalls[0]; - - expect(overrides).toHaveProperty( - 'connection.filename', - expect.stringContaining('some-file-path'), - ); - }); - it('provides an inmemory sqlite database if top level is also inmemory and plugin config is not present', async () => { const testManager = DatabaseManager.fromConfig( new ConfigReader({ @@ -215,6 +193,110 @@ describe('DatabaseManager', () => { ); }); + it('throws if top level sqlite filename is provided', async () => { + const testManager = DatabaseManager.fromConfig( + new ConfigReader({ + backend: { + database: { + client: 'sqlite3', + connection: 'some-file-path', + }, + }, + }), + ); + + await expect( + testManager.forPlugin('pluginwithoutconfig').getClient(), + ).rejects.toBeInstanceOf(Error); + }); + + it('creates plugin-specific sqlite files when plugin config is not present', async () => { + const testManager = DatabaseManager.fromConfig( + new ConfigReader({ + backend: { + database: { + client: 'sqlite3', + connection: { + directory: 'sqlite-files', + }, + }, + }, + }), + ); + + await testManager.forPlugin('pluginwithoutconfig').getClient(); + const mockCalls = mocked(createDatabaseClient).mock.calls.splice(-1); + const [_, overrides] = mockCalls[0]; + + expect(overrides).toHaveProperty( + 'connection.filename', + path.join('sqlite-files', 'pluginwithoutconfig.sqlite'), + ); + }); + + it('uses sqlite directory from top level config and filename from plugin config', async () => { + const testManager = DatabaseManager.fromConfig( + new ConfigReader({ + backend: { + database: { + client: 'sqlite3', + connection: { + directory: 'sqlite-files', + }, + plugin: { + test: { + connection: { + filename: 'other.sqlite', + }, + }, + }, + }, + }, + }), + ); + + await testManager.forPlugin('test').getClient(); + const mockCalls = mocked(createDatabaseClient).mock.calls.splice(-1); + const [_, overrides] = mockCalls[0]; + + expect(overrides).toHaveProperty( + 'connection.filename', + path.join('sqlite-files', 'other.sqlite'), + ); + }); + + it('uses sqlite directory and filename from plugin config', async () => { + const testManager = DatabaseManager.fromConfig( + new ConfigReader({ + backend: { + database: { + client: 'sqlite3', + connection: { + directory: 'sqlite-files', + }, + plugin: { + test: { + connection: { + directory: 'custom-sqlite-files', + filename: 'other.sqlite', + }, + }, + }, + }, + }, + }), + ); + + await testManager.forPlugin('test').getClient(); + const mockCalls = mocked(createDatabaseClient).mock.calls.splice(-1); + const [_, overrides] = mockCalls[0]; + + expect(overrides).toHaveProperty( + 'connection.filename', + path.join('custom-sqlite-files', 'other.sqlite'), + ); + }); + it('connects to a plugin database using a specific database name', async () => { // testdbname.connection.database is set in config await manager.forPlugin('testdbname').getClient(); @@ -285,7 +367,7 @@ describe('DatabaseManager', () => { // sqlite3 uses 'filename' instead of 'database' expect(overrides).toHaveProperty( 'connection.filename', - path.join('plugin_with_different_client', `${pluginId}.sqlite`), + 'plugin_with_different_client', ); }); diff --git a/packages/backend-common/src/database/DatabaseManager.ts b/packages/backend-common/src/database/DatabaseManager.ts index a00a4991cb..5959c60c83 100644 --- a/packages/backend-common/src/database/DatabaseManager.ts +++ b/packages/backend-common/src/database/DatabaseManager.ts @@ -115,16 +115,18 @@ export class DatabaseManager { const connection = this.getConnectionConfig(pluginId); if (this.getClientType(pluginId).client === 'sqlite3') { - const sqliteFilename = (connection as Knex.Sqlite3ConnectionConfig) - ?.filename; + const sqliteFilename: string | undefined = ( + connection as Knex.Sqlite3ConnectionConfig + ).filename; - // if persisting to a file, create separate files per plugin to avoid db migration issues. - if (sqliteFilename && sqliteFilename !== ':memory:') { - return path.join(sqliteFilename, `${pluginId}.sqlite`); + if (sqliteFilename === ':memory:') { + return sqliteFilename; } - // sqlite database name should fallback to ':memory:' as a special case - return ':memory:'; + const sqliteDirectory = + (connection as { directory?: string }).directory ?? '.'; + + return path.join(sqliteDirectory, sqliteFilename ?? `${pluginId}.sqlite`); } const databaseName = (connection as Knex.ConnectionConfig)?.database; @@ -212,6 +214,17 @@ export class DatabaseManager { this.config.get('connection'), this.config.getString('client'), ); + + if ( + client === 'sqlite3' && + 'filename' in baseConnection && + baseConnection.filename !== ':memory:' + ) { + throw new Error( + '`connection.filename` is not supported for the base sqlite connection. Prefer `connection.directory` or provide a filename for the plugin connection instead.', + ); + } + // Databases cannot be shared unless the `pluginDivisionMode` is set to `schema`. The // `database` property from the base connection is omitted unless `pluginDivisionMode` // is set to `schema`. SQLite3's `filename` property is an exception as this is used as a