From 948052cbb4b692bb9a6ad444cc8f19431e763a12 Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Wed, 21 Oct 2020 09:34:43 +0200 Subject: [PATCH] feat: add ability to dry run adding a new location The location is now added in a transaction and afterwards rolled back. This allows users to dry run this operation to see if there entity has issues. This is probably done my automated tools. --- .changeset/angry-crabs-relate.md | 9 ++ .../features/software-catalog/installation.md | 1 + .../catalog/DatabaseEntitiesCatalog.test.ts | 46 +++++++-- .../src/catalog/DatabaseEntitiesCatalog.ts | 42 +++++--- .../src/catalog/DatabaseLocationsCatalog.ts | 18 +++- plugins/catalog-backend/src/catalog/types.ts | 14 ++- .../src/database/CommonDatabase.test.ts | 5 +- .../src/database/CommonDatabase.ts | 60 +++++++---- plugins/catalog-backend/src/database/index.ts | 1 + plugins/catalog-backend/src/database/types.ts | 33 +++++-- .../ingestion/HigherOrderOperations.test.ts | 70 ++++++++++++- .../src/ingestion/HigherOrderOperations.ts | 72 ++++++++------ .../catalog-backend/src/ingestion/types.ts | 5 +- .../src/service/CatalogBuilder.ts | 1 + .../src/service/router.test.ts | 99 +++++++++++++------ plugins/catalog-backend/src/service/router.ts | 40 ++++---- 16 files changed, 378 insertions(+), 138 deletions(-) create mode 100644 .changeset/angry-crabs-relate.md diff --git a/.changeset/angry-crabs-relate.md b/.changeset/angry-crabs-relate.md new file mode 100644 index 0000000000..66cdb29be5 --- /dev/null +++ b/.changeset/angry-crabs-relate.md @@ -0,0 +1,9 @@ +--- +'@backstage/plugin-catalog-backend': minor +--- + +Add ability to dry run adding a new location ot the catalog API. + +The location is now added in a transaction and afterwards rolled back. +This allows users to dry run this operation to see if there entity has issues. +This is probably done by automated tools in the CI/CD pipeline. diff --git a/docs/features/software-catalog/installation.md b/docs/features/software-catalog/installation.md index 26990033aa..1e037461b4 100644 --- a/docs/features/software-catalog/installation.md +++ b/docs/features/software-catalog/installation.md @@ -122,6 +122,7 @@ export default async function createPlugin({ entitiesCatalog, locationsCatalog, locationReader, + db, logger, ); diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts index 6407000b6f..7e751bab92 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts @@ -16,12 +16,13 @@ import { getVoidLogger } from '@backstage/backend-common'; import type { Entity } from '@backstage/catalog-model'; -import { Database, DatabaseManager } from '../database'; +import { Database, DatabaseManager, Transaction } from '../database'; import { DatabaseEntitiesCatalog } from './DatabaseEntitiesCatalog'; import { EntityUpsertRequest } from './types'; describe('DatabaseEntitiesCatalog', () => { let db: jest.Mocked; + let transaction: jest.Mocked; beforeAll(() => { db = { @@ -40,11 +41,14 @@ describe('DatabaseEntitiesCatalog', () => { locationHistory: jest.fn(), addLocationUpdateLogEvent: jest.fn(), }; + transaction = { + rollback: jest.fn(), + }; }); beforeEach(() => { jest.resetAllMocks(); - db.transaction.mockImplementation(async f => f('tx')); + db.transaction.mockImplementation(async f => f(transaction)); }); describe('batchAddOrUpdateEntities', () => { @@ -82,6 +86,36 @@ describe('DatabaseEntitiesCatalog', () => { expect(result).toEqual([{ entityId: 'u' }]); }); + it('adds using existing transaction', async () => { + const entity: Entity = { + apiVersion: 'a', + kind: 'b', + metadata: { + name: 'c', + namespace: 'd', + }, + }; + + const existingTransaction: jest.Mocked = { + rollback: jest.fn(), + }; + + db.entities.mockResolvedValue([]); + db.addEntities.mockResolvedValue([{ entity }]); + + const catalog = new DatabaseEntitiesCatalog(db, getVoidLogger()); + const result = await catalog.addOrUpdateEntity(entity, { + tx: existingTransaction, + }); + + expect(db.addEntities).toHaveBeenCalledTimes(1); + expect(db.addEntities).toHaveBeenCalledWith( + existingTransaction, + expect.anything(), + ); + expect(result).toBe(entity); + }); + it('updates when given uid', async () => { const entity: Entity = { apiVersion: 'a', @@ -131,10 +165,10 @@ describe('DatabaseEntitiesCatalog', () => { ]); expect(db.entityByName).not.toHaveBeenCalled(); expect(db.entityByUid).toHaveBeenCalledTimes(1); - expect(db.entityByUid).toHaveBeenCalledWith(expect.anything(), 'u'); + expect(db.entityByUid).toHaveBeenCalledWith(transaction, 'u'); expect(db.updateEntity).toHaveBeenCalledTimes(1); expect(db.updateEntity).toHaveBeenCalledWith( - expect.anything(), + transaction, { entity: { apiVersion: 'a', @@ -206,14 +240,14 @@ describe('DatabaseEntitiesCatalog', () => { }, ]); expect(db.entityByName).toHaveBeenCalledTimes(1); - expect(db.entityByName).toHaveBeenCalledWith(expect.anything(), { + expect(db.entityByName).toHaveBeenCalledWith(transaction, { kind: 'b', namespace: 'd', name: 'c', }); expect(db.updateEntity).toHaveBeenCalledTimes(1); expect(db.updateEntity).toHaveBeenCalledWith( - expect.anything(), + transaction, { entity: { apiVersion: 'a', diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts index 947736dc06..28a972e8e3 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts @@ -27,7 +27,12 @@ import { import { chunk, groupBy } from 'lodash'; import limiterFactory from 'p-limit'; import { Logger } from 'winston'; -import type { Database, DbEntityResponse, EntityFilters } from '../database'; +import type { + Database, + DbEntityResponse, + EntityFilters, + Transaction, +} from '../database'; import { durationText } from '../util/timing'; import type { EntitiesCatalog, @@ -60,7 +65,14 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { private readonly logger: Logger, ) {} - async entities(filters?: EntityFilters[]): Promise { + async entities(options?: { + filters?: EntityFilters[]; + tx?: Transaction; + }): Promise { + const filters = options?.filters; + + // TODO: Support with and without transaction! + const items = await this.database.transaction(tx => this.database.entities(tx, filters), ); @@ -135,8 +147,10 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { */ async batchAddOrUpdateEntities( requests: EntityUpsertRequest[], - locationId?: string, + options?: { locationId?: string; tx?: Transaction }, ): Promise { + // TODO: How to work with the transaction here? + // Group the entities by unique kind+namespace combinations const entitiesByKindAndNamespace = groupBy(requests, ({ entity }) => { const name = getEntityName(entity); @@ -163,7 +177,11 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { ); // Retry the batch write a few times to deal with contention - const context = { kind, namespace, locationId }; + const context = { + kind, + namespace, + locationId: options?.locationId, + }; for (let attempt = 1; attempt <= BATCH_ATTEMPTS; ++attempt) { try { const { toAdd, toUpdate, toIgnore } = await this.analyzeBatch( @@ -234,13 +252,15 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { const markTimestamp = process.hrtime(); const names = requests.map(({ entity }) => entity.metadata.name); - const oldEntities = await this.entities([ - { - kind: kind, - 'metadata.namespace': namespace, - 'metadata.name': names, - }, - ]); + const oldEntities = await this.entities({ + filters: [ + { + kind: kind, + 'metadata.namespace': namespace, + 'metadata.name': names, + }, + ], + }); const oldEntitiesByName = new Map( oldEntities.map(e => [e.metadata.name, e]), diff --git a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts index f515829b91..b78b9933b9 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts @@ -15,7 +15,7 @@ */ import { Location } from '@backstage/catalog-model'; -import type { Database } from '../database'; +import type { Database, Transaction } from '../database'; import { DatabaseLocationUpdateLogEvent, DatabaseLocationUpdateLogStatus, @@ -25,9 +25,19 @@ import { LocationResponse, LocationsCatalog } from './types'; export class DatabaseLocationsCatalog implements LocationsCatalog { constructor(private readonly database: Database) {} - async addLocation(location: Location): Promise { - const added = await this.database.addLocation(location); - return added; + async addLocation( + location: Location, + options?: { tx?: Transaction }, + ): Promise { + const transaction = options?.tx; + + if (transaction) { + return await this.database.addLocation(transaction, location); + } + + return await this.database.transaction( + async tx => await this.database.addLocation(tx, location), + ); } async removeLocation(id: string): Promise { diff --git a/plugins/catalog-backend/src/catalog/types.ts b/plugins/catalog-backend/src/catalog/types.ts index 539a0e531f..1ac06618b5 100644 --- a/plugins/catalog-backend/src/catalog/types.ts +++ b/plugins/catalog-backend/src/catalog/types.ts @@ -15,7 +15,7 @@ */ import { Entity, EntityRelationSpec, Location } from '@backstage/catalog-model'; -import type { EntityFilters } from '../database'; +import type { EntityFilters, Transaction } from '../database'; // // Entities @@ -31,7 +31,10 @@ export type EntityUpsertResponse = { }; export type EntitiesCatalog = { - entities(filters?: EntityFilters[]): Promise; + entities(options?: { + filters?: EntityFilters[]; + tx?: Transaction; + }): Promise; removeEntityByUid(uid: string): Promise; /** @@ -42,7 +45,7 @@ export type EntitiesCatalog = { */ batchAddOrUpdateEntities( entities: EntityUpsertRequest[], - locationId?: string, + options?: { locationId?: string; tx?: Transaction }, ): Promise; }; @@ -70,7 +73,10 @@ export type LocationResponse = { }; export type LocationsCatalog = { - addLocation(location: Location): Promise; + addLocation( + location: Location, + options?: { tx?: Transaction }, + ): Promise; removeLocation(id: string): Promise; locations(): Promise; location(id: string): Promise; diff --git a/plugins/catalog-backend/src/database/CommonDatabase.test.ts b/plugins/catalog-backend/src/database/CommonDatabase.test.ts index ac11f1dd1a..b2a238b111 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.test.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.test.ts @@ -91,7 +91,7 @@ describe('CommonDatabase', () => { timestamp: null, }; - await db.addLocation(input); + await db.transaction(async tx => await db.addLocation(tx, input)); const locations = await db.locations(); expect(locations).toEqual( @@ -238,7 +238,8 @@ describe('CommonDatabase', () => { type: 'a', target: 'b', }; - await db.addLocation(location); + + await db.transaction(async tx => await db.addLocation(tx, location)); await db.addLocationUpdateLogEvent( 'dd12620d-0436-422f-93bd-929aa0788123', diff --git a/plugins/catalog-backend/src/database/CommonDatabase.ts b/plugins/catalog-backend/src/database/CommonDatabase.ts index 72e6bf0a5f..bd6dc29abc 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.ts @@ -46,6 +46,7 @@ import { DbLocationsRow, DbLocationsRowWithStatus, EntityFilters, + Transaction, } from './types'; // The number of items that are sent per batch to the database layer, when @@ -63,9 +64,23 @@ export class CommonDatabase implements Database { private readonly logger: Logger, ) {} - async transaction(fn: (tx: unknown) => Promise): Promise { + async transaction(fn: (tx: Transaction) => Promise): Promise { try { - return await this.database.transaction(fn); + let result: T | undefined = undefined; + + await this.database.transaction( + async tx => { + // We can't return here, as knex swallows the return type in case the transaction is rolled back: + // https://github.com/knex/knex/blob/e37aeaa31c8ef9c1b07d2e4d3ec6607e557d800d/lib/transaction.js#L136 + result = await fn(tx); + }, + { + // If we explicitly trigger a rollback, don't fail. + doNotRejectOnRollback: true, + }, + ); + + return result!; } catch (e) { this.logger.debug(`Error during transaction, ${e}`); @@ -81,7 +96,7 @@ export class CommonDatabase implements Database { } async addEntity( - txOpaque: unknown, + txOpaque: Transaction, request: DbEntityRequest, ): Promise { const tx = txOpaque as Knex.Transaction; @@ -110,7 +125,7 @@ export class CommonDatabase implements Database { } async addEntities( - txOpaque: unknown, + txOpaque: Transaction, request: DbEntityRequest[], ): Promise { const tx = txOpaque as Knex.Transaction; @@ -158,7 +173,7 @@ export class CommonDatabase implements Database { } async updateEntity( - txOpaque: unknown, + txOpaque: Transaction, request: DbEntityRequest, matchingEtag?: string, matchingGeneration?: number, @@ -217,7 +232,7 @@ export class CommonDatabase implements Database { } async entities( - txOpaque: unknown, + txOpaque: Transaction, filters?: EntityFilters[], ): Promise { const tx = txOpaque as Knex.Transaction; @@ -295,7 +310,7 @@ export class CommonDatabase implements Database { } async entityByName( - txOpaque: unknown, + txOpaque: Transaction, name: EntityName, ): Promise { const tx = txOpaque as Knex.Transaction; @@ -314,7 +329,7 @@ export class CommonDatabase implements Database { } async entityByUid( - txOpaque: unknown, + txOpaque: Transaction, uid: string, ): Promise { const tx = txOpaque as Knex.Transaction; @@ -330,7 +345,7 @@ export class CommonDatabase implements Database { return this.toEntityResponse(tx, rows[0]); } - async removeEntityByUid(txOpaque: unknown, uid: string): Promise { + async removeEntityByUid(txOpaque: Transaction, uid: string): Promise { const tx = txOpaque as Knex.Transaction; const result = await tx('entities').where({ id: uid }).del(); @@ -341,7 +356,7 @@ export class CommonDatabase implements Database { } async setRelations( - txOpaque: unknown, + txOpaque: Transaction, originatingEntityId: string, relations: EntityRelationSpec[], ): Promise { @@ -368,19 +383,22 @@ export class CommonDatabase implements Database { await tx.batchInsert('entities_relations', relationsRows, BATCH_SIZE); } - async addLocation(location: Location): Promise { - return await this.database.transaction(async tx => { - const row: DbLocationsRow = { - id: location.id, - type: location.type, - target: location.target, - }; - await tx('locations').insert(row); - return row; - }); + async addLocation( + txOpaque: Transaction, + location: Location, + ): Promise { + const tx = txOpaque as Knex.Transaction; + + const row: DbLocationsRow = { + id: location.id, + type: location.type, + target: location.target, + }; + await tx('locations').insert(row); + return row; } - async removeLocation(txOpaque: unknown, id: string): Promise { + async removeLocation(txOpaque: Transaction, id: string): Promise { const tx = txOpaque as Knex.Transaction; await tx('entities') diff --git a/plugins/catalog-backend/src/database/index.ts b/plugins/catalog-backend/src/database/index.ts index 565a41cfb2..0d84106815 100644 --- a/plugins/catalog-backend/src/database/index.ts +++ b/plugins/catalog-backend/src/database/index.ts @@ -22,4 +22,5 @@ export type { DbEntityResponse, EntityFilter, EntityFilters, + Transaction, } from './types'; diff --git a/plugins/catalog-backend/src/database/types.ts b/plugins/catalog-backend/src/database/types.ts index b8a2a6231a..62b2d55b88 100644 --- a/plugins/catalog-backend/src/database/types.ts +++ b/plugins/catalog-backend/src/database/types.ts @@ -101,6 +101,13 @@ export type EntityFilter = null | string | (null | string)[]; */ export type EntityFilters = Record; +/** + * An abstraction for transactions of the underlying database technology. + */ +export type Transaction = { + rollback(): Promise; +}; + /** * An abstraction on top of the underlying database, wrapping the basic CRUD * needs. @@ -114,7 +121,7 @@ export type Database = { * * @param fn The callback that implements the transaction */ - transaction(fn: (tx: unknown) => Promise): Promise; + transaction(fn: (tx: Transaction) => Promise): Promise; /** * Adds a set of new entities to the catalog. @@ -123,7 +130,7 @@ export type Database = { * @param request The entities being added */ addEntities( - tx: unknown, + tx: Transaction, request: DbEntityRequest[], ): Promise; @@ -147,22 +154,28 @@ export type Database = { * @returns The updated entity */ updateEntity( - tx: unknown, + tx: Transaction, request: DbEntityRequest, matchingEtag?: string, matchingGeneration?: number, ): Promise; - entities(tx: unknown, filters?: EntityFilters[]): Promise; + entities( + tx: Transaction, + filters?: EntityFilters[], + ): Promise; entityByName( - tx: unknown, + tx: Transaction, name: EntityName, ): Promise; - entityByUid(tx: unknown, uid: string): Promise; + entityByUid( + tx: Transaction, + uid: string, + ): Promise; - removeEntityByUid(tx: unknown, uid: string): Promise; + removeEntityByUid(tx: Transaction, uid: string): Promise; /** * Remove current relations for the entity and replace them with the new relations array @@ -171,14 +184,14 @@ export type Database = { * @param relations the relationships to be set */ setRelations( - tx: unknown, + tx: Transaction, entityUid: string, relations: EntityRelationSpec[], ): Promise; - addLocation(location: Location): Promise; + addLocation(tx: Transaction, location: Location): Promise; - removeLocation(tx: unknown, id: string): Promise; + removeLocation(tx: Transaction, id: string): Promise; location(id: string): Promise; diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts index 933433b47e..174428799d 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts @@ -18,7 +18,11 @@ import { getVoidLogger } from '@backstage/backend-common'; import { Entity, Location, LocationSpec } from '@backstage/catalog-model'; import { EntitiesCatalog, LocationsCatalog } from '../catalog'; import { LocationUpdateStatus } from '../catalog/types'; -import { DatabaseLocationUpdateLogStatus } from '../database/types'; +import { + Database, + DatabaseLocationUpdateLogStatus, + Transaction, +} from '../database/types'; import { HigherOrderOperations } from './HigherOrderOperations'; import { LocationReader } from './types'; @@ -26,6 +30,8 @@ describe('HigherOrderOperations', () => { let entitiesCatalog: jest.Mocked; let locationsCatalog: jest.Mocked; let locationReader: jest.Mocked; + let transaction: jest.Mocked; + let database: jest.Mocked; let higherOrderOperation: HigherOrderOperations; beforeAll(() => { @@ -46,10 +52,29 @@ describe('HigherOrderOperations', () => { locationReader = { read: jest.fn(), }; + transaction = { + rollback: jest.fn(), + }; + database = { + transaction: jest.fn(), + addEntities: jest.fn(), + updateEntity: jest.fn(), + entities: jest.fn(), + entityByName: jest.fn(), + entityByUid: jest.fn(), + removeEntityByUid: jest.fn(), + addLocation: jest.fn(), + removeLocation: jest.fn(), + location: jest.fn(), + locations: jest.fn(), + locationHistory: jest.fn(), + addLocationUpdateLogEvent: jest.fn(), + }; higherOrderOperation = new HigherOrderOperations( entitiesCatalog, locationsCatalog, locationReader, + database, getVoidLogger(), ); }); @@ -64,6 +89,7 @@ describe('HigherOrderOperations', () => { type: 'a', target: 'b', }; + database.transaction.mockImplementation(x => x(transaction)); locationsCatalog.addLocation.mockImplementation(x => Promise.resolve(x)); locationsCatalog.locations.mockResolvedValue([]); locationReader.read.mockResolvedValue({ @@ -90,7 +116,9 @@ describe('HigherOrderOperations', () => { id: expect.anything(), ...spec, }), + { tx: transaction }, ); + expect(transaction.rollback).not.toBeCalled(); }); it('reuses the location if a match already existed', async () => { @@ -103,6 +131,7 @@ describe('HigherOrderOperations', () => { ...spec, }; + database.transaction.mockImplementation(x => x(transaction)); locationsCatalog.locations.mockResolvedValue([ { currentStatus: { timestamp: '', status: '', message: '' }, @@ -123,6 +152,7 @@ describe('HigherOrderOperations', () => { expect(locationReader.read).toBeCalledWith({ type: 'a', target: 'b' }); expect(entitiesCatalog.batchAddOrUpdateEntities).not.toBeCalled(); expect(locationsCatalog.addLocation).not.toBeCalled(); + expect(transaction.rollback).not.toBeCalled(); }); it('rejects the whole operation if any entity could not be read', async () => { @@ -137,6 +167,7 @@ describe('HigherOrderOperations', () => { metadata: { name: 'n' }, }; + database.transaction.mockImplementation(x => x(transaction)); locationsCatalog.locations.mockResolvedValue([]); locationReader.read.mockResolvedValue({ entities: [{ entity, location, relations: [] }], @@ -149,6 +180,43 @@ describe('HigherOrderOperations', () => { expect(locationsCatalog.locations).toBeCalledTimes(1); expect(entitiesCatalog.batchAddOrUpdateEntities).not.toBeCalled(); expect(locationsCatalog.addLocation).not.toBeCalled(); + expect(transaction.rollback).not.toBeCalled(); + }); + + it('rollback everything after a dry run', async () => { + const spec = { + type: 'a', + target: 'b', + }; + database.transaction.mockImplementation(x => x(transaction)); + locationsCatalog.addLocation.mockImplementation(x => Promise.resolve(x)); + locationsCatalog.locations.mockResolvedValue([]); + locationReader.read.mockResolvedValue({ entities: [], errors: [] }); + + const result = await higherOrderOperation.addLocation(spec, { + dryRun: true, + }); + + expect(result.location).toEqual( + expect.objectContaining({ + id: expect.anything(), + ...spec, + }), + ); + expect(result.entities).toEqual([]); + expect(locationsCatalog.locations).toBeCalledTimes(1); + expect(locationReader.read).toBeCalledTimes(1); + expect(locationReader.read).toBeCalledWith({ type: 'a', target: 'b' }); + expect(entitiesCatalog.addOrUpdateEntity).not.toBeCalled(); + expect(locationsCatalog.addLocation).toBeCalledTimes(1); + expect(locationsCatalog.addLocation).toBeCalledWith( + expect.objectContaining({ + id: expect.anything(), + ...spec, + }), + { tx: transaction }, + ); + expect(transaction.rollback).toBeCalled(); }); }); diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts index 0e62815be5..2ca79c60b4 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts @@ -17,6 +17,7 @@ import { Location, LocationSpec } from '@backstage/catalog-model'; import { v4 as uuidv4 } from 'uuid'; import { Logger } from 'winston'; +import { Database } from '../database'; import { EntitiesCatalog, LocationsCatalog } from '../catalog'; import { durationText } from '../util/timing'; import { @@ -33,22 +34,13 @@ import { * database more directly. */ export class HigherOrderOperations implements HigherOrderOperation { - private readonly entitiesCatalog: EntitiesCatalog; - private readonly locationsCatalog: LocationsCatalog; - private readonly locationReader: LocationReader; - private readonly logger: Logger; - constructor( - entitiesCatalog: EntitiesCatalog, - locationsCatalog: LocationsCatalog, - locationReader: LocationReader, - logger: Logger, - ) { - this.entitiesCatalog = entitiesCatalog; - this.locationsCatalog = locationsCatalog; - this.locationReader = locationReader; - this.logger = logger; - } + private readonly entitiesCatalog: EntitiesCatalog, + private readonly locationsCatalog: LocationsCatalog, + private readonly locationReader: LocationReader, + private readonly database: Database, + private readonly logger: Logger, + ) {} /** * Adds a single location to the catalog. @@ -62,7 +54,12 @@ export class HigherOrderOperations implements HigherOrderOperation { * * @param spec The location to add */ - async addLocation(spec: LocationSpec): Promise { + async addLocation( + spec: LocationSpec, + options?: { dryRun?: boolean }, + ): Promise { + const dryRun = options?.dryRun || false; + // Attempt to find a previous location matching the spec const previousLocations = await this.locationsCatalog.locations(); const previousLocation = previousLocations.find( @@ -88,21 +85,36 @@ export class HigherOrderOperations implements HigherOrderOperation { // in the entities list. But we aren't sure what to do about those yet. // Write - if (!previousLocation) { - await this.locationsCatalog.addLocation(location); - } - if (readerOutput.entities.length === 0) { - return { location, entities: [] }; - } + const entities = await this.database.transaction(async tx => { + if (!previousLocation) { + await this.locationsCatalog.addLocation(location, { tx }); + } + if (readerOutput.entities.length === 0) { + return { location, entities: [] }; + } - const writtenEntities = await this.entitiesCatalog.batchAddOrUpdateEntities( - readerOutput.entities, - location.id, - ); + const writtenEntities = await this.entitiesCatalog.batchAddOrUpdateEntities( + readerOutput.entities, + { + locationId: location.id, + tx, + }, + ); - const entities = await this.entitiesCatalog.entities([ - { 'metadata.uid': writtenEntities.map(e => e.entityId) }, - ]); + const outputEntities = await this.entitiesCatalog.entities( + [{ 'metadata.uid': writtenEntities.map(e => e.entityId) }], + { tx }, + ); + + if (dryRun) { + // If this is only a dry run, cancel the database transaction even if it was successful. + await tx.rollback(); + + this.logger.debug(`Perfomed successful dry run of adding a location`); + } + + return outputEntities; + }); return { location, entities }; } @@ -168,7 +180,7 @@ export class HigherOrderOperations implements HigherOrderOperation { try { await this.entitiesCatalog.batchAddOrUpdateEntities( readerOutput.entities, - location.id, + { locationId: location.id }, ); } catch (e) { for (const entity of readerOutput.entities) { diff --git a/plugins/catalog-backend/src/ingestion/types.ts b/plugins/catalog-backend/src/ingestion/types.ts index c586e2d438..a83b4adecc 100644 --- a/plugins/catalog-backend/src/ingestion/types.ts +++ b/plugins/catalog-backend/src/ingestion/types.ts @@ -26,7 +26,10 @@ import type { // export type HigherOrderOperation = { - addLocation(spec: LocationSpec): Promise; + addLocation( + spec: LocationSpec, + options?: { dryRun?: boolean }, + ): Promise; refreshAllLocations(): Promise; }; diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index b041dac5ab..f2a4ac7468 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -227,6 +227,7 @@ export class CatalogBuilder { entitiesCatalog, locationsCatalog, locationReader, + db, logger, ); diff --git a/plugins/catalog-backend/src/service/router.test.ts b/plugins/catalog-backend/src/service/router.test.ts index a6b22eaad9..3a43ac96ad 100644 --- a/plugins/catalog-backend/src/service/router.test.ts +++ b/plugins/catalog-backend/src/service/router.test.ts @@ -23,6 +23,8 @@ import { LocationResponse } from '../catalog/types'; import { HigherOrderOperation } from '../ingestion/types'; import { createRouter } from './router'; +// TODO: ... + describe('createRouter', () => { let entitiesCatalog: jest.Mocked; let locationsCatalog: jest.Mocked; @@ -83,13 +85,15 @@ describe('createRouter', () => { expect(response.status).toEqual(200); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { - a: ['1', null, '3'], - b: ['4'], - }, - { c: [null] }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + filters: [ + { + a: ['1', null, '3'], + b: ['4'], + }, + { c: [null] }, + ], + }); }); }); @@ -107,9 +111,9 @@ describe('createRouter', () => { const response = await request(app).get('/entities/by-uid/zzz'); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { 'metadata.uid': 'zzz' }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + filters: [{ 'metadata.uid': 'zzz' }], + }); expect(response.status).toEqual(200); expect(response.body).toEqual(expect.objectContaining(entity)); }); @@ -120,9 +124,9 @@ describe('createRouter', () => { const response = await request(app).get('/entities/by-uid/zzz'); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { 'metadata.uid': 'zzz' }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + filters: [{ 'metadata.uid': 'zzz' }], + }); expect(response.status).toEqual(404); expect(response.text).toMatch(/uid/); }); @@ -143,13 +147,15 @@ describe('createRouter', () => { const response = await request(app).get('/entities/by-name/k/ns/n'); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { - kind: 'k', - 'metadata.namespace': 'ns', - 'metadata.name': 'n', - }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + filters: [ + { + kind: 'k', + 'metadata.namespace': 'ns', + 'metadata.name': 'n', + }, + ], + }); expect(response.status).toEqual(200); expect(response.body).toEqual(expect.objectContaining(entity)); }); @@ -160,13 +166,15 @@ describe('createRouter', () => { const response = await request(app).get('/entities/by-name/b/d/c'); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { - kind: 'b', - 'metadata.namespace': 'd', - 'metadata.name': 'c', - }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + filters: [ + { + kind: 'b', + 'metadata.namespace': 'd', + 'metadata.name': 'c', + }, + ], + }); expect(response.status).toEqual(404); expect(response.text).toMatch(/name/); }); @@ -209,9 +217,9 @@ describe('createRouter', () => { { entity, relations: [] }, ]); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { 'metadata.uid': 'u' }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + filters: [{ 'metadata.uid': 'u' }], + }); expect(response.status).toEqual(200); expect(response.body).toEqual(entity); }); @@ -285,7 +293,36 @@ describe('createRouter', () => { const response = await request(app).post('/locations').send(spec); expect(higherOrderOperation.addLocation).toHaveBeenCalledTimes(1); - expect(higherOrderOperation.addLocation).toHaveBeenCalledWith(spec); + expect(higherOrderOperation.addLocation).toHaveBeenCalledWith(spec, { + dryRun: false, + }); + expect(response.status).toEqual(201); + expect(response.body).toEqual( + expect.objectContaining({ + location: { id: 'a', ...spec }, + }), + ); + }); + + it('supports dry run', async () => { + const spec: LocationSpec = { + type: 'b', + target: 'c', + }; + + higherOrderOperation.addLocation.mockResolvedValue({ + location: { id: 'a', ...spec }, + entities: [], + }); + + const response = await request(app) + .post('/locations?dryRun=true') + .send(spec); + + expect(higherOrderOperation.addLocation).toHaveBeenCalledTimes(1); + expect(higherOrderOperation.addLocation).toHaveBeenCalledWith(spec, { + dryRun: true, + }); expect(response.status).toEqual(201); expect(response.body).toEqual( expect.objectContaining({ diff --git a/plugins/catalog-backend/src/service/router.ts b/plugins/catalog-backend/src/service/router.ts index 785e44f68c..e297bf16c6 100644 --- a/plugins/catalog-backend/src/service/router.ts +++ b/plugins/catalog-backend/src/service/router.ts @@ -20,6 +20,7 @@ import { locationSpecSchema } from '@backstage/catalog-model'; import express from 'express'; import Router from 'express-promise-router'; import { Logger } from 'winston'; +import yn from 'yn'; import { EntitiesCatalog, LocationsCatalog } from '../catalog'; import { HigherOrderOperation } from '../ingestion/types'; import { @@ -48,7 +49,7 @@ export async function createRouter( .get('/entities', async (req, res) => { const filters = translateQueryToEntityFilters(req.query); const fieldMapper = translateQueryToFieldMapper(req.query); - const entities = await entitiesCatalog.entities(filters); + const entities = await entitiesCatalog.entities({ filters }); res.status(200).send(entities.map(fieldMapper)); }) .post('/entities', async (req, res) => { @@ -56,18 +57,20 @@ export async function createRouter( const [result] = await entitiesCatalog.batchAddOrUpdateEntities([ { entity: body as Entity, relations: [] }, ]); - const [entity] = await entitiesCatalog.entities([ - { 'metadata.uid': result.entityId }, - ]); + const [entity] = await entitiesCatalog.entities({ + filters: [{ 'metadata.uid': result.entityId }], + }); res.status(200).send(entity); }) .get('/entities/by-uid/:uid', async (req, res) => { const { uid } = req.params; - const entities = await entitiesCatalog.entities([ - { - 'metadata.uid': uid, - }, - ]); + const entities = await entitiesCatalog.entities({ + filters: [ + { + 'metadata.uid': uid, + }, + ], + }); if (!entities.length) { res.status(404).send(`No entity with uid ${uid}`); } @@ -80,13 +83,15 @@ export async function createRouter( }) .get('/entities/by-name/:kind/:namespace/:name', async (req, res) => { const { kind, namespace, name } = req.params; - const entities = await entitiesCatalog.entities([ - { - kind: kind, - 'metadata.namespace': namespace, - 'metadata.name': name, - }, - ]); + const entities = await entitiesCatalog.entities({ + filters: [ + { + kind: kind, + 'metadata.namespace': namespace, + 'metadata.name': name, + }, + ], + }); if (!entities.length) { res .status(404) @@ -101,7 +106,8 @@ export async function createRouter( if (higherOrderOperation) { router.post('/locations', async (req, res) => { const input = await validateRequestBody(req, locationSpecSchema); - const output = await higherOrderOperation.addLocation(input); + const dryRun = yn(req.query.dryRun, { default: false }); + const output = await higherOrderOperation.addLocation(input, { dryRun }); res.status(201).send(output); }); }