diff --git a/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts b/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts index 2963b4c9df..820865c350 100644 --- a/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts +++ b/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts @@ -18,23 +18,23 @@ import { BackgroundContext, Context, TransactionValue } from './Context'; import { DatabaseManager } from './database/DatabaseManager'; import { DefaultLocationStore } from './DefaultLocationStore'; -describe('DefaultLocationStore', () => { - const createLocationStore = async () => { - const knex = await DatabaseManager.createTestDatabaseConnection(); - const db = await DatabaseManager.createDatabase(knex); - const connection = { applyMutation: jest.fn() }; - const store = new DefaultLocationStore(knex); - await store.connect(connection); +const createLocationStore = async () => { + const knex = await DatabaseManager.createTestDatabaseConnection(); + const db = await DatabaseManager.createDatabase(knex); + const connection = { applyMutation: jest.fn() }; + const store = new DefaultLocationStore(knex); + await store.connect(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 }; + 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 }; +}; +describe('DefaultLocationStore', () => { it('should do a full sync with the locations on connect', async () => { const { connection } = await createLocationStore(); @@ -46,128 +46,105 @@ describe('DefaultLocationStore', () => { describe('listLocations', () => { it('lists empty locations when there is no locations', async () => { - expect.assertions(1); - - const { store, withContext } = await createLocationStore(); - await withContext(async ctx => { - expect(await store.listLocations(ctx)).toEqual([]); - }); + const { store } = await createLocationStore(); + expect(await store.listLocations()).toEqual([]); }); it('lists locations that are added to the db', async () => { - expect.assertions(2); - - 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 { store } = await createLocationStore(); + await store.createLocation({ + 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 () => { - expect.assertions(1); - - const { store, withContext } = await createLocationStore(); + const { store } = await createLocationStore(); const spec = { target: 'https://github.com/backstage/demo/blob/master/catalog-info.yml', type: 'url', }; - await withContext(async ctx => { - await store.createLocation(ctx, spec); - - await expect(() => store.createLocation(ctx, spec)).rejects.toThrow( - new RegExp(`Location ${spec.type}:${spec.target} already exists`), - ); - }); + await store.createLocation(spec); + await expect(() => store.createLocation(spec)).rejects.toThrow( + new RegExp(`Location ${spec.type}:${spec.target} already exists`), + ); }); it('calls apply mutation when adding a new location', async () => { expect.assertions(1); - const { store, connection, withContext } = await createLocationStore(); + const { store, connection } = await createLocationStore(); - await withContext(async ctx => { - await store.createLocation(ctx, { - target: - 'https://github.com/backstage/demo/blob/master/catalog-info.yml', - type: 'url', - }); + await store.createLocation({ + 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', - }, - }), - ]), - }); + 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 () => { - 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}`), - ); - }); + const { store } = await createLocationStore(); + const id = uuid(); + await expect(() => store.deleteLocation(id)).rejects.toThrow( + new RegExp(`Found no location with ID ${id}`), + ); }); it('calls apply mutation when adding a new location', async () => { - expect.assertions(1); + const { store, connection } = await createLocationStore(); - const { store, connection, withContext } = await createLocationStore(); + const location = await store.createLocation({ + target: + 'https://github.com/backstage/demo/blob/master/catalog-info.yml', + type: 'url', + }); - await withContext(async ctx => { - const location = await store.createLocation(ctx, { - target: - 'https://github.com/backstage/demo/blob/master/catalog-info.yml', - type: 'url', - }); + await store.deleteLocation(location.id); - 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', - }, - }), - ]), - }); + 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 a11cb9edf1..116ccb2c8e 100644 --- a/plugins/catalog-backend/src/next/DefaultLocationStore.ts +++ b/plugins/catalog-backend/src/next/DefaultLocationStore.ts @@ -23,7 +23,6 @@ import { import { v4 as uuid } from 'uuid'; import { locationSpecToLocationEntity } from './util'; import { ConflictError, NotFoundError } from '@backstage/errors'; -import { BackgroundContext, Context, TransactionValue } from './Context'; import { Knex } from 'knex'; type DbLocationsRow = { @@ -41,27 +40,31 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { return 'DefaultLocationStore'; } - 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`, + async createLocation(spec: LocationSpec): Promise { + const location = await this.db.transaction(async tx => { + // Attempt to find a previous location matching the spec + const previousLocations = await this.locations(tx); + // TODO: when location id's are a compilation of spec target we can remove this full + // lookup of locations first and just grab the by that instead. + 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`, + ); + } - const location: DbLocationsRow = { - id: uuid(), - type: spec.type, - target: spec.target, - }; - await tx('locations').insert(location); + const inner: DbLocationsRow = { + id: uuid(), + type: spec.type, + target: spec.target, + }; + + await tx('locations').insert(inner); + + return inner; + }); await this.connection.applyMutation({ type: 'delta', @@ -72,9 +75,12 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { return location; } - async listLocations(ctx: Context): Promise { - const tx = TransactionValue.from(ctx); - const locations = await tx('locations').select(); + async listLocations(): Promise { + return await this.locations(this.db); + } + + private async locations(dbOrTx: Knex | Knex.Transaction) { + const locations = await dbOrTx('locations').select(); return ( locations // TODO(blam): We should create a mutation to remove this location for everyone @@ -88,9 +94,10 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { ); } - async getLocation(ctx: Context, id: string): Promise { - const tx = TransactionValue.from(ctx); - const items = await tx('locations').where({ id }).select(); + async getLocation(id: string): Promise { + const items = await this.db('locations') + .where({ id }) + .select(); if (!items.length) { throw new NotFoundError(`Found no location with ID ${id}`); @@ -98,28 +105,28 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { return items[0]; } - async deleteLocation(ctx: Context, id: string): Promise { + async deleteLocation(id: string): Promise { if (!this.connection) { throw new Error('location store is not initialized'); } - const tx = TransactionValue.from(ctx); - const location = await this.getLocation(ctx, id); + const deleted = await this.db.transaction(async tx => { + const [location] = await tx('locations') + .where({ id }) + .select(); - const locations = await tx('locations') - .where({ id }) - .select(); + if (!location) { + throw new NotFoundError(`Found no location with ID ${id}`); + } - if (!locations.length) { - throw new NotFoundError(`Found no location with ID ${id}`); - } - - await tx('locations').where({ id }).del(); + await tx('locations').where({ id }).del(); + return location; + }); await this.connection.applyMutation({ type: 'delta', added: [], - removed: [locationSpecToLocationEntity(location)], + removed: [locationSpecToLocationEntity(deleted)], }); } @@ -134,12 +141,7 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { async connect(connection: EntityProviderConnection): Promise { this._connection = connection; - // 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 locations = await this.locations(this.db); const entities = locations.map(location => { return locationSpecToLocationEntity(location); diff --git a/plugins/catalog-backend/src/next/NextCatalogBuilder.ts b/plugins/catalog-backend/src/next/NextCatalogBuilder.ts index 0158cc9fc9..92f3822055 100644 --- a/plugins/catalog-backend/src/next/NextCatalogBuilder.ts +++ b/plugins/catalog-backend/src/next/NextCatalogBuilder.ts @@ -261,7 +261,7 @@ export class NextCatalogBuilder { }); const entitiesCatalog = new NextEntitiesCatalog(dbClient); - const locationStore = new DefaultLocationStore(db); + const locationStore = new DefaultLocationStore(dbClient); const stitcher = new Stitcher(dbClient, logger); const configLocationProvider = new ConfigLocationEntityProvider(config); const processingEngine = new DefaultCatalogProcessingEngine( diff --git a/plugins/catalog-backend/src/next/types.ts b/plugins/catalog-backend/src/next/types.ts index 9e7d7ced0a..410066eafd 100644 --- a/plugins/catalog-backend/src/next/types.ts +++ b/plugins/catalog-backend/src/next/types.ts @@ -21,7 +21,6 @@ import { EntityRelationSpec, } from '@backstage/catalog-model'; import { JsonObject } from '@backstage/config'; -import { Context } from './Context'; export interface LocationService { createLocation( @@ -34,10 +33,10 @@ export interface LocationService { } export interface LocationStore { - createLocation(ctx: Context, spec: LocationSpec): Promise; - listLocations(ctx: Context): Promise; - getLocation(ctx: Context, id: string): Promise; - deleteLocation(ctx: Context, id: string): Promise; + createLocation(spec: LocationSpec): Promise; + listLocations(): Promise; + getLocation(id: string): Promise; + deleteLocation(id: string): Promise; } export interface CatalogProcessingEngine {