From a1e6ed1fccc2e061ba5e72d5ebfa384cb10b666f Mon Sep 17 00:00:00 2001 From: Johan Haals Date: Mon, 3 May 2021 13:41:27 +0200 Subject: [PATCH] Refactor LocationStore to transaction from context Co-authored-by: Ben Lambert Signed-off-by: Johan Haals --- .../src/next/DefaultLocationStore.test.ts | 175 ++++++++++-------- .../src/next/DefaultLocationStore.ts | 117 +++++++----- plugins/catalog-backend/src/next/types.ts | 9 +- 3 files changed, 180 insertions(+), 121 deletions(-) diff --git a/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts b/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts index f9bb97477c..2963b4c9df 100644 --- a/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts +++ b/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts @@ -14,17 +14,25 @@ * limitations under the License. */ import { v4 as uuid } from 'uuid'; +import { BackgroundContext, Context, TransactionValue } from './Context'; import { DatabaseManager } from './database/DatabaseManager'; import { DefaultLocationStore } from './DefaultLocationStore'; -/* eslint-disable */ -xdescribe('DefaultLocationStore', () => { +describe('DefaultLocationStore', () => { const createLocationStore = async () => { - const db = await DatabaseManager.createTestDatabase(); + const knex = await DatabaseManager.createTestDatabaseConnection(); + const db = await DatabaseManager.createDatabase(knex); const connection = { applyMutation: jest.fn() }; - const store = new DefaultLocationStore(db); + const store = new DefaultLocationStore(knex); await store.connect(connection); - return { store, connection }; + + const withContext = async (handler: (ctx: Context) => Promise) => { + return await db.transaction(async tx => { + const ctx = TransactionValue.in(new BackgroundContext(), tx as any); + return await handler(ctx); + }); + }; + return { store, connection, db, withContext }; }; it('should do a full sync with the locations on connect', async () => { @@ -38,109 +46,128 @@ xdescribe('DefaultLocationStore', () => { describe('listLocations', () => { it('lists empty locations when there is no locations', async () => { - const { store } = await createLocationStore(); + expect.assertions(1); - expect(await store.listLocations()).toEqual([]); + const { store, withContext } = await createLocationStore(); + await withContext(async ctx => { + expect(await store.listLocations(ctx)).toEqual([]); + }); }); it('lists locations that are added to the db', async () => { - const { store } = await createLocationStore(); + expect.assertions(2); - await store.createLocation({ - target: - 'https://github.com/backstage/demo/blob/master/catalog-info.yml', - type: 'url', + const { store, withContext } = await createLocationStore(); + + await withContext(async ctx => { + await store.createLocation(ctx, { + target: + 'https://github.com/backstage/demo/blob/master/catalog-info.yml', + type: 'url', + }); + + const listLocations = await store.listLocations(ctx); + expect(listLocations).toHaveLength(1); + expect(listLocations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + target: + 'https://github.com/backstage/demo/blob/master/catalog-info.yml', + type: 'url', + }), + ]), + ); }); - - const listLocations = await store.listLocations(); - - expect(listLocations).toHaveLength(1); - expect(listLocations).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - target: - 'https://github.com/backstage/demo/blob/master/catalog-info.yml', - type: 'url', - }), - ]), - ); }); }); describe('createLocation', () => { it('throws when the location already exists', async () => { - const { store } = await createLocationStore(); + expect.assertions(1); + + const { store, withContext } = await createLocationStore(); const spec = { target: 'https://github.com/backstage/demo/blob/master/catalog-info.yml', type: 'url', }; - await store.createLocation(spec); + await withContext(async ctx => { + await store.createLocation(ctx, spec); - await expect(() => store.createLocation(spec)).rejects.toThrow( - new RegExp(`Location ${spec.type}:${spec.target} already exists`), - ); + await expect(() => store.createLocation(ctx, spec)).rejects.toThrow( + new RegExp(`Location ${spec.type}:${spec.target} already exists`), + ); + }); }); it('calls apply mutation when adding a new location', async () => { - const { store, connection } = await createLocationStore(); + expect.assertions(1); - await store.createLocation({ - target: - 'https://github.com/backstage/demo/blob/master/catalog-info.yml', - type: 'url', - }); + const { store, connection, withContext } = await createLocationStore(); - expect(connection.applyMutation).toHaveBeenCalledWith({ - type: 'delta', - removed: [], - added: expect.arrayContaining([ - expect.objectContaining({ - spec: { - target: - 'https://github.com/backstage/demo/blob/master/catalog-info.yml', - type: 'url', - }, - }), - ]), + await withContext(async ctx => { + await store.createLocation(ctx, { + target: + 'https://github.com/backstage/demo/blob/master/catalog-info.yml', + type: 'url', + }); + + expect(connection.applyMutation).toHaveBeenCalledWith({ + type: 'delta', + removed: [], + added: expect.arrayContaining([ + expect.objectContaining({ + spec: { + target: + 'https://github.com/backstage/demo/blob/master/catalog-info.yml', + type: 'url', + }, + }), + ]), + }); }); }); }); describe('deleteLocation', () => { it('throws if the location does not exist', async () => { - const { store } = await createLocationStore(); - - const id = uuid(); - - await expect(() => store.deleteLocation(id)).rejects.toThrow( - new RegExp(`Found no location with ID ${id}`), - ); + expect.assertions(1); + const { store, withContext } = await createLocationStore(); + await withContext(async ctx => { + const id = uuid(); + await expect(() => store.deleteLocation(ctx, id)).rejects.toThrow( + new RegExp(`Found no location with ID ${id}`), + ); + }); }); it('calls apply mutation when adding a new location', async () => { - const { store, connection } = await createLocationStore(); + expect.assertions(1); - const location = await store.createLocation({ - target: - 'https://github.com/backstage/demo/blob/master/catalog-info.yml', - type: 'url', - }); + const { store, connection, withContext } = await createLocationStore(); - await store.deleteLocation(location.id); + await withContext(async ctx => { + const location = await store.createLocation(ctx, { + target: + 'https://github.com/backstage/demo/blob/master/catalog-info.yml', + type: 'url', + }); - expect(connection.applyMutation).toHaveBeenCalledWith({ - type: 'delta', - added: [], - removed: expect.arrayContaining([ - expect.objectContaining({ - spec: { - target: - 'https://github.com/backstage/demo/blob/master/catalog-info.yml', - type: 'url', - }, - }), - ]), + await store.deleteLocation(ctx, location.id); + + expect(connection.applyMutation).toHaveBeenCalledWith({ + type: 'delta', + added: [], + removed: expect.arrayContaining([ + expect.objectContaining({ + spec: { + target: + 'https://github.com/backstage/demo/blob/master/catalog-info.yml', + type: 'url', + }, + }), + ]), + }); }); }); }); diff --git a/plugins/catalog-backend/src/next/DefaultLocationStore.ts b/plugins/catalog-backend/src/next/DefaultLocationStore.ts index f6267df6a5..a11cb9edf1 100644 --- a/plugins/catalog-backend/src/next/DefaultLocationStore.ts +++ b/plugins/catalog-backend/src/next/DefaultLocationStore.ts @@ -15,7 +15,6 @@ */ import { LocationSpec, Location } from '@backstage/catalog-model'; -import { Database } from '../database'; import { LocationStore, EntityProvider, @@ -23,52 +22,61 @@ import { } from './types'; import { v4 as uuid } from 'uuid'; import { locationSpecToLocationEntity } from './util'; -import { ConflictError } from '@backstage/errors'; +import { ConflictError, NotFoundError } from '@backstage/errors'; +import { BackgroundContext, Context, TransactionValue } from './Context'; +import { Knex } from 'knex'; + +type DbLocationsRow = { + id: string; + type: string; + target: string; +}; export class DefaultLocationStore implements LocationStore, EntityProvider { private _connection: EntityProviderConnection | undefined; - constructor(private readonly db: Database) {} + constructor(private readonly db: Knex) {} getProviderName(): string { return 'DefaultLocationStore'; } - async createLocation(spec: LocationSpec): Promise { - return this.db.transaction(async tx => { - // Attempt to find a previous location matching the spec - const previousLocations = await this.listLocations(); - const previousLocation = previousLocations.some( - l => spec.type === l.type && spec.target === l.target, + async createLocation(ctx: Context, spec: LocationSpec): Promise { + const tx = TransactionValue.from(ctx); + + // Attempt to find a previous location matching the spec + const previousLocations = await this.listLocations(ctx); + const previousLocation = previousLocations.some( + l => spec.type === l.type && spec.target === l.target, + ); + + if (previousLocation) { + throw new ConflictError( + `Location ${spec.type}:${spec.target} already exists`, ); + } - if (previousLocation) { - throw new ConflictError( - `Location ${spec.type}:${spec.target} already exists`, - ); - } + const location: DbLocationsRow = { + id: uuid(), + type: spec.type, + target: spec.target, + }; + await tx('locations').insert(location); - // TODO: id should really be type and target combined and not a uuid. - const location = await this.db.addLocation(tx, { - id: uuid(), - type: spec.type, - target: spec.target, - }); - - await this.connection.applyMutation({ - type: 'delta', - added: [locationSpecToLocationEntity(location)], - removed: [], - }); - - return location; + await this.connection.applyMutation({ + type: 'delta', + added: [locationSpecToLocationEntity(location)], + removed: [], }); + + return location; } - async listLocations(): Promise { - const dbLocations = await this.db.locations(); + async listLocations(ctx: Context): Promise { + const tx = TransactionValue.from(ctx); + const locations = await tx('locations').select(); return ( - dbLocations + locations // TODO(blam): We should create a mutation to remove this location for everyone // eventually when it's all done and dusted .filter(({ type }) => type !== 'bootstrap') @@ -80,23 +88,38 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { ); } - getLocation(id: string): Promise { - return this.db.location(id); + async getLocation(ctx: Context, id: string): Promise { + const tx = TransactionValue.from(ctx); + const items = await tx('locations').where({ id }).select(); + + if (!items.length) { + throw new NotFoundError(`Found no location with ID ${id}`); + } + return items[0]; } - deleteLocation(id: string): Promise { + async deleteLocation(ctx: Context, id: string): Promise { if (!this.connection) { throw new Error('location store is not initialized'); } - return this.db.transaction(async tx => { - const location = await this.db.location(id); - await this.db.removeLocation(tx, id); - await this.connection.applyMutation({ - type: 'delta', - added: [], - removed: [locationSpecToLocationEntity(location)], - }); + const tx = TransactionValue.from(ctx); + const location = await this.getLocation(ctx, id); + + const locations = await tx('locations') + .where({ id }) + .select(); + + if (!locations.length) { + throw new NotFoundError(`Found no location with ID ${id}`); + } + + await tx('locations').where({ id }).del(); + + await this.connection.applyMutation({ + type: 'delta', + added: [], + removed: [locationSpecToLocationEntity(location)], }); } @@ -110,10 +133,18 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { async connect(connection: EntityProviderConnection): Promise { this._connection = connection; - const locations = await this.listLocations(); + + // TODO: this feels a little weird creating the ctx in multiple places + // Let's work out a better way, maybe. + const locations = await this.db.transaction(tx => { + const ctx = TransactionValue.in(new BackgroundContext(), tx); + return this.listLocations(ctx); + }); + const entities = locations.map(location => { return locationSpecToLocationEntity(location); }); + await this.connection.applyMutation({ type: 'full', entities, diff --git a/plugins/catalog-backend/src/next/types.ts b/plugins/catalog-backend/src/next/types.ts index 410066eafd..9e7d7ced0a 100644 --- a/plugins/catalog-backend/src/next/types.ts +++ b/plugins/catalog-backend/src/next/types.ts @@ -21,6 +21,7 @@ import { EntityRelationSpec, } from '@backstage/catalog-model'; import { JsonObject } from '@backstage/config'; +import { Context } from './Context'; export interface LocationService { createLocation( @@ -33,10 +34,10 @@ export interface LocationService { } export interface LocationStore { - createLocation(spec: LocationSpec): Promise; - listLocations(): Promise; - getLocation(id: string): Promise; - deleteLocation(id: string): Promise; + createLocation(ctx: Context, spec: LocationSpec): Promise; + listLocations(ctx: Context): Promise; + getLocation(ctx: Context, id: string): Promise; + deleteLocation(ctx: Context, id: string): Promise; } export interface CatalogProcessingEngine {