Merge pull request #7822 from bomkamp/add-global-pg-plugin-schema-flag

Adding config prop `usePluginSchemas` to allow plugins using the `pg`…
2021-11-18 11:23:21 +01:00
parent 8d815522db e605194868
commit 0ebc659c17
11 changed files with 497 additions and 22 deletions
@@ -0,0 +1,5 @@
+---
+'@backstage/backend-common': patch
+---
+
+Adding config prop `pluginDivisionMode` to allow plugins using the `pg` client to create their own management schemas in the db. This allows `pg` client plugins to work in separate schemas in the same db.
@@ -69,6 +69,19 @@ export interface Config {
       * Defaults to true if unspecified.
       */
      ensureExists?: boolean;
+      /**
+       * How plugins databases are managed/divided in the provided database instance.
+       *
+       * `database` -> Plugins are each given their own database to manage their schemas/tables.
+       *
+       * `schema` -> Plugins will be given their own schema (in the specified/default database)
+       *             to manage their tables.
+       *
+       * NOTE: Currently only supported by the `pg` client.
+       *
+       * @default database
+       */
+      pluginDivisionMode?: 'database' | 'schema';
      /** Plugin specific database configuration and client override */
      plugin?: {
        [pluginId: string]: {
@@ -15,13 +15,18 @@
 */
 import { ConfigReader } from '@backstage/config';
 import { omit } from 'lodash';
-import { createDatabaseClient, ensureDatabaseExists } from './connection';
+import {
+  createDatabaseClient,
+  ensureDatabaseExists,
+  ensureSchemaExists,
+} from './connection';
 import { DatabaseManager } from './DatabaseManager';

 jest.mock('./connection', () => ({
  ...jest.requireActual('./connection'),
  createDatabaseClient: jest.fn(),
  ensureDatabaseExists: jest.fn(),
+  ensureSchemaExists: jest.fn(),
 }));

 describe('DatabaseManager', () => {
@@ -314,5 +319,191 @@ describe('DatabaseManager', () => {
        expect.stringContaining('userdbname'),
      );
    });
+
+    it('plugin sets schema override for pg client', async () => {
+      const overrideConfig = {
+        backend: {
+          database: {
+            client: 'pg',
+            pluginDivisionMode: 'schema',
+            connection: {
+              host: 'localhost',
+              user: 'foo',
+              password: 'bar',
+              database: 'foodb',
+            },
+          },
+        },
+      };
+      const testManager = DatabaseManager.fromConfig(
+        new ConfigReader(overrideConfig),
+      );
+      const pluginId = 'schemaoverride';
+      await testManager.forPlugin(pluginId).getClient();
+
+      const mockCalls = mocked(createDatabaseClient).mock.calls.splice(-1);
+      const [baseConfig, overrides] = mockCalls[0];
+
+      expect(baseConfig.get()).toMatchObject({
+        client: 'pg',
+        connection: config.backend.database.connection,
+      });
+
+      expect(overrides).toMatchObject({
+        searchPath: [pluginId],
+      });
+    });
+
+    it('plugin does not provide schema override for non pg client', async () => {
+      const testManager = DatabaseManager.fromConfig(
+        new ConfigReader({
+          backend: {
+            database: {
+              client: 'sqlite3',
+              pluginDivisionMode: 'schema',
+              connection: {
+                host: 'localhost',
+                user: 'foo',
+                password: 'bar',
+                database: 'foodb',
+              },
+            },
+          },
+        }),
+      );
+      const pluginId = 'any-plugin';
+      await testManager.forPlugin(pluginId).getClient();
+
+      const mockCalls = mocked(createDatabaseClient).mock.calls.splice(-1);
+      const [baseConfig, overrides] = mockCalls[0];
+
+      expect(baseConfig.get()).toMatchObject({
+        client: 'sqlite3',
+        connection: config.backend.database.connection,
+      });
+
+      expect(overrides).not.toHaveProperty('searchPath');
+    });
+
+    it('plugin does not provide schema override if pluginDivisionMode is set to database', async () => {
+      const testManager = DatabaseManager.fromConfig(
+        new ConfigReader({
+          backend: {
+            database: {
+              client: 'pg',
+              pluginDivisionMode: 'database',
+              connection: 'some-file-path',
+            },
+          },
+        }),
+      );
+
+      const pluginId = 'any-plugin';
+      await testManager.forPlugin(pluginId).getClient();
+
+      const mockCalls = mocked(createDatabaseClient).mock.calls.splice(-1);
+      const [_baseConfig, overrides] = mockCalls[0];
+
+      expect(overrides).not.toHaveProperty('searchPath');
+    });
+
+    it('plugin does not provide schema override if pluginDivisionMode is not set', async () => {
+      const testManager = DatabaseManager.fromConfig(
+        new ConfigReader({
+          backend: {
+            database: {
+              client: 'pg',
+              connection: {
+                host: 'localhost',
+                user: 'foo',
+                password: 'bar',
+                database: 'foodb',
+              },
+            },
+          },
+        }),
+      );
+
+      const pluginId = 'schemaoverride';
+      await testManager.forPlugin(pluginId).getClient();
+
+      const mockCalls = mocked(createDatabaseClient).mock.calls.splice(-1);
+      const [_baseConfig, overrides] = mockCalls[0];
+
+      expect(overrides).not.toHaveProperty('searchPath');
+    });
+
+    it('pluginDivisionMode ensures that each plugin schema exists', async () => {
+      const testManager = DatabaseManager.fromConfig(
+        new ConfigReader({
+          backend: {
+            database: {
+              client: 'pg',
+              pluginDivisionMode: 'schema',
+              connection: {
+                host: 'localhost',
+                user: 'foo',
+                password: 'bar',
+                database: 'foodb',
+              },
+            },
+          },
+        }),
+      );
+      const pluginId = 'testdbname';
+      await testManager.forPlugin(pluginId).getClient();
+
+      const mockCalls = mocked(ensureSchemaExists).mock.calls;
+      const [_, schemaName] = mockCalls[0];
+
+      expect(schemaName).toEqual('testdbname');
+    });
+
+    it('pluginDivisionMode allows connection overrides for plugins', async () => {
+      const testManager = DatabaseManager.fromConfig(
+        new ConfigReader({
+          backend: {
+            database: {
+              client: 'pg',
+              pluginDivisionMode: 'schema',
+              connection: {
+                host: 'localhost',
+                user: 'foo',
+                password: 'bar',
+                database: 'foodb',
+              },
+              plugin: {
+                testdbname: {
+                  connection: {
+                    database: 'database_name_overriden',
+                    host: 'newhost',
+                  },
+                },
+              },
+            },
+          },
+        }),
+      );
+      const pluginId = 'testdbname';
+      await testManager.forPlugin(pluginId).getClient();
+
+      const mockCalls = mocked(createDatabaseClient).mock.calls.splice(-1);
+      const [baseConfig, overrides] = mockCalls[0];
+
+      expect(baseConfig.get()).toMatchObject({
+        client: 'pg',
+        connection: {
+          database: 'database_name_overriden',
+          host: 'newhost',
+          user: 'foo',
+          password: 'bar',
+        },
+      });
+      expect(overrides).toHaveProperty('searchPath', ['testdbname']);
+      expect(overrides).toHaveProperty(
+        'connection.database',
+        'database_name_overriden',
+      );
+    });
  });
 });
@@ -19,12 +19,15 @@ import { omit } from 'lodash';
 import { Config, ConfigReader } from '@backstage/config';
 import { JsonObject } from '@backstage/types';
 import {
-  createDatabaseClient,
  createNameOverride,
  ensureDatabaseExists,
  normalizeConnection,
+  createSchemaOverride,
+  ensureSchemaExists,
+  createDatabaseClient,
 } from './connection';
 import { PluginDatabaseManager } from './types';
+import { mergeDatabaseConfig } from './config';

 /**
 * Provides a config lookup path for a plugin's config block.
@@ -80,14 +83,15 @@ export class DatabaseManager {
   * Provides the canonical database name for a given plugin.
   *
   * This method provides the effective database name which is determined using global
-   * and plugin specific database config. If no explicit database name is configured,
-   * this method will provide a generated name which is the pluginId prefixed with
-   * 'backstage_plugin_'.
+   * and plugin specific database config. If no explicit database name is configured
+   * and `pluginDivisionMode` is not `schema`, this method will provide a generated name
+   * which is the pluginId prefixed with 'backstage_plugin_'. If `pluginDivisionMode` is
+   * `schema`, it will fallback to using the default database for the knex instance.
   *
   * @param pluginId Lookup the database name for given plugin
   * @returns String representing the plugin's database name
   */
-  private getDatabaseName(pluginId: string): string {
+  private getDatabaseName(pluginId: string): string | undefined {
    const connection = this.getConnectionConfig(pluginId);

    if (this.getClientType(pluginId).client === 'sqlite3') {
@@ -96,11 +100,16 @@ export class DatabaseManager {
        (connection as Knex.Sqlite3ConnectionConfig)?.filename ?? ':memory:'
      );
    }
+
+    const databaseName = (connection as Knex.ConnectionConfig)?.database;
+
+    // `pluginDivisionMode` as `schema` should use overridden databaseName if supplied or fallback to default knex database
+    if (this.getPluginDivisionModeConfig() === 'schema') {
+      return databaseName;
+    }
+
    // all other supported databases should fallback to an auto-prefixed name
-    return (
-      (connection as Knex.ConnectionConfig)?.database ??
-      `${this.prefix}${pluginId}`
-    );
+    return databaseName ?? `${this.prefix}${pluginId}`;
  }

  /**
@@ -137,13 +146,18 @@ export class DatabaseManager {
    );
  }

+  private getPluginDivisionModeConfig(): string {
+    return this.config.getOptionalString('pluginDivisionMode') ?? 'database';
+  }
+
  /**
   * Provides a Knex connection plugin config by combining base and plugin config.
   *
   * This method provides a baseConfig for a plugin database connector. If the client type
   * has not been overridden, the global connection config will be included with plugin
   * specific config as the base. Values from the plugin connection take precedence over the
-   * base. Base database name is omitted for all supported databases excluding SQLite.
+   * base. Base database name is omitted for all supported databases excluding SQLite unless
+   * `pluginDivisionMode` is set to `schema`.
   */
  private getConnectionConfig(
    pluginId: string,
@@ -154,10 +168,13 @@ export class DatabaseManager {
      this.config.get('connection'),
      this.config.getString('client'),
    );
-    // As databases cannot be shared, the `database` property from the base connection
-    // is omitted. SQLite3's `filename` property is an exception as this is used as a
+    // 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
    // directory elsewhere so we preserve `filename`.
-    baseConnection = omit(baseConnection, 'database');
+    if (this.getPluginDivisionModeConfig() !== 'schema') {
+      baseConnection = omit(baseConnection, 'database');
+    }

    // get and normalize optional plugin specific database connection
    const connection = normalizeConnection(
@@ -188,6 +205,16 @@ export class DatabaseManager {
    };
  }

+  /**
+   * Provides a partial Knex.Config database schema override for a given plugin.
+   *
+   * @param pluginId Target plugin to get database schema override
+   * @returns Partial Knex.Config with database schema override
+   */
+  private getSchemaOverrides(pluginId: string): Knex.Config | undefined {
+    return createSchemaOverride(this.getClientType(pluginId).client, pluginId);
+  }
+
  /**
   * Provides a partial Knex.Config database name override for a given plugin.
   *
@@ -195,10 +222,10 @@ export class DatabaseManager {
   * @returns Partial Knex.Config with database name override
   */
  private getDatabaseOverrides(pluginId: string): Knex.Config {
-    return createNameOverride(
-      this.getClientType(pluginId).client,
-      this.getDatabaseName(pluginId),
-    );
+    const databaseName = this.getDatabaseName(pluginId);
+    return databaseName
+      ? createNameOverride(this.getClientType(pluginId).client, databaseName)
+      : {};
  }

  /**
@@ -212,8 +239,8 @@ export class DatabaseManager {
      this.getConfigForPlugin(pluginId) as JsonObject,
    );

-    if (this.getEnsureExistsConfig(pluginId)) {
-      const databaseName = this.getDatabaseName(pluginId);
+    const databaseName = this.getDatabaseName(pluginId);
+    if (databaseName && this.getEnsureExistsConfig(pluginId)) {
      try {
        await ensureDatabaseExists(pluginConfig, databaseName);
      } catch (error) {
@@ -223,9 +250,24 @@ export class DatabaseManager {
      }
    }

-    return createDatabaseClient(
-      pluginConfig,
+    let schemaOverrides;
+    if (this.getPluginDivisionModeConfig() === 'schema') {
+      try {
+        schemaOverrides = this.getSchemaOverrides(pluginId);
+        await ensureSchemaExists(pluginConfig, pluginId);
+      } catch (error) {
+        throw new Error(
+          `Failed to connect to the database to make sure that schema for plugin '${pluginId}' exists, ${error}`,
+        );
+      }
+    }
+
+    const databaseClientOverrides = mergeDatabaseConfig(
+      {},
      this.getDatabaseOverrides(pluginId),
+      schemaOverrides,
    );
+
+    return createDatabaseClient(pluginConfig, databaseClientOverrides);
  }
 }
@@ -18,8 +18,24 @@ import { ConfigReader } from '@backstage/config';
 import {
  createDatabaseClient,
  createNameOverride,
+  createSchemaOverride,
+  ensureSchemaExists,
  parseConnectionString,
 } from './connection';
+import { pgConnector } from './connectors';
+
+const mocked = (f: Function) => f as jest.Mock;
+
+jest.mock('./connectors', () => {
+  const connectors = jest.requireActual('./connectors');
+  return {
+    ...connectors,
+    pgConnector: {
+      ...connectors.pgConnector,
+      ensureSchemaExists: jest.fn(),
+    },
+  };
+});

 describe('database connection', () => {
  describe('createDatabaseClient', () => {
@@ -152,4 +168,63 @@ describe('database connection', () => {
      expect(() => parseConnectionString('sqlite://')).toThrow();
    });
  });
+
+  describe('createSchemaOverride', () => {
+    it('returns Knex config for postgres', () => {
+      expect(createSchemaOverride('pg', 'testpg')).toHaveProperty(
+        'searchPath',
+        ['testpg'],
+      );
+    });
+
+    it('throws error for sqlite', () => {
+      expect(createSchemaOverride('sqlite3', 'testsqlite')).toBeUndefined();
+    });
+
+    it('returns Knex config for mysql', () => {
+      expect(createSchemaOverride('mysql', 'testmysql')).toBeUndefined();
+    });
+
+    it('throws an error for unknown connection', () => {
+      expect(createSchemaOverride('unknown', 'testname')).toBeUndefined();
+    });
+  });
+
+  describe('ensureSchemaExists', () => {
+    it('returns sucessfully with pg client', async () => {
+      await ensureSchemaExists(
+        new ConfigReader({
+          client: 'pg',
+          schema: 'catalog',
+          connection: 'postgresql://testuser:testpass@acme:5432/userdbname',
+        }),
+        'catalog',
+      );
+
+      const mockCalls = mocked(
+        pgConnector.ensureSchemaExists as Function,
+      ).mock.calls.splice(-1);
+      const [baseConfig, schemaName] = mockCalls[0];
+
+      expect(baseConfig.get()).toMatchObject({
+        client: 'pg',
+        connection: 'postgresql://testuser:testpass@acme:5432/userdbname',
+      });
+
+      expect(schemaName).toEqual('catalog');
+    });
+
+    it('throws error for non pg client', () => {
+      return expect(
+        ensureSchemaExists(
+          new ConfigReader({
+            client: 'sqlite3',
+            schema: 'catalog',
+            connection: ':memory:',
+          }),
+          'catalog',
+        ),
+      ).resolves.toBeUndefined();
+    });
+  });
 });
@@ -82,6 +82,23 @@ export async function ensureDatabaseExists(
  );
 }

+/**
+ * Ensures that the given schemas all exist, creating them if they do not.
+ *
+ * @public
+ */
+export async function ensureSchemaExists(
+  dbConfig: Config,
+  ...schemas: Array<string>
+): Promise<void> {
+  const client: DatabaseClient = dbConfig.getString('client');
+
+  return await ConnectorMapping[client]?.ensureSchemaExists?.(
+    dbConfig,
+    ...schemas,
+  );
+}
+
 /**
 * Provides a Knex.Config object with the provided database name for a given client.
 */
@@ -99,6 +116,23 @@ export function createNameOverride(
  }
 }

+/**
+ * Provides a Knex.Config object with the provided database schema for a given client. Currently only supported by `pg`.
+ */
+export function createSchemaOverride(
+  client: string,
+  name: string,
+): Partial<Knex.Config | undefined> {
+  try {
+    return ConnectorMapping[client]?.createSchemaOverride?.(name);
+  } catch (e) {
+    throw new InputError(
+      `Unable to create database schema override for '${client}' connector`,
+      e,
+    );
+  }
+}
+
 /**
 * Parses a connection string for a given client and provides a connection config.
 */
@@ -0,0 +1,25 @@
+/*
+ * Copyright 2021 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import defaultSchemaOverride from './defaultSchemaOverride';
+
+describe('defaultNameOverride()', () => {
+  it('returns a partial knex static connection config with searchPath set to [schemaName]', () => {
+    const schemaName = 'schemaName';
+    expect(defaultSchemaOverride(schemaName)).toHaveProperty('searchPath', [
+      schemaName,
+    ]);
+  });
+});
@@ -0,0 +1,29 @@
+/*
+ * Copyright 2021 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import { Knex } from 'knex';
+
+/**
+ * Provides a partial knex config with schema name override.
+ *
+ * @param name schema name to get config override for
+ */
+export default function defaultSchemaOverride(
+  name: string,
+): Partial<Knex.Config> {
+  return {
+    searchPath: [name],
+  };
+}
@@ -76,6 +76,24 @@ describe('postgres', () => {
      });
    });

+    it('overrides the schema name', () => {
+      const mockConnection = {
+        ...createMockConnection(),
+        schema: 'schemaName',
+      };
+
+      expect(
+        buildPgDatabaseConfig(createConfig(mockConnection), {
+          searchPath: ['schemaName'],
+        }),
+      ).toEqual({
+        client: 'pg',
+        connection: mockConnection,
+        searchPath: ['schemaName'],
+        useNullAsDefault: true,
+      });
+    });
+
    it('adds additional config settings', () => {
      const mockConnection = createMockConnection();

@@ -21,6 +21,7 @@ import { ForwardedError } from '@backstage/errors';
 import { mergeDatabaseConfig } from '../config';
 import { DatabaseConnector } from '../types';
 import defaultNameOverride from './defaultNameOverride';
+import defaultSchemaOverride from './defaultSchemaOverride';

 /**
 * Creates a knex postgres database connection
@@ -135,6 +136,29 @@ export async function ensurePgDatabaseExists(
  }
 }

+/**
+ * Creates the missing Postgres schema if it does not exist
+ *
+ * @param dbConfig The database config
+ * @param schemas The name of the schemas to create
+ */
+export async function ensurePgSchemaExists(
+  dbConfig: Config,
+  ...schemas: Array<string>
+): Promise<void> {
+  const admin = createPgDatabaseClient(dbConfig);
+
+  try {
+    const ensureSchema = async (database: string) => {
+      await admin.raw(`CREATE SCHEMA IF NOT EXISTS ??`, [database]);
+    };
+
+    await Promise.all(schemas.map(ensureSchema));
+  } finally {
+    await admin.destroy();
+  }
+}
+
 /**
 * PostgreSQL database connector.
 *
@@ -143,6 +167,8 @@ export async function ensurePgDatabaseExists(
 export const pgConnector: DatabaseConnector = Object.freeze({
  createClient: createPgDatabaseClient,
  ensureDatabaseExists: ensurePgDatabaseExists,
+  ensureSchemaExists: ensurePgSchemaExists,
  createNameOverride: defaultNameOverride,
+  createSchemaOverride: defaultSchemaOverride,
  parseConnectionString: parsePgConnectionString,
 });
@@ -45,6 +45,11 @@ export interface DatabaseConnector {
   * database name.
   */
  createNameOverride(name: string): Partial<Knex.Config>;
+  /**
+   * createSchemaOverride provides a partial knex config sufficient to override a
+   * PostgreSQL schema name within utilizing the `searchPath` knex configuration.
+   */
+  createSchemaOverride?(name: string): Partial<Knex.Config>;
  /**
   * parseConnectionString produces a knex connection config object representing
   * a database connection string.
@@ -64,4 +69,16 @@ export interface DatabaseConnector {
    dbConfig: Config,
    ...databases: Array<string>
  ): Promise<void>;
+
+  /**
+   * ensureSchemaExists performs a side-effect to ensure schema names passed in are
+   * present.
+   *
+   * Calling this function on schemas which already exist should do nothing.
+   * Missing schemas should be created if needed.
+   */
+  ensureSchemaExists?(
+    dbConfig: Config,
+    ...schemas: Array<string>
+  ): Promise<void>;
 }