From a1e6ed1fccc2e061ba5e72d5ebfa384cb10b666f Mon Sep 17 00:00:00 2001 From: Johan Haals Date: Mon, 3 May 2021 13:41:27 +0200 Subject: [PATCH 1/4] 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 { From 86d1d7affac0134400978a254da2b617ad1a21a1 Mon Sep 17 00:00:00 2001 From: Johan Haals Date: Tue, 4 May 2021 16:16:21 +0200 Subject: [PATCH 2/4] Remove context from LocationStore Co-authored-by: Ben Lambert Signed-off-by: Johan Haals --- .../src/next/DefaultLocationStore.test.ts | 183 ++++++++---------- .../src/next/DefaultLocationStore.ts | 90 ++++----- .../src/next/NextCatalogBuilder.ts | 2 +- plugins/catalog-backend/src/next/types.ts | 9 +- 4 files changed, 131 insertions(+), 153 deletions(-) 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 { From 3492eccef2a4c0a062d3519f96d813bf9aea1a36 Mon Sep 17 00:00:00 2001 From: Johan Haals Date: Tue, 4 May 2021 16:19:55 +0200 Subject: [PATCH 3/4] Remove redundant assertion Signed-off-by: Johan Haals --- plugins/catalog-backend/src/next/DefaultLocationStore.test.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts b/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts index 820865c350..5a9da22287 100644 --- a/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts +++ b/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts @@ -87,10 +87,7 @@ describe('DefaultLocationStore', () => { }); it('calls apply mutation when adding a new location', async () => { - expect.assertions(1); - const { store, connection } = await createLocationStore(); - await store.createLocation({ target: 'https://github.com/backstage/demo/blob/master/catalog-info.yml', From ea6af512694bbc167377237ba44d28f989b28cc5 Mon Sep 17 00:00:00 2001 From: Johan Haals Date: Tue, 4 May 2021 16:28:39 +0200 Subject: [PATCH 4/4] chore: cleanups Co-authored-by: Ben Lambert Signed-off-by: Johan Haals --- .../src/next/DefaultLocationStore.test.ts | 11 ++---- .../src/next/DefaultLocationStore.ts | 34 +++++++++---------- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts b/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts index 5a9da22287..88806d0ace 100644 --- a/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts +++ b/plugins/catalog-backend/src/next/DefaultLocationStore.test.ts @@ -14,24 +14,17 @@ * 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'; const createLocationStore = async () => { const knex = await DatabaseManager.createTestDatabaseConnection(); - const db = await DatabaseManager.createDatabase(knex); + 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 }; + return { store, connection }; }; describe('DefaultLocationStore', () => { diff --git a/plugins/catalog-backend/src/next/DefaultLocationStore.ts b/plugins/catalog-backend/src/next/DefaultLocationStore.ts index 116ccb2c8e..fe1eb30467 100644 --- a/plugins/catalog-backend/src/next/DefaultLocationStore.ts +++ b/plugins/catalog-backend/src/next/DefaultLocationStore.ts @@ -76,22 +76,7 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { } 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 - // eventually when it's all done and dusted - .filter(({ type }) => type !== 'bootstrap') - .map(item => ({ - id: item.id, - target: item.target, - type: item.type, - })) - ); + return await this.locations(); } async getLocation(id: string): Promise { @@ -141,7 +126,7 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { async connect(connection: EntityProviderConnection): Promise { this._connection = connection; - const locations = await this.locations(this.db); + const locations = await this.locations(); const entities = locations.map(location => { return locationSpecToLocationEntity(location); @@ -152,4 +137,19 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { entities, }); } + + private async locations(dbOrTx: Knex.Transaction | Knex = this.db) { + const locations = await dbOrTx('locations').select(); + return ( + 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') + .map(item => ({ + id: item.id, + target: item.target, + type: item.type, + })) + ); + } }