From fdff44b72814511376d2413da6450bbb50980b4c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 20 Oct 2020 19:32:34 +0200 Subject: [PATCH] catalog-backend: update tests to work with relation changes + fix relation updates --- .../catalog/DatabaseEntitiesCatalog.test.ts | 171 +++++++++++++----- .../src/catalog/DatabaseEntitiesCatalog.ts | 16 +- .../ingestion/HigherOrderOperations.test.ts | 25 ++- .../src/ingestion/HigherOrderOperations.ts | 8 + .../src/service/CatalogBuilder.test.ts | 8 +- .../src/service/router.test.ts | 20 +- plugins/catalog-backend/src/service/router.ts | 2 +- 7 files changed, 188 insertions(+), 62 deletions(-) diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts index d84a8e071c..58cac90b7d 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts @@ -18,6 +18,7 @@ import { getVoidLogger } from '@backstage/backend-common'; import type { Entity } from '@backstage/catalog-model'; import { Database, DatabaseManager } from '../database'; import { DatabaseEntitiesCatalog } from './DatabaseEntitiesCatalog'; +import { EntityMutationRequest } from './types'; describe('DatabaseEntitiesCatalog', () => { let db: jest.Mocked; @@ -46,7 +47,7 @@ describe('DatabaseEntitiesCatalog', () => { db.transaction.mockImplementation(async f => f('tx')); }); - describe('addOrUpdateEntity', () => { + describe('batchAddOrUpdateEntities', () => { it('adds when no given uid and no matching by name', async () => { const entity: Entity = { apiVersion: 'a', @@ -58,19 +59,25 @@ describe('DatabaseEntitiesCatalog', () => { }; db.entities.mockResolvedValue([]); - db.addEntities.mockResolvedValue([{ entity }]); + db.addEntities.mockResolvedValue([ + { entity: { ...entity, metadata: { ...entity.metadata, uid: 'u' } } }, + ]); const catalog = new DatabaseEntitiesCatalog(db, getVoidLogger()); - const result = await catalog.addOrUpdateEntity(entity); + const result = await catalog.batchAddOrUpdateEntities([ + { entity, relations: [] }, + ]); - expect(db.entityByName).toHaveBeenCalledTimes(1); - expect(db.entityByName).toHaveBeenCalledWith(expect.anything(), { + expect(db.entities).toHaveBeenCalledTimes(1); + expect(db.entities).toHaveBeenCalledWith(expect.anything(), { kind: 'b', - namespace: 'd', - name: 'c', + 'metadata.namespace': 'd', + 'metadata.name': ['c'], }); + expect(db.setRelations).toHaveBeenCalledTimes(1); + expect(db.setRelations).toHaveBeenCalledWith(expect.anything(), 'u', []); expect(db.addEntities).toHaveBeenCalledTimes(1); - expect(result).toBe(entity); + expect(result).toEqual([{ entityId: 'u' }]); }); it('updates when given uid', async () => { @@ -82,9 +89,11 @@ describe('DatabaseEntitiesCatalog', () => { name: 'c', namespace: 'd', }, + spec: { + x: 'b', + }, }; - - db.entityByUid.mockResolvedValue({ + const existing = { entity: { apiVersion: 'a', kind: 'b', @@ -95,14 +104,28 @@ describe('DatabaseEntitiesCatalog', () => { name: 'c', namespace: 'd', }, + spec: { + x: 'a', + }, }, - }); + }; + + db.entities.mockResolvedValue([existing]); + db.entityByUid.mockResolvedValue(existing); db.updateEntity.mockResolvedValue({ entity }); const catalog = new DatabaseEntitiesCatalog(db, getVoidLogger()); - const result = await catalog.addOrUpdateEntity(entity); + const result = await catalog.batchAddOrUpdateEntities([ + { entity, relations: [] }, + ]); - expect(db.entities).toHaveBeenCalledTimes(0); + expect(db.entities).toHaveBeenCalledTimes(1); + expect(db.entities).toHaveBeenCalledWith(expect.anything(), { + kind: 'b', + 'metadata.namespace': 'd', + 'metadata.name': ['c'], + }); + expect(db.entityByName).not.toHaveBeenCalled(); expect(db.entityByUid).toHaveBeenCalledTimes(1); expect(db.entityByUid).toHaveBeenCalledWith(expect.anything(), 'u'); expect(db.updateEntity).toHaveBeenCalledTimes(1); @@ -114,17 +137,22 @@ describe('DatabaseEntitiesCatalog', () => { kind: 'b', metadata: { uid: 'u', - etag: 'e', - generation: 1, + etag: expect.any(String), + generation: 2, name: 'c', namespace: 'd', }, + spec: { + x: 'b', + }, }, }, 'e', 1, ); - expect(result).toBe(entity); + expect(db.setRelations).toHaveBeenCalledTimes(1); + expect(db.setRelations).toHaveBeenCalledWith(expect.anything(), 'u', []); + expect(result).toEqual([{ entityId: 'u' }]); }); it('update when no given uid and matching by name', async () => { @@ -135,25 +163,42 @@ describe('DatabaseEntitiesCatalog', () => { name: 'c', namespace: 'd', }, + spec: { + x: 'b', + }, }; - const existing: Entity = { - apiVersion: 'a', - kind: 'b', - metadata: { - uid: 'u', - etag: 'e', - generation: 1, - name: 'c', - namespace: 'd', + const existing = { + entity: { + apiVersion: 'a', + kind: 'b', + metadata: { + uid: 'u', + etag: 'e', + generation: 1, + name: 'c', + namespace: 'd', + }, + spec: { + x: 'a', + }, }, }; - db.entityByName.mockResolvedValue({ entity: existing }); - db.updateEntity.mockResolvedValue({ entity: existing }); + db.entities.mockResolvedValue([existing]); + db.entityByName.mockResolvedValue(existing); + db.updateEntity.mockResolvedValue(existing); const catalog = new DatabaseEntitiesCatalog(db, getVoidLogger()); - const result = await catalog.addOrUpdateEntity(added); + const result = await catalog.batchAddOrUpdateEntities([ + { entity: added, relations: [] }, + ]); + expect(db.entities).toHaveBeenCalledTimes(1); + expect(db.entities).toHaveBeenCalledWith(expect.anything(), { + kind: 'b', + 'metadata.namespace': 'd', + 'metadata.name': ['c'], + }); expect(db.entityByName).toHaveBeenCalledTimes(1); expect(db.entityByName).toHaveBeenCalledWith(expect.anything(), { kind: 'b', @@ -169,32 +214,73 @@ describe('DatabaseEntitiesCatalog', () => { kind: 'b', metadata: { uid: 'u', - etag: 'e', - generation: 1, + etag: expect.any(String), + generation: 2, name: 'c', namespace: 'd', }, + spec: { + x: 'b', + }, }, }, 'e', 1, ); - expect(result).toEqual(existing); + expect(result).toEqual([{ entityId: 'u' }]); + }); + + it('should not update if entity is unchanged', async () => { + const entity: Entity = { + apiVersion: 'a', + kind: 'b', + metadata: { + uid: 'u', + name: 'c', + namespace: 'd', + }, + spec: { + x: 'a', + }, + }; + + db.entities.mockResolvedValue([{ entity }]); + db.entityByUid.mockResolvedValue({ entity }); + db.updateEntity.mockResolvedValue({ entity }); + + const catalog = new DatabaseEntitiesCatalog(db, getVoidLogger()); + const result = await catalog.batchAddOrUpdateEntities([ + { entity, relations: [] }, + ]); + + expect(db.entities).toHaveBeenCalledTimes(1); + expect(db.entities).toHaveBeenCalledWith(expect.anything(), { + kind: 'b', + 'metadata.namespace': 'd', + 'metadata.name': ['c'], + }); + expect(db.entityByName).not.toHaveBeenCalled(); + expect(db.entityByUid).not.toHaveBeenCalled(); + expect(db.updateEntity).not.toHaveBeenCalled(); + expect(db.setRelations).toHaveBeenCalledTimes(1); + expect(db.setRelations).toHaveBeenCalledWith(expect.anything(), 'u', []); + expect(result).toEqual([{ entityId: 'u' }]); }); - }); - describe('batchAddOrUpdateEntities', () => { it('both adds and updates', async () => { const catalog = new DatabaseEntitiesCatalog( await DatabaseManager.createTestDatabase(), getVoidLogger(), ); - const entities: Entity[] = []; + const entities: EntityMutationRequest[] = []; for (let i = 0; i < 500; ++i) { entities.push({ - apiVersion: 'a', - kind: 'k', - metadata: { name: `n${i}` }, + entity: { + apiVersion: 'a', + kind: 'k', + metadata: { name: `n${i}` }, + }, + relations: [], }); } @@ -202,11 +288,14 @@ describe('DatabaseEntitiesCatalog', () => { const afterFirst = await catalog.entities(); expect(afterFirst.length).toBe(500); - entities[40].metadata.op = 'changed'; + entities[40].entity.metadata.op = 'changed'; entities.push({ - apiVersion: 'a', - kind: 'k', - metadata: { name: `n500`, op: 'added' }, + entity: { + apiVersion: 'a', + kind: 'k', + metadata: { name: `n500`, op: 'added' }, + }, + relations: [], }); await catalog.batchAddOrUpdateEntities(entities); diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts index e4accf9a8d..18769cfdf7 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts @@ -166,7 +166,7 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { const context = { kind, namespace, locationId }; for (let attempt = 1; attempt <= BATCH_ATTEMPTS; ++attempt) { try { - const { toAdd, toUpdate } = await this.analyzeBatch( + const { toAdd, toUpdate, toIgnore } = await this.analyzeBatch( batch, context, ); @@ -180,6 +180,14 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { ...(await this.batchUpdate(toUpdate, context)), ); } + // TODO(Rugvip): We currently always update relations, but we + // likely want to figure out a way to avoid that + for (const { entity, relations } of toIgnore) { + const entityId = entity.metadata.uid!; + await this.setRelations(entityId, relations); + modifiedEntityIds.push({ entityId }); + } + break; } catch (e) { if (e instanceof ConflictError && attempt < BATCH_ATTEMPTS) { @@ -221,6 +229,7 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { ): Promise<{ toAdd: EntityMutationRequest[]; toUpdate: EntityMutationRequest[]; + toIgnore: EntityMutationRequest[]; }> { const markTimestamp = process.hrtime(); @@ -237,6 +246,7 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { const toAdd: EntityMutationRequest[] = []; const toUpdate: EntityMutationRequest[] = []; + const toIgnore: EntityMutationRequest[] = []; for (const request of requests) { const newEntity = request.entity; @@ -248,6 +258,8 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { // but should probably calculate the end result entity right here // instead and call a dedicated batch update database method instead toUpdate.push(request); + } else { + toIgnore.push(request); } } @@ -257,7 +269,7 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { } entities to update in ${durationText(markTimestamp)}`, ); - return { toAdd, toUpdate }; + return { toAdd, toUpdate, toIgnore }; } // Efficiently adds the given entities to storage, under the assumption that diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts index b8aaa5fb13..933433b47e 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts @@ -83,7 +83,7 @@ describe('HigherOrderOperations', () => { expect(locationsCatalog.locations).toBeCalledTimes(1); expect(locationReader.read).toBeCalledTimes(1); expect(locationReader.read).toBeCalledWith({ type: 'a', target: 'b' }); - expect(entitiesCatalog.addOrUpdateEntity).not.toBeCalled(); + expect(entitiesCatalog.batchAddOrUpdateEntities).not.toBeCalled(); expect(locationsCatalog.addLocation).toBeCalledTimes(1); expect(locationsCatalog.addLocation).toBeCalledWith( expect.objectContaining({ @@ -121,7 +121,7 @@ describe('HigherOrderOperations', () => { expect(locationsCatalog.locations).toBeCalledTimes(1); expect(locationReader.read).toBeCalledTimes(1); expect(locationReader.read).toBeCalledWith({ type: 'a', target: 'b' }); - expect(entitiesCatalog.addOrUpdateEntity).not.toBeCalled(); + expect(entitiesCatalog.batchAddOrUpdateEntities).not.toBeCalled(); expect(locationsCatalog.addLocation).not.toBeCalled(); }); @@ -147,7 +147,7 @@ describe('HigherOrderOperations', () => { /abcd/, ); expect(locationsCatalog.locations).toBeCalledTimes(1); - expect(entitiesCatalog.addOrUpdateEntity).not.toBeCalled(); + expect(entitiesCatalog.batchAddOrUpdateEntities).not.toBeCalled(); expect(locationsCatalog.addLocation).not.toBeCalled(); }); }); @@ -162,7 +162,7 @@ describe('HigherOrderOperations', () => { expect(locationsCatalog.locations).toHaveBeenCalledTimes(1); expect(locationReader.read).not.toHaveBeenCalled(); - expect(entitiesCatalog.addOrUpdateEntity).not.toHaveBeenCalled(); + expect(entitiesCatalog.batchAddOrUpdateEntities).not.toHaveBeenCalled(); }); it('can update a single location where a matching entity did not exist', async () => { @@ -182,6 +182,7 @@ describe('HigherOrderOperations', () => { metadata: { name: 'c1' }, spec: { type: 'service' }, }; + const entityId = 'xyz123'; locationsCatalog.locations.mockResolvedValue([ { currentStatus: locationStatus, data: location }, @@ -190,7 +191,9 @@ describe('HigherOrderOperations', () => { entities: [{ entity: desc, location, relations: [] }], errors: [], }); - entitiesCatalog.batchAddOrUpdateEntities.mockResolvedValue(undefined); + entitiesCatalog.batchAddOrUpdateEntities.mockResolvedValue([ + { entityId }, + ]); await expect( higherOrderOperation.refreshAllLocations(), @@ -203,9 +206,13 @@ describe('HigherOrderOperations', () => { target: 'thing', }); expect(entitiesCatalog.batchAddOrUpdateEntities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.batchAddOrUpdateEntities).toHaveBeenNthCalledWith( - 1, - [expect.objectContaining({ metadata: { name: 'c1' } })], + expect(entitiesCatalog.batchAddOrUpdateEntities).toHaveBeenCalledWith( + [ + expect.objectContaining({ + entity: expect.objectContaining({ metadata: { name: 'c1' } }), + relations: [], + }), + ], '123', ); }); @@ -236,7 +243,7 @@ describe('HigherOrderOperations', () => { errors: [], }); entitiesCatalog.entities.mockResolvedValue([]); - entitiesCatalog.addEntities.mockResolvedValue(undefined); + entitiesCatalog.batchAddOrUpdateEntities.mockResolvedValue([]); await expect( higherOrderOperation.refreshAllLocations(), diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts index 2771945db2..314b91cb13 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts @@ -94,11 +94,19 @@ export class HigherOrderOperations implements HigherOrderOperation { if (!previousLocation) { await this.locationsCatalog.addLocation(location); } + if (readerOutput.entities.length === 0) { + return { location, entities: [] }; + } + const writtenEntities = await this.entitiesCatalog.batchAddOrUpdateEntities( readerOutput.entities, location.id, ); + if (writtenEntities.length === 0) { + return { location, entities: [] }; + } + const entities = await this.entitiesCatalog.entities({ 'metadata.uid': writtenEntities.map(e => e.entityId), }); diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.test.ts b/plugins/catalog-backend/src/service/CatalogBuilder.test.ts index 3f7c613c00..bc71934488 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.test.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.test.ts @@ -128,12 +128,16 @@ describe('CatalogBuilder', () => { { apiVersion: 'av', kind: 'Component', - metadata: expect.objectContaining({ + metadata: { name: 'n', namespace: 'ns', post: 'p', replaced: 'tt2', - }), + uid: expect.any(String), + etag: expect.any(String), + generation: expect.any(Number), + }, + relations: [], }, ]); }); diff --git a/plugins/catalog-backend/src/service/router.test.ts b/plugins/catalog-backend/src/service/router.test.ts index 71caeb3d6c..cf843acf3c 100644 --- a/plugins/catalog-backend/src/service/router.test.ts +++ b/plugins/catalog-backend/src/service/router.test.ts @@ -170,7 +170,7 @@ describe('createRouter', () => { .set('Content-Type', 'application/json') .send(); - expect(entitiesCatalog.addOrUpdateEntity).not.toHaveBeenCalled(); + expect(entitiesCatalog.batchAddOrUpdateEntities).not.toHaveBeenCalled(); expect(response.status).toEqual(400); expect(response.text).toMatch(/body/); }); @@ -185,18 +185,24 @@ describe('createRouter', () => { }, }; - entitiesCatalog.addOrUpdateEntity.mockResolvedValue(entity); + entitiesCatalog.batchAddOrUpdateEntities.mockResolvedValue([ + { entityId: 'u' }, + ]); + entitiesCatalog.entities.mockResolvedValue([entity]); const response = await request(app) .post('/entities') .send(entity) .set('Content-Type', 'application/json'); - expect(entitiesCatalog.addOrUpdateEntity).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.addOrUpdateEntity).toHaveBeenNthCalledWith( - 1, - entity, - ); + expect(entitiesCatalog.batchAddOrUpdateEntities).toHaveBeenCalledTimes(1); + expect(entitiesCatalog.batchAddOrUpdateEntities).toHaveBeenCalledWith([ + { entity, relations: [] }, + ]); + expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + 'metadata.uid': 'u', + }); expect(response.status).toEqual(200); expect(response.body).toEqual(entity); }); diff --git a/plugins/catalog-backend/src/service/router.ts b/plugins/catalog-backend/src/service/router.ts index 5af773f1a0..c7d05afb2d 100644 --- a/plugins/catalog-backend/src/service/router.ts +++ b/plugins/catalog-backend/src/service/router.ts @@ -52,7 +52,7 @@ export async function createRouter( const [result] = await entitiesCatalog.batchAddOrUpdateEntities([ { entity: body as Entity, relations: [] }, ]); - const entity = await entitiesCatalog.entities({ + const [entity] = await entitiesCatalog.entities({ 'metadata.uid': result.entityId, }); res.status(200).send(entity);