From 85f50d43a183a8ef03749908ce3b7c2888e9ec67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Fri, 30 Aug 2024 14:29:52 +0200 Subject: [PATCH] force the passing of deps to the database manager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/wise-forks-play.md | 3 +- packages/backend-common/api-report.md | 5 ++- .../backend-common/src/deprecated/index.ts | 29 ++++++++++++-- .../backend-defaults/api-report-database.md | 6 +-- .../database/DatabaseManager.test.ts | 40 +++++++++++-------- .../entrypoints/database/DatabaseManager.ts | 26 +++++++----- .../entrypoints/database/connectors/mysql.ts | 9 ++--- .../database/connectors/postgres.ts | 9 ++--- .../database/connectors/sqlite3.test.ts | 10 ++++- .../database/connectors/sqlite3.ts | 21 +++++----- .../database/databaseServiceFactory.ts | 5 ++- .../src/entrypoints/database/types.ts | 9 ++--- 12 files changed, 102 insertions(+), 70 deletions(-) diff --git a/.changeset/wise-forks-play.md b/.changeset/wise-forks-play.md index 84a6da9003..c5348a8add 100644 --- a/.changeset/wise-forks-play.md +++ b/.changeset/wise-forks-play.md @@ -6,5 +6,6 @@ **BREAKING**: Simplifications and cleanup as part of the Backend System 1.0 work. - The deprecated `dropDatabase` function has now been removed, without replacement. -- The deprecated `LegacyRootDatabaseService` type has now been removed. The implication is that you now need to pass deps to a `DatabaseManager` when constructing one. +- The deprecated `LegacyRootDatabaseService` type has now been removed. - The return type from `DatabaseManager.forPlugin` is now directly a `DatabaseService`, as arguably expected. +- `DatabaseManager.forPlugin` now requires the `deps` argument, with the logger and lifecycle services. diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index 2555ab6f5b..8a895dd037 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -157,7 +157,10 @@ export class DatabaseManager { // (undocumented) static fromConfig( config: Config, - options?: DatabaseManagerOptions, + options?: { + migrations?: DatabaseService['migrations']; + logger?: LoggerService; + }, ): DatabaseManager; } diff --git a/packages/backend-common/src/deprecated/index.ts b/packages/backend-common/src/deprecated/index.ts index d34b81aa92..3b4b518de1 100644 --- a/packages/backend-common/src/deprecated/index.ts +++ b/packages/backend-common/src/deprecated/index.ts @@ -46,6 +46,8 @@ import { isChildPath as _isChildPath, LifecycleService, PluginMetadataService, + DatabaseService, + LoggerService, } from '@backstage/backend-plugin-api'; export * from './hot'; @@ -168,14 +170,20 @@ export type CacheClientOptions = CacheServiceOptions; * @deprecated Use `DatabaseManager` from the `@backstage/backend-defaults` package instead */ export class DatabaseManager { - private constructor(private readonly _databaseManager: _DatabaseManager) {} + private constructor( + private readonly _databaseManager: _DatabaseManager, + private readonly logger?: LoggerService, + ) {} static fromConfig( config: Config, - options?: DatabaseManagerOptions, + options?: { + migrations?: DatabaseService['migrations']; + logger?: LoggerService; + }, ): DatabaseManager { const _databaseManager = _DatabaseManager.fromConfig(config, options); - return new DatabaseManager(_databaseManager); + return new DatabaseManager(_databaseManager, options?.logger); } forPlugin( @@ -184,7 +192,20 @@ export class DatabaseManager { | { lifecycle: LifecycleService; pluginMetadata: PluginMetadataService } | undefined, ): PluginDatabaseManager { - return this._databaseManager.forPlugin(pluginId, deps); + const logger: LoggerService = this.logger ?? { + debug() {}, + info() {}, + warn() {}, + error() {}, + child() { + return this; + }, + }; + const lifecycle: LifecycleService = deps?.lifecycle ?? { + addShutdownHook() {}, + addStartupHook() {}, + }; + return this._databaseManager.forPlugin(pluginId, { logger, lifecycle }); } } diff --git a/packages/backend-defaults/api-report-database.md b/packages/backend-defaults/api-report-database.md index 8f3857fe23..d9e9e9e287 100644 --- a/packages/backend-defaults/api-report-database.md +++ b/packages/backend-defaults/api-report-database.md @@ -6,7 +6,6 @@ import { DatabaseService } from '@backstage/backend-plugin-api'; import { LifecycleService } from '@backstage/backend-plugin-api'; import { LoggerService } from '@backstage/backend-plugin-api'; -import { PluginMetadataService } from '@backstage/backend-plugin-api'; import { RootConfigService } from '@backstage/backend-plugin-api'; import { ServiceFactory } from '@backstage/backend-plugin-api'; @@ -14,9 +13,9 @@ import { ServiceFactory } from '@backstage/backend-plugin-api'; export class DatabaseManager { forPlugin( pluginId: string, - deps?: { + deps: { + logger: LoggerService; lifecycle: LifecycleService; - pluginMetadata: PluginMetadataService; }, ): DatabaseService; static fromConfig( @@ -28,7 +27,6 @@ export class DatabaseManager { // @public export type DatabaseManagerOptions = { migrations?: DatabaseService['migrations']; - logger?: LoggerService; }; // @public diff --git a/packages/backend-defaults/src/entrypoints/database/DatabaseManager.test.ts b/packages/backend-defaults/src/entrypoints/database/DatabaseManager.test.ts index fca6080329..26c2d54231 100644 --- a/packages/backend-defaults/src/entrypoints/database/DatabaseManager.test.ts +++ b/packages/backend-defaults/src/entrypoints/database/DatabaseManager.test.ts @@ -17,12 +17,18 @@ import { ConfigReader } from '@backstage/config'; import { DatabaseManagerImpl } from './DatabaseManager'; import { Connector } from './types'; +import { mockServices } from '@backstage/backend-test-utils'; describe('DatabaseManagerImpl', () => { afterEach(() => { jest.clearAllMocks(); }); + const deps = { + logger: mockServices.logger.mock(), + lifecycle: mockServices.lifecycle.mock(), + }; + it('calls the right connector, only once per plugin id', async () => { const connector1 = { getClient: jest.fn(), @@ -41,19 +47,19 @@ describe('DatabaseManagerImpl', () => { }, ); - await impl.forPlugin('plugin1').getClient(); + await impl.forPlugin('plugin1', deps).getClient(); expect(connector1.getClient).toHaveBeenCalledTimes(1); - expect(connector1.getClient).toHaveBeenLastCalledWith('plugin1', undefined); + expect(connector1.getClient).toHaveBeenLastCalledWith('plugin1', deps); expect(connector2.getClient).toHaveBeenCalledTimes(0); - await impl.forPlugin('plugin1').getClient(); + await impl.forPlugin('plugin1', deps).getClient(); expect(connector1.getClient).toHaveBeenCalledTimes(1); - expect(connector1.getClient).toHaveBeenLastCalledWith('plugin1', undefined); + expect(connector1.getClient).toHaveBeenLastCalledWith('plugin1', deps); expect(connector2.getClient).toHaveBeenCalledTimes(0); - await impl.forPlugin('plugin2').getClient(); + await impl.forPlugin('plugin2', deps).getClient(); expect(connector1.getClient).toHaveBeenCalledTimes(2); - expect(connector1.getClient).toHaveBeenLastCalledWith('plugin2', undefined); + expect(connector1.getClient).toHaveBeenLastCalledWith('plugin2', deps); expect(connector2.getClient).toHaveBeenCalledTimes(0); }); @@ -80,16 +86,16 @@ describe('DatabaseManagerImpl', () => { }, ); - await impl.forPlugin('plugin1').getClient(); + await impl.forPlugin('plugin1', deps).getClient(); expect(connector1.getClient).toHaveBeenCalledTimes(1); - expect(connector1.getClient).toHaveBeenLastCalledWith('plugin1', undefined); + expect(connector1.getClient).toHaveBeenLastCalledWith('plugin1', deps); expect(connector2.getClient).toHaveBeenCalledTimes(0); - await impl.forPlugin('plugin2').getClient(); + await impl.forPlugin('plugin2', deps).getClient(); expect(connector1.getClient).toHaveBeenCalledTimes(1); - expect(connector1.getClient).toHaveBeenLastCalledWith('plugin1', undefined); + expect(connector1.getClient).toHaveBeenLastCalledWith('plugin1', deps); expect(connector2.getClient).toHaveBeenCalledTimes(1); - expect(connector2.getClient).toHaveBeenLastCalledWith('plugin2', undefined); + expect(connector2.getClient).toHaveBeenLastCalledWith('plugin2', deps); }); it('migration skip options take precedence over config', async () => { @@ -112,7 +118,7 @@ describe('DatabaseManagerImpl', () => { }, { migrations: { skip: false } }, ); - expect((await impl.forPlugin('plugin1')).migrations).toEqual({ + expect((await impl.forPlugin('plugin1', deps)).migrations).toEqual({ skip: false, }); @@ -120,7 +126,7 @@ describe('DatabaseManagerImpl', () => { pg: connector, }); - expect((await impl1.forPlugin('plugin1')).migrations).toEqual({ + expect((await impl1.forPlugin('plugin1', deps)).migrations).toEqual({ skip: false, }); }); @@ -142,10 +148,10 @@ describe('DatabaseManagerImpl', () => { }, ); - expect((await impl.forPlugin('plugin1')).migrations).toEqual({ + expect((await impl.forPlugin('plugin1', deps)).migrations).toEqual({ skip: true, }); - expect((await impl.forPlugin('plugin2')).migrations).toEqual({ + expect((await impl.forPlugin('plugin2', deps)).migrations).toEqual({ skip: false, }); @@ -163,10 +169,10 @@ describe('DatabaseManagerImpl', () => { pg: connector, }, ); - expect((await impl2.forPlugin('plugin1')).migrations).toEqual({ + expect((await impl2.forPlugin('plugin1', deps)).migrations).toEqual({ skip: false, }); - expect((await impl2.forPlugin('plugin2')).migrations).toEqual({ + expect((await impl2.forPlugin('plugin2', deps)).migrations).toEqual({ skip: true, }); }); diff --git a/packages/backend-defaults/src/entrypoints/database/DatabaseManager.ts b/packages/backend-defaults/src/entrypoints/database/DatabaseManager.ts index 1230280e07..46d4ee8cf7 100644 --- a/packages/backend-defaults/src/entrypoints/database/DatabaseManager.ts +++ b/packages/backend-defaults/src/entrypoints/database/DatabaseManager.ts @@ -18,7 +18,6 @@ import { DatabaseService, LifecycleService, LoggerService, - PluginMetadataService, RootConfigService, } from '@backstage/backend-plugin-api'; import { Config } from '@backstage/config'; @@ -43,7 +42,6 @@ function pluginPath(pluginId: string): string { */ export type DatabaseManagerOptions = { migrations?: DatabaseService['migrations']; - logger?: LoggerService; }; /** @@ -66,9 +64,9 @@ export class DatabaseManagerImpl { */ forPlugin( pluginId: string, - deps?: { + deps: { + logger: LoggerService; lifecycle: LifecycleService; - pluginMetadata: PluginMetadataService; }, ): PluginDatabaseManager { const client = this.getClientType(pluginId).client; @@ -128,9 +126,9 @@ export class DatabaseManagerImpl { private async getDatabase( pluginId: string, connector: Connector, - deps?: { + deps: { + logger: LoggerService; lifecycle: LifecycleService; - pluginMetadata: PluginMetadataService; }, ): Promise { if (this.databaseCache.has(pluginId)) { @@ -141,13 +139,19 @@ export class DatabaseManagerImpl { this.databaseCache.set(pluginId, clientPromise); if (process.env.NODE_ENV !== 'test') { - clientPromise.then(client => this.startKeepaliveLoop(pluginId, client)); + clientPromise.then(client => + this.startKeepaliveLoop(pluginId, client, deps.logger), + ); } return clientPromise; } - private startKeepaliveLoop(pluginId: string, client: Knex): void { + private startKeepaliveLoop( + pluginId: string, + client: Knex, + logger: LoggerService, + ): void { let lastKeepaliveFailed = false; setInterval(() => { @@ -160,7 +164,7 @@ export class DatabaseManagerImpl { (error: unknown) => { if (!lastKeepaliveFailed) { lastKeepaliveFailed = true; - this.options?.logger?.warn( + logger.warn( `Database keepalive failed for plugin ${pluginId}, ${stringifyError( error, )}`, @@ -225,9 +229,9 @@ export class DatabaseManager { */ forPlugin( pluginId: string, - deps?: { + deps: { + logger: LoggerService; lifecycle: LifecycleService; - pluginMetadata: PluginMetadataService; }, ): PluginDatabaseManager { return this.impl.forPlugin(pluginId, deps); diff --git a/packages/backend-defaults/src/entrypoints/database/connectors/mysql.ts b/packages/backend-defaults/src/entrypoints/database/connectors/mysql.ts index f06db5999d..d5bd88deef 100644 --- a/packages/backend-defaults/src/entrypoints/database/connectors/mysql.ts +++ b/packages/backend-defaults/src/entrypoints/database/connectors/mysql.ts @@ -14,10 +14,7 @@ * limitations under the License. */ -import { - LifecycleService, - PluginMetadataService, -} from '@backstage/backend-plugin-api'; +import { LifecycleService, LoggerService } from '@backstage/backend-plugin-api'; import { Config, ConfigReader } from '@backstage/config'; import { InputError } from '@backstage/errors'; import { JsonObject } from '@backstage/types'; @@ -253,9 +250,9 @@ export class MysqlConnector implements Connector { async getClient( pluginId: string, - _deps?: { + _deps: { + logger: LoggerService; lifecycle: LifecycleService; - pluginMetadata: PluginMetadataService; }, ): Promise { const pluginConfig = new ConfigReader( diff --git a/packages/backend-defaults/src/entrypoints/database/connectors/postgres.ts b/packages/backend-defaults/src/entrypoints/database/connectors/postgres.ts index 18418a271a..b5cfffd1f4 100644 --- a/packages/backend-defaults/src/entrypoints/database/connectors/postgres.ts +++ b/packages/backend-defaults/src/entrypoints/database/connectors/postgres.ts @@ -14,10 +14,7 @@ * limitations under the License. */ -import { - LifecycleService, - PluginMetadataService, -} from '@backstage/backend-plugin-api'; +import { LifecycleService, LoggerService } from '@backstage/backend-plugin-api'; import { Config, ConfigReader } from '@backstage/config'; import { ForwardedError } from '@backstage/errors'; import { JsonObject } from '@backstage/types'; @@ -261,9 +258,9 @@ export class PgConnector implements Connector { async getClient( pluginId: string, - _deps?: { + _deps: { + logger: LoggerService; lifecycle: LifecycleService; - pluginMetadata: PluginMetadataService; }, ): Promise { const pluginConfig = new ConfigReader( diff --git a/packages/backend-defaults/src/entrypoints/database/connectors/sqlite3.test.ts b/packages/backend-defaults/src/entrypoints/database/connectors/sqlite3.test.ts index 2bd517d373..769d402eda 100644 --- a/packages/backend-defaults/src/entrypoints/database/connectors/sqlite3.test.ts +++ b/packages/backend-defaults/src/entrypoints/database/connectors/sqlite3.test.ts @@ -20,11 +20,17 @@ import { buildSqliteDatabaseConfig, createSqliteDatabaseClient, } from './sqlite3'; +import { mockServices } from '@backstage/backend-test-utils'; describe('better-sqlite3', () => { const createConfig = (connection: any) => new ConfigReader({ client: 'better-sqlite3', connection }); + const deps = { + logger: mockServices.logger.mock(), + lifecycle: mockServices.lifecycle.mock(), + }; + describe('buildSqliteDatabaseConfig', () => { it('builds an in-memory connection', () => { expect(buildSqliteDatabaseConfig(createConfig(':memory:'))).toEqual({ @@ -90,7 +96,9 @@ describe('better-sqlite3', () => { describe('createSqliteDatabaseClient', () => { it('creates an in memory knex instance', () => { - expect(createSqliteDatabaseClient(createConfig(':memory:'))).toBeTruthy(); + expect( + createSqliteDatabaseClient('p', createConfig(':memory:'), deps), + ).toBeTruthy(); }); }); }); diff --git a/packages/backend-defaults/src/entrypoints/database/connectors/sqlite3.ts b/packages/backend-defaults/src/entrypoints/database/connectors/sqlite3.ts index 77e8303390..67dd9d975f 100644 --- a/packages/backend-defaults/src/entrypoints/database/connectors/sqlite3.ts +++ b/packages/backend-defaults/src/entrypoints/database/connectors/sqlite3.ts @@ -15,10 +15,7 @@ */ import { DevDataStore } from '@backstage/backend-dev-utils'; -import { - LifecycleService, - PluginMetadataService, -} from '@backstage/backend-plugin-api'; +import { LifecycleService, LoggerService } from '@backstage/backend-plugin-api'; import { Config, ConfigReader } from '@backstage/config'; import { JsonObject } from '@backstage/types'; import { ensureDirSync } from 'fs-extra'; @@ -35,12 +32,13 @@ import { mergeDatabaseConfig } from './mergeDatabaseConfig'; * @param overrides - Additional options to merge with the config */ export function createSqliteDatabaseClient( + pluginId: string, dbConfig: Config, - overrides?: Knex.Config, - deps?: { + deps: { + logger: LoggerService; lifecycle: LifecycleService; - pluginMetadata: PluginMetadataService; }, + overrides?: Knex.Config, ) { const knexConfig = buildSqliteDatabaseConfig(dbConfig, overrides); const connConfig = knexConfig.connection as Knex.Sqlite3ConnectionConfig; @@ -62,7 +60,7 @@ export function createSqliteDatabaseClient( const devStore = DevDataStore.get(); if (devStore) { - const dataKey = `sqlite3-db-${deps.pluginMetadata.getId()}`; + const dataKey = `sqlite3-db-${pluginId}`; const connectionLoader = async () => { // If seed data is available, use it tconnectionLoader restore the database @@ -180,9 +178,9 @@ export class Sqlite3Connector implements Connector { async getClient( pluginId: string, - deps?: { + deps: { + logger: LoggerService; lifecycle: LifecycleService; - pluginMetadata: PluginMetadataService; }, ): Promise { const pluginConfig = new ConfigReader( @@ -202,9 +200,10 @@ export class Sqlite3Connector implements Connector { ); const client = createSqliteDatabaseClient( + pluginId, pluginConfig, - databaseClientOverrides, deps, + databaseClientOverrides, ); return client; diff --git a/packages/backend-defaults/src/entrypoints/database/databaseServiceFactory.ts b/packages/backend-defaults/src/entrypoints/database/databaseServiceFactory.ts index 3bdf1790f3..167779e807 100644 --- a/packages/backend-defaults/src/entrypoints/database/databaseServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/database/databaseServiceFactory.ts @@ -35,6 +35,7 @@ export const databaseServiceFactory = createServiceFactory({ deps: { config: coreServices.rootConfig, lifecycle: coreServices.lifecycle, + logger: coreServices.logger, pluginMetadata: coreServices.pluginMetadata, }, async createRootContext({ config }) { @@ -48,10 +49,10 @@ export const databaseServiceFactory = createServiceFactory({ }), ); }, - async factory({ pluginMetadata, lifecycle }, databaseManager) { + async factory({ pluginMetadata, lifecycle, logger }, databaseManager) { return databaseManager.forPlugin(pluginMetadata.getId(), { - pluginMetadata, lifecycle, + logger, }); }, }); diff --git a/packages/backend-defaults/src/entrypoints/database/types.ts b/packages/backend-defaults/src/entrypoints/database/types.ts index 57b85ff3c1..beded85d65 100644 --- a/packages/backend-defaults/src/entrypoints/database/types.ts +++ b/packages/backend-defaults/src/entrypoints/database/types.ts @@ -14,10 +14,7 @@ * limitations under the License. */ -import { - LifecycleService, - PluginMetadataService, -} from '@backstage/backend-plugin-api'; +import { LifecycleService, LoggerService } from '@backstage/backend-plugin-api'; import { Knex } from 'knex'; export type { DatabaseService as PluginDatabaseManager } from '@backstage/backend-plugin-api'; @@ -25,9 +22,9 @@ export type { DatabaseService as PluginDatabaseManager } from '@backstage/backen export interface Connector { getClient( pluginId: string, - deps?: { + deps: { + logger: LoggerService; lifecycle: LifecycleService; - pluginMetadata: PluginMetadataService; }, ): Promise; }