Adding config prop usePluginSchemas 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.

Signed-off-by: Greg Bomkamp <Gregory.Bomkamp@libertymutual.com>
2021-10-28 10:43:22 -04:00
parent dd028ea861
commit 7ad9a07b27
11 changed files with 497 additions and 22 deletions
@@ -0,0 +1,5 @@
+---
+'@backstage/backend-common': patch
+---
+
+Adding config prop `usePluginSchemas` 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,16 @@ export interface Config {
       * Defaults to true if unspecified.
       */
      ensureExists?: boolean;
+      /**
+       * Whether plugins should use their own schemas instead of databases. If enabled,
+       * each plugin will create a schema in the configured database instance
+       * using the `pluginId` as its schema name.
+       *
+       * NOTE: Currently only supported by the `pg` client.
+       *
+       * @default false
+       */
+      usePluginSchemas?: boolean;
      /** 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',
+            usePluginSchemas: true,
+            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',
+              usePluginSchemas: true,
+              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 usePluginSchemas is not provided', async () => {
+      const testManager = DatabaseManager.fromConfig(
+        new ConfigReader({
+          backend: {
+            database: {
+              client: 'pg',
+              usePluginSchemas: false,
+              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 usePluginSchemas is false', 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('usePluginSchemas ensures that each plugin schema exists', async () => {
+      const testManager = DatabaseManager.fromConfig(
+        new ConfigReader({
+          backend: {
+            database: {
+              client: 'pg',
+              usePluginSchemas: true,
+              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('usePluginSchemas allows connection overrides for plugins', async () => {
+      const testManager = DatabaseManager.fromConfig(
+        new ConfigReader({
+          backend: {
+            database: {
+              client: 'pg',
+              usePluginSchemas: true,
+              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',
+      );
+    });
  });
 });
@@ -18,12 +18,15 @@ import { omit } from 'lodash';
 import { Config, ConfigReader } from '@backstage/config';
 import { JsonObject } from '@backstage/types';
 import {
-  createDatabaseClient,
  ensureDatabaseExists,
  createNameOverride,
  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.
@@ -79,14 +82,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 usePluginSchemas is not enabled, this method will provide a generated name
+   * which is the pluginId prefixed with 'backstage_plugin_'. If `usePluginSchemas` is
+   * enabled, 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') {
@@ -95,11 +99,16 @@ export class DatabaseManager {
        (connection as Knex.Sqlite3ConnectionConfig)?.filename ?? ':memory:'
      );
    }
+
+    const databaseName = (connection as Knex.ConnectionConfig)?.database;
+
+    // usePluginSchemas enabled should use overridden databaseName if supplied or fallback to default knex database
+    if (this.getUsePluginSchemasConfig()) {
+      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}`;
  }

  /**
@@ -136,13 +145,18 @@ export class DatabaseManager {
    );
  }

+  private getUsePluginSchemasConfig(): boolean {
+    return this.config.getOptionalBoolean('usePluginSchemas') ?? false;
+  }
+
  /**
   * 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
+   * `usePluginSchemas` is enabled.
   */
  private getConnectionConfig(
    pluginId: string,
@@ -153,10 +167,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 usePluginSchemas is enabled. The
+    // `database` property from the base connection is omitted unless usePluginSchemas
+    // is enabled. 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.getUsePluginSchemasConfig()) {
+      baseConnection = omit(baseConnection, 'database');
+    }

    // get and normalize optional plugin specific database connection
    const connection = normalizeConnection(
@@ -187,6 +204,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.
   *
@@ -194,10 +221,13 @@ 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 databaseNameOverride = this.getDatabaseName(pluginId);
+    return databaseNameOverride
+      ? createNameOverride(
+          this.getClientType(pluginId).client,
+          databaseNameOverride,
+        )
+      : {};
  }

  /**
@@ -211,8 +241,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) {
@@ -222,9 +252,24 @@ export class DatabaseManager {
      }
    }

-    return createDatabaseClient(
-      pluginConfig,
+    let schemaOverrides;
+    if (this.getUsePluginSchemasConfig()) {
+      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>;
 }