From 62493e54c1b63f94c72ae9940eb163d937ade166 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 23 Aug 2020 10:42:17 +0200 Subject: [PATCH 1/8] relations wip --- dev-components.yaml | 38 +++++++++ .../ingestion/HigherOrderOperations.test.ts | 15 +++- .../src/ingestion/LocationReaders.ts | 10 ++- .../processors/GroupPopulatorProcessor.ts | 82 +++++++++++++++++++ .../src/ingestion/processors/index.ts | 1 + .../src/ingestion/processors/results.ts | 10 ++- .../src/ingestion/processors/types.ts | 12 ++- .../catalog-backend/src/ingestion/types.ts | 8 +- .../src/service/CatalogBuilder.ts | 2 + 9 files changed, 172 insertions(+), 6 deletions(-) create mode 100644 dev-components.yaml create mode 100644 plugins/catalog-backend/src/ingestion/processors/GroupPopulatorProcessor.ts diff --git a/dev-components.yaml b/dev-components.yaml new file mode 100644 index 0000000000..1671f6057f --- /dev/null +++ b/dev-components.yaml @@ -0,0 +1,38 @@ +apiVersion: backstage.io/v1alpha1 +kind: Group +metadata: + name: root +spec: + type: org +--- +apiVersion: backstage.io/v1alpha1 +kind: Group +metadata: + name: platform +spec: + type: mission + parent: root +# --- +# apiVersion: backstage.io/v1alpha1 +# kind: Group +# metadata: +# name: pdx +# spec: +# type: tribe +# parent: platform +# --- +# apiVersion: backstage.io/v1alpha1 +# kind: Group +# metadata: +# name: tools +# spec: +# type: squad +# parent: pdx +# --- +# apiVersion: backstage.io/v1alpha1 +# kind: Group +# metadata: +# name: pulp-fiction +# spec: +# type: squad +# parent: pdx diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts index 34320fe18f..c7c8e738c8 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts @@ -69,7 +69,11 @@ describe('HigherOrderOperations', () => { }; locationsCatalog.addLocation.mockImplementation(x => Promise.resolve(x)); locationsCatalog.locations.mockResolvedValue([]); - locationReader.read.mockResolvedValue({ entities: [], errors: [] }); + locationReader.read.mockResolvedValue({ + entities: [], + errors: [], + relations: [], + }); const result = await higherOrderOperation.addLocation(spec); @@ -109,7 +113,11 @@ describe('HigherOrderOperations', () => { data: location, }, ]); - locationReader.read.mockResolvedValue({ entities: [], errors: [] }); + locationReader.read.mockResolvedValue({ + entities: [], + errors: [], + relations: [], + }); const result = await higherOrderOperation.addLocation(spec); @@ -138,6 +146,7 @@ describe('HigherOrderOperations', () => { locationReader.read.mockResolvedValue({ entities: [{ entity, location }], errors: [{ error: new Error('abcd'), location }], + relations: [], }); await expect(higherOrderOperation.addLocation(spec)).rejects.toThrow( @@ -186,6 +195,7 @@ describe('HigherOrderOperations', () => { locationReader.read.mockResolvedValue({ entities: [{ entity: desc, location }], errors: [], + relations: [], }); entitiesCatalog.batchAddOrUpdateEntities.mockResolvedValue(undefined); @@ -231,6 +241,7 @@ describe('HigherOrderOperations', () => { locationReader.read.mockResolvedValue({ entities: [{ entity: desc, location }], errors: [], + relations: [], }); entitiesCatalog.entities.mockResolvedValue([]); entitiesCatalog.addEntities.mockResolvedValue(undefined); diff --git a/plugins/catalog-backend/src/ingestion/LocationReaders.ts b/plugins/catalog-backend/src/ingestion/LocationReaders.ts index e23a4524b6..597a8ad6a3 100644 --- a/plugins/catalog-backend/src/ingestion/LocationReaders.ts +++ b/plugins/catalog-backend/src/ingestion/LocationReaders.ts @@ -61,7 +61,11 @@ export class LocationReaders implements LocationReader { async read(location: LocationSpec): Promise { const { rulesEnforcer, logger } = this.options; - const output: ReadLocationResult = { entities: [], errors: [] }; + const output: ReadLocationResult = { + entities: [], + errors: [], + relations: [], + }; let items: CatalogProcessorResult[] = [result.location(location, false)]; for (let depth = 0; depth < MAX_DEPTH; ++depth) { @@ -96,6 +100,10 @@ export class LocationReaders implements LocationReader { location: item.location, error: item.error, }); + } else if (item.type === 'relation') { + output.relations.push({ + relation: item.relation, + }); } } diff --git a/plugins/catalog-backend/src/ingestion/processors/GroupPopulatorProcessor.ts b/plugins/catalog-backend/src/ingestion/processors/GroupPopulatorProcessor.ts new file mode 100644 index 0000000000..a1b0c6f15e --- /dev/null +++ b/plugins/catalog-backend/src/ingestion/processors/GroupPopulatorProcessor.ts @@ -0,0 +1,82 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { + Entity, + ENTITY_DEFAULT_NAMESPACE, + GroupEntity, + LocationSpec, +} from '@backstage/catalog-model'; +import { CatalogProcessor, CatalogProcessorEmit } from './types'; +import * as result from './results'; + +export class GroupPopulatorProcessor implements CatalogProcessor { + async postProcessEntity( + entity: Entity, + _location: LocationSpec, + emit: CatalogProcessorEmit, + ): Promise { + if (entity.kind.toLowerCase() !== 'group') { + return entity; + } + const spec = entity.spec as GroupEntity['spec']; + + const { parent, children } = spec; + const self = { + kind: entity.kind, + name: entity.metadata.name, + namespace: entity.metadata.namespace ?? ENTITY_DEFAULT_NAMESPACE, + }; + + if (parent) { + emit( + result.relation({ + type: 'parent', + source: self, + target: { ...self, name: parent }, + }), + ); + emit( + result.relation({ + type: 'child', + source: { ...self, name: parent }, + target: self, + }), + ); + } + + if (children) { + for (const child of children) { + emit( + result.relation({ + type: 'child', + source: self, + target: { ...self, name: child }, + }), + ); + emit( + result.relation({ + type: 'parent', + source: { ...self, name: child }, + target: self, + }), + ); + } + } + + return entity; + } +} diff --git a/plugins/catalog-backend/src/ingestion/processors/index.ts b/plugins/catalog-backend/src/ingestion/processors/index.ts index 59a87af555..2a09247e45 100644 --- a/plugins/catalog-backend/src/ingestion/processors/index.ts +++ b/plugins/catalog-backend/src/ingestion/processors/index.ts @@ -28,6 +28,7 @@ export { GithubOrgReaderProcessor } from './GithubOrgReaderProcessor'; export { GithubReaderProcessor } from './GithubReaderProcessor'; export { GitlabApiReaderProcessor } from './GitlabApiReaderProcessor'; export { GitlabReaderProcessor } from './GitlabReaderProcessor'; +export { GroupPopulatorProcessor } from './GroupPopulatorProcessor'; export { LocationRefProcessor } from './LocationEntityProcessor'; export { PlaceholderProcessor } from './PlaceholderProcessor'; export type { PlaceholderResolver } from './PlaceholderProcessor'; diff --git a/plugins/catalog-backend/src/ingestion/processors/results.ts b/plugins/catalog-backend/src/ingestion/processors/results.ts index 2718d871dd..158bc3d950 100644 --- a/plugins/catalog-backend/src/ingestion/processors/results.ts +++ b/plugins/catalog-backend/src/ingestion/processors/results.ts @@ -15,7 +15,11 @@ */ import { InputError, NotFoundError } from '@backstage/backend-common'; -import { Entity, LocationSpec } from '@backstage/catalog-model'; +import { + Entity, + EntityRelationSpec, + LocationSpec, +} from '@backstage/catalog-model'; import { CatalogProcessorResult } from './types'; export function notFoundError( @@ -67,3 +71,7 @@ export function entity( ): CatalogProcessorResult { return { type: 'entity', location: atLocation, entity: newEntity }; } + +export function relation(spec: EntityRelationSpec): CatalogProcessorResult { + return { type: 'relation', relation: spec }; +} diff --git a/plugins/catalog-backend/src/ingestion/processors/types.ts b/plugins/catalog-backend/src/ingestion/processors/types.ts index c9adf07eff..9ed5e6e92d 100644 --- a/plugins/catalog-backend/src/ingestion/processors/types.ts +++ b/plugins/catalog-backend/src/ingestion/processors/types.ts @@ -14,7 +14,11 @@ * limitations under the License. */ -import { Entity, LocationSpec } from '@backstage/catalog-model'; +import { + Entity, + EntityRelationSpec, + LocationSpec, +} from '@backstage/catalog-model'; export type CatalogProcessor = { /** @@ -113,6 +117,11 @@ export type CatalogProcessorEntityResult = { location: LocationSpec; }; +export type CatalogProcessorRelationResult = { + type: 'relation'; + relation: EntityRelationSpec; +}; + export type CatalogProcessorErrorResult = { type: 'error'; error: Error; @@ -123,4 +132,5 @@ export type CatalogProcessorResult = | CatalogProcessorLocationResult | CatalogProcessorDataResult | CatalogProcessorEntityResult + | CatalogProcessorRelationResult | CatalogProcessorErrorResult; diff --git a/plugins/catalog-backend/src/ingestion/types.ts b/plugins/catalog-backend/src/ingestion/types.ts index 5bbec01e7e..acaae61cc9 100644 --- a/plugins/catalog-backend/src/ingestion/types.ts +++ b/plugins/catalog-backend/src/ingestion/types.ts @@ -14,7 +14,12 @@ * limitations under the License. */ -import type { Entity, Location, LocationSpec } from '@backstage/catalog-model'; +import type { + Entity, + EntityRelationSpec, + Location, + LocationSpec, +} from '@backstage/catalog-model'; // // HigherOrderOperation @@ -47,6 +52,7 @@ export type LocationReader = { export type ReadLocationResult = { entities: ReadLocationEntity[]; + relations: { relation: EntityRelationSpec }[]; errors: ReadLocationError[]; }; diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index e6fb540f16..52f10232ce 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -52,6 +52,7 @@ import { GithubReaderProcessor, GitlabApiReaderProcessor, GitlabReaderProcessor, + GroupPopulatorProcessor, HigherOrderOperation, HigherOrderOperations, LocationReaders, @@ -334,6 +335,7 @@ export class CatalogBuilder { new YamlProcessor(), new CodeOwnersProcessor({ reader }), new LocationRefProcessor(), + new GroupPopulatorProcessor(), new AnnotateLocationEntityProcessor(), ]; From e26201d9d24a103b95ccdd4445ba65985ce30ecc Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 20 Oct 2020 17:57:11 +0200 Subject: [PATCH 2/8] catalog-backend: relations working e2e Co-authored-by: blam --- .../src/kinds/GroupEntityV1alpha1.ts | 30 ++--- .../src/kinds/UserEntityV1alpha1.ts | 10 +- .../src/catalog/DatabaseEntitiesCatalog.ts | 117 ++++++++++++------ plugins/catalog-backend/src/catalog/types.ts | 21 ++-- .../ingestion/HigherOrderOperations.test.ts | 14 +-- .../src/ingestion/HigherOrderOperations.ts | 22 ++-- .../src/ingestion/LocationReaders.ts | 54 ++++++-- .../src/ingestion/processors/types.ts | 1 + .../catalog-backend/src/ingestion/types.ts | 2 +- .../src/service/router.test.ts | 3 - plugins/catalog-backend/src/service/router.ts | 9 +- 11 files changed, 176 insertions(+), 107 deletions(-) diff --git a/packages/catalog-model/src/kinds/GroupEntityV1alpha1.ts b/packages/catalog-model/src/kinds/GroupEntityV1alpha1.ts index f0e91ddce8..8d1fbe78e8 100644 --- a/packages/catalog-model/src/kinds/GroupEntityV1alpha1.ts +++ b/packages/catalog-model/src/kinds/GroupEntityV1alpha1.ts @@ -32,21 +32,21 @@ const schema = yup.object>({ // one element and there is no simple workaround -_- // the cast is there to convince typescript that the array itself is // required without using .required() - ancestors: yup.array(yup.string().required()).test({ - name: 'isDefined', - message: 'ancestors must be defined', - test: v => Boolean(v), - }) as yup.ArraySchema, - children: yup.array(yup.string().required()).test({ - name: 'isDefined', - message: 'children must be defined', - test: v => Boolean(v), - }) as yup.ArraySchema, - descendants: yup.array(yup.string().required()).test({ - name: 'isDefined', - message: 'descendants must be defined', - test: v => Boolean(v), - }) as yup.ArraySchema, + // ancestors: yup.array(yup.string().required()).test({ + // name: 'isDefined', + // message: 'ancestors must be defined', + // test: v => Boolean(v), + // }) as yup.ArraySchema, + // children: yup.array(yup.string().required()).test({ + // name: 'isDefined', + // message: 'children must be defined', + // test: v => Boolean(v), + // }) as yup.ArraySchema, + // descendants: yup.array(yup.string().required()).test({ + // name: 'isDefined', + // message: 'descendants must be defined', + // test: v => Boolean(v), + // }) as yup.ArraySchema, }) .required(), }); diff --git a/packages/catalog-model/src/kinds/UserEntityV1alpha1.ts b/packages/catalog-model/src/kinds/UserEntityV1alpha1.ts index 6086b3776e..293f87b319 100644 --- a/packages/catalog-model/src/kinds/UserEntityV1alpha1.ts +++ b/packages/catalog-model/src/kinds/UserEntityV1alpha1.ts @@ -37,11 +37,11 @@ const schema = yup.object>({ // element and there is no simple workaround -_- // the cast is there to convince typescript that the array itself is // required without using .required() - memberOf: yup.array(yup.string().required()).test({ - name: 'isDefined', - message: 'memberOf must be defined', - test: v => Boolean(v), - }) as yup.ArraySchema, + // memberOf: yup.array(yup.string().required()).test({ + // name: 'isDefined', + // message: 'memberOf must be defined', + // test: v => Boolean(v), + // }) as yup.ArraySchema, }) .required(), }); diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts index efb541727a..e4accf9a8d 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts @@ -29,7 +29,11 @@ import limiterFactory from 'p-limit'; import { Logger } from 'winston'; import type { Database, DbEntityResponse, EntityFilters } from '../database'; import { durationText } from '../util/timing'; -import type { EntitiesCatalog } from './types'; +import type { + EntitiesCatalog, + EntityMutationRequest, + EntityMutationResponse, +} from './types'; type BatchContext = { kind: string; @@ -63,7 +67,7 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { return items.map(i => i.entity); } - async addOrUpdateEntity( + private async addOrUpdateEntity( entity: Entity, locationId?: string, ): Promise { @@ -96,15 +100,6 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { }); } - async addEntities(entities: Entity[], locationId?: string): Promise { - await this.database.transaction(async tx => { - await this.database.addEntities( - tx, - entities.map(entity => ({ locationId, entity })), - ); - }); - } - async removeEntityByUid(uid: string): Promise { return await this.database.transaction(async tx => { const entityResponse = await this.database.entityByUid(tx, uid); @@ -138,27 +133,31 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { * @param entities Some entities * @param locationId The location that they all belong to */ - async batchAddOrUpdateEntities(entities: Entity[], locationId?: string) { + async batchAddOrUpdateEntities( + requests: EntityMutationRequest[], + locationId?: string, + ): Promise { // Group the entities by unique kind+namespace combinations - const entitiesByKindAndNamespace = groupBy(entities, entity => { + const entitiesByKindAndNamespace = groupBy(requests, ({ entity }) => { const name = getEntityName(entity); return `${name.kind}:${name.namespace}`.toLowerCase(); }); const limiter = limiterFactory(BATCH_CONCURRENCY); - const tasks: Promise[] = []; + const tasks: Promise[] = []; - for (const groupEntities of Object.values(entitiesByKindAndNamespace)) { - const { kind, namespace } = getEntityName(groupEntities[0]); + for (const groupRequests of Object.values(entitiesByKindAndNamespace)) { + const { kind, namespace } = getEntityName(groupRequests[0].entity); // Go through the new entities in reasonable chunk sizes (sometimes, // sources produce tens of thousands of entities, and those are too large // batch sizes to reasonably send to the database) - for (const batch of chunk(groupEntities, BATCH_SIZE)) { + for (const batch of chunk(groupRequests, BATCH_SIZE)) { tasks.push( limiter(async () => { - const first = serializeEntityRef(batch[0]); - const last = serializeEntityRef(batch[batch.length - 1]); + const first = serializeEntityRef(batch[0].entity); + const last = serializeEntityRef(batch[batch.length - 1].entity); + const modifiedEntityIds: EntityMutationResponse[] = []; this.logger.debug( `Considering batch ${first}-${last} (${batch.length} entries)`, ); @@ -171,8 +170,16 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { batch, context, ); - if (toAdd.length) await this.batchAdd(toAdd, context); - if (toUpdate.length) await this.batchUpdate(toUpdate, context); + if (toAdd.length) { + modifiedEntityIds.push( + ...(await this.batchAdd(toAdd, context)), + ); + } + if (toUpdate.length) { + modifiedEntityIds.push( + ...(await this.batchUpdate(toUpdate, context)), + ); + } break; } catch (e) { if (e instanceof ConflictError && attempt < BATCH_ATTEMPTS) { @@ -184,16 +191,18 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { } } } + + return modifiedEntityIds; }), ); } } - await Promise.all(tasks); + return (await Promise.all(tasks)).flat(); } // Set the relations originating from an entity using the DB layer - async setRelations( + private async setRelations( originatingEntityId: string, relations: EntityRelationSpec[], ): Promise { @@ -207,15 +216,15 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { // produce the list of entities to be added, and the list of entities to be // updated private async analyzeBatch( - newEntities: Entity[], + requests: EntityMutationRequest[], { kind, namespace }: BatchContext, ): Promise<{ - toAdd: Entity[]; - toUpdate: Entity[]; + toAdd: EntityMutationRequest[]; + toUpdate: EntityMutationRequest[]; }> { const markTimestamp = process.hrtime(); - const names = newEntities.map(e => e.metadata.name); + const names = requests.map(({ entity }) => entity.metadata.name); const oldEntities = await this.entities({ kind: kind, 'metadata.namespace': namespace, @@ -226,18 +235,19 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { oldEntities.map(e => [e.metadata.name, e]), ); - const toAdd: Entity[] = []; - const toUpdate: Entity[] = []; + const toAdd: EntityMutationRequest[] = []; + const toUpdate: EntityMutationRequest[] = []; - for (const newEntity of newEntities) { + for (const request of requests) { + const newEntity = request.entity; const oldEntity = oldEntitiesByName.get(newEntity.metadata.name); if (!oldEntity) { - toAdd.push(newEntity); + toAdd.push(request); } else if (entityHasChanges(oldEntity, newEntity)) { // TODO(freben): This currently uses addOrUpdateEntity under the hood, // but should probably calculate the end result entity right here // instead and call a dedicated batch update database method instead - toUpdate.push(newEntity); + toUpdate.push(request); } } @@ -252,28 +262,55 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { // Efficiently adds the given entities to storage, under the assumption that // they do not conflict with any existing entities - private async batchAdd(entities: Entity[], { locationId }: BatchContext) { + private async batchAdd( + requests: EntityMutationRequest[], + { locationId }: BatchContext, + ): Promise { const markTimestamp = process.hrtime(); - await this.addEntities(entities, locationId); + const res = await this.database.transaction( + async tx => + await this.database.addEntities( + tx, + requests.map(({ entity }) => ({ locationId, entity })), + ), + ); + + const entityIds = res.map(({ entity }) => ({ + entityId: entity.metadata.uid!, + })); + + for (const [index, { entityId }] of entityIds.entries()) { + await this.setRelations(entityId, requests[index].relations); + } this.logger.debug( - `Added ${entities.length} entities in ${durationText(markTimestamp)}`, + `Added ${requests.length} entities in ${durationText(markTimestamp)}`, ); + + return entityIds; } // Efficiently updates the given entities into storage, under the assumption // that there already exist entities with the same names - private async batchUpdate(entities: Entity[], { locationId }: BatchContext) { + private async batchUpdate( + requests: EntityMutationRequest[], + { locationId }: BatchContext, + ): Promise { const markTimestamp = process.hrtime(); - + const responseIds: EntityMutationResponse[] = []; // TODO(freben): Still not batched - for (const entity of entities) { - await this.addOrUpdateEntity(entity, locationId); + for (const entity of requests) { + const res = await this.addOrUpdateEntity(entity.entity, locationId); + const entityId = res.metadata.uid!; + responseIds.push({ entityId }); + await this.setRelations(entityId, entity.relations); } this.logger.debug( - `Updated ${entities.length} entities in ${durationText(markTimestamp)}`, + `Updated ${requests.length} entities in ${durationText(markTimestamp)}`, ); + + return responseIds; } } diff --git a/plugins/catalog-backend/src/catalog/types.ts b/plugins/catalog-backend/src/catalog/types.ts index e50b5f15db..c9c514c42f 100644 --- a/plugins/catalog-backend/src/catalog/types.ts +++ b/plugins/catalog-backend/src/catalog/types.ts @@ -21,10 +21,17 @@ import type { EntityFilters } from '../database'; // Entities // +export type EntityMutationRequest = { + entity: Entity; + relations: EntityRelationSpec[]; +}; + +export type EntityMutationResponse = { + entityId: string; +}; + export type EntitiesCatalog = { entities(filters?: EntityFilters): Promise; - addOrUpdateEntity(entity: Entity, locationId?: string): Promise; - addEntities(entities: Entity[], locationId?: string): Promise; removeEntityByUid(uid: string): Promise; /** @@ -34,15 +41,9 @@ export type EntitiesCatalog = { * @param locationId The location that they all belong to */ batchAddOrUpdateEntities( - entities: Entity[], + entities: EntityMutationRequest[], locationId?: string, - ): Promise; - - // Same as the DB layer - setRelations( - entityUid: string, - relations: EntityRelationSpec[], - ): Promise; + ): Promise; }; // diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts index c7c8e738c8..b8aaa5fb13 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts @@ -31,10 +31,7 @@ describe('HigherOrderOperations', () => { beforeAll(() => { entitiesCatalog = { entities: jest.fn(), - addOrUpdateEntity: jest.fn(), - addEntities: jest.fn(), removeEntityByUid: jest.fn(), - setRelations: jest.fn(), batchAddOrUpdateEntities: jest.fn(), }; locationsCatalog = { @@ -72,7 +69,6 @@ describe('HigherOrderOperations', () => { locationReader.read.mockResolvedValue({ entities: [], errors: [], - relations: [], }); const result = await higherOrderOperation.addLocation(spec); @@ -116,7 +112,6 @@ describe('HigherOrderOperations', () => { locationReader.read.mockResolvedValue({ entities: [], errors: [], - relations: [], }); const result = await higherOrderOperation.addLocation(spec); @@ -144,9 +139,8 @@ describe('HigherOrderOperations', () => { locationsCatalog.locations.mockResolvedValue([]); locationReader.read.mockResolvedValue({ - entities: [{ entity, location }], + entities: [{ entity, location, relations: [] }], errors: [{ error: new Error('abcd'), location }], - relations: [], }); await expect(higherOrderOperation.addLocation(spec)).rejects.toThrow( @@ -193,9 +187,8 @@ describe('HigherOrderOperations', () => { { currentStatus: locationStatus, data: location }, ]); locationReader.read.mockResolvedValue({ - entities: [{ entity: desc, location }], + entities: [{ entity: desc, location, relations: [] }], errors: [], - relations: [], }); entitiesCatalog.batchAddOrUpdateEntities.mockResolvedValue(undefined); @@ -239,9 +232,8 @@ describe('HigherOrderOperations', () => { { currentStatus: locationStatus, data: location }, ]); locationReader.read.mockResolvedValue({ - entities: [{ entity: desc, location }], + entities: [{ entity: desc, location, relations: [] }], errors: [], - relations: [], }); entitiesCatalog.entities.mockResolvedValue([]); entitiesCatalog.addEntities.mockResolvedValue(undefined); diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts index 6df6d9b4b7..2771945db2 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts @@ -15,7 +15,7 @@ */ import { InputError } from '@backstage/backend-common'; -import { Entity, Location, LocationSpec } from '@backstage/catalog-model'; +import { Location, LocationSpec } from '@backstage/catalog-model'; import { v4 as uuidv4 } from 'uuid'; import { Logger } from 'winston'; import { EntitiesCatalog, LocationsCatalog } from '../catalog'; @@ -94,16 +94,16 @@ export class HigherOrderOperations implements HigherOrderOperation { if (!previousLocation) { await this.locationsCatalog.addLocation(location); } - const outputEntities: Entity[] = []; - for (const entity of readerOutput.entities) { - const out = await this.entitiesCatalog.addOrUpdateEntity( - entity.entity, - location.id, - ); - outputEntities.push(out); - } + const writtenEntities = await this.entitiesCatalog.batchAddOrUpdateEntities( + readerOutput.entities, + location.id, + ); - return { location, entities: outputEntities }; + const entities = await this.entitiesCatalog.entities({ + 'metadata.uid': writtenEntities.map(e => e.entityId), + }); + + return { location, entities }; } /** @@ -166,7 +166,7 @@ export class HigherOrderOperations implements HigherOrderOperation { try { await this.entitiesCatalog.batchAddOrUpdateEntities( - readerOutput.entities.map(e => e.entity), + readerOutput.entities, location.id, ); } catch (e) { diff --git a/plugins/catalog-backend/src/ingestion/LocationReaders.ts b/plugins/catalog-backend/src/ingestion/LocationReaders.ts index 597a8ad6a3..215bfc9dc1 100644 --- a/plugins/catalog-backend/src/ingestion/LocationReaders.ts +++ b/plugins/catalog-backend/src/ingestion/LocationReaders.ts @@ -18,6 +18,7 @@ import { UrlReader } from '@backstage/backend-common'; import { Entity, EntityPolicy, + EntityRelationSpec, ENTITY_DEFAULT_NAMESPACE, LocationSpec, } from '@backstage/catalog-model'; @@ -64,7 +65,6 @@ export class LocationReaders implements LocationReader { const output: ReadLocationResult = { entities: [], errors: [], - relations: [], }; let items: CatalogProcessorResult[] = [result.location(location, false)]; @@ -79,11 +79,21 @@ export class LocationReaders implements LocationReader { await this.handleData(item, emit); } else if (item.type === 'entity') { if (rulesEnforcer.isAllowed(item.entity, item.location)) { - const entity = await this.handleEntity(item, emit); + const relations = Array(); + + const entity = await this.handleEntity(item, emitResult => { + if (emitResult.type === 'relation') { + relations.push(emitResult.relation); + return; + } + emit(emitResult); + }); + if (entity) { output.entities.push({ entity, location: item.location, + relations, }); } } else { @@ -100,10 +110,6 @@ export class LocationReaders implements LocationReader { location: item.location, error: item.error, }); - } else if (item.type === 'relation') { - output.relations.push({ - relation: item.relation, - }); } } @@ -126,11 +132,23 @@ export class LocationReaders implements LocationReader { ) { const { processors, logger } = this.options; + const validatedEmit: CatalogProcessorEmit = emitResult => { + if (emitResult.type === 'relation') { + throw new Error('readLocation may not emit entity relations'); + } + + emit(emitResult); + }; + for (const processor of processors) { if (processor.readLocation) { try { if ( - await processor.readLocation(item.location, item.optional, emit) + await processor.readLocation( + item.location, + item.optional, + validatedEmit, + ) ) { return; } @@ -153,10 +171,20 @@ export class LocationReaders implements LocationReader { ) { const { processors, logger } = this.options; + const validatedEmit: CatalogProcessorEmit = emitResult => { + if (emitResult.type === 'relation') { + throw new Error('parseData may not emit entity relations'); + } + + emit(emitResult); + }; + for (const processor of processors) { if (processor.parseData) { try { - if (await processor.parseData(item.data, item.location, emit)) { + if ( + await processor.parseData(item.data, item.location, validatedEmit) + ) { return; } } catch (e) { @@ -250,10 +278,18 @@ export class LocationReaders implements LocationReader { `Encountered error at location ${item.location.type} ${item.location.target}, ${item.error}`, ); + const validatedEmit: CatalogProcessorEmit = emitResult => { + if (emitResult.type === 'relation') { + throw new Error('handleError may not emit entity relations'); + } + + emit(emitResult); + }; + for (const processor of processors) { if (processor.handleError) { try { - await processor.handleError(item.error, item.location, emit); + await processor.handleError(item.error, item.location, validatedEmit); } catch (e) { const message = `Processor ${processor.constructor.name} threw an error while handling another error at ${item.location.type} ${item.location.target}, ${e}`; emit(result.generalError(item.location, message)); diff --git a/plugins/catalog-backend/src/ingestion/processors/types.ts b/plugins/catalog-backend/src/ingestion/processors/types.ts index 9ed5e6e92d..7821a61437 100644 --- a/plugins/catalog-backend/src/ingestion/processors/types.ts +++ b/plugins/catalog-backend/src/ingestion/processors/types.ts @@ -120,6 +120,7 @@ export type CatalogProcessorEntityResult = { export type CatalogProcessorRelationResult = { type: 'relation'; relation: EntityRelationSpec; + entityRef?: string; }; export type CatalogProcessorErrorResult = { diff --git a/plugins/catalog-backend/src/ingestion/types.ts b/plugins/catalog-backend/src/ingestion/types.ts index acaae61cc9..c586e2d438 100644 --- a/plugins/catalog-backend/src/ingestion/types.ts +++ b/plugins/catalog-backend/src/ingestion/types.ts @@ -52,13 +52,13 @@ export type LocationReader = { export type ReadLocationResult = { entities: ReadLocationEntity[]; - relations: { relation: EntityRelationSpec }[]; errors: ReadLocationError[]; }; export type ReadLocationEntity = { location: LocationSpec; entity: Entity; + relations: EntityRelationSpec[]; }; export type ReadLocationError = { diff --git a/plugins/catalog-backend/src/service/router.test.ts b/plugins/catalog-backend/src/service/router.test.ts index a2b17ba360..71caeb3d6c 100644 --- a/plugins/catalog-backend/src/service/router.test.ts +++ b/plugins/catalog-backend/src/service/router.test.ts @@ -32,10 +32,7 @@ describe('createRouter', () => { beforeAll(async () => { entitiesCatalog = { entities: jest.fn(), - addOrUpdateEntity: jest.fn(), - addEntities: jest.fn(), removeEntityByUid: jest.fn(), - setRelations: jest.fn(), batchAddOrUpdateEntities: jest.fn(), }; locationsCatalog = { diff --git a/plugins/catalog-backend/src/service/router.ts b/plugins/catalog-backend/src/service/router.ts index a262e9b5f4..5af773f1a0 100644 --- a/plugins/catalog-backend/src/service/router.ts +++ b/plugins/catalog-backend/src/service/router.ts @@ -49,8 +49,13 @@ export async function createRouter( }) .post('/entities', async (req, res) => { const body = await requireRequestBody(req); - const result = await entitiesCatalog.addOrUpdateEntity(body as Entity); - res.status(200).send(result); + const [result] = await entitiesCatalog.batchAddOrUpdateEntities([ + { entity: body as Entity, relations: [] }, + ]); + const entity = await entitiesCatalog.entities({ + 'metadata.uid': result.entityId, + }); + res.status(200).send(entity); }) .get('/entities/by-uid/:uid', async (req, res) => { const { uid } = req.params; From fdff44b72814511376d2413da6450bbb50980b4c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 20 Oct 2020 19:32:34 +0200 Subject: [PATCH 3/8] 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); From 26371bba78c695bae1561d31ee715e6239685a08 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 21 Oct 2020 12:35:04 +0200 Subject: [PATCH 4/8] catalog-backend: revert GroupPopulatorProc, replace with OwnerRelationProc + relation consts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: blam Co-authored-by: Fredrik Adelöw --- dev-components.yaml | 38 ----------- .../src/kinds/GroupEntityV1alpha1.ts | 30 ++++----- .../src/kinds/UserEntityV1alpha1.ts | 10 +-- packages/catalog-model/src/kinds/index.ts | 1 + packages/catalog-model/src/kinds/relations.ts | 55 +++++++++++++++ ...Processor.ts => OwnerRelationProcessor.ts} | 67 +++++++++---------- .../src/ingestion/processors/index.ts | 2 +- .../src/service/CatalogBuilder.ts | 4 +- 8 files changed, 110 insertions(+), 97 deletions(-) delete mode 100644 dev-components.yaml create mode 100644 packages/catalog-model/src/kinds/relations.ts rename plugins/catalog-backend/src/ingestion/processors/{GroupPopulatorProcessor.ts => OwnerRelationProcessor.ts} (52%) diff --git a/dev-components.yaml b/dev-components.yaml deleted file mode 100644 index 1671f6057f..0000000000 --- a/dev-components.yaml +++ /dev/null @@ -1,38 +0,0 @@ -apiVersion: backstage.io/v1alpha1 -kind: Group -metadata: - name: root -spec: - type: org ---- -apiVersion: backstage.io/v1alpha1 -kind: Group -metadata: - name: platform -spec: - type: mission - parent: root -# --- -# apiVersion: backstage.io/v1alpha1 -# kind: Group -# metadata: -# name: pdx -# spec: -# type: tribe -# parent: platform -# --- -# apiVersion: backstage.io/v1alpha1 -# kind: Group -# metadata: -# name: tools -# spec: -# type: squad -# parent: pdx -# --- -# apiVersion: backstage.io/v1alpha1 -# kind: Group -# metadata: -# name: pulp-fiction -# spec: -# type: squad -# parent: pdx diff --git a/packages/catalog-model/src/kinds/GroupEntityV1alpha1.ts b/packages/catalog-model/src/kinds/GroupEntityV1alpha1.ts index 8d1fbe78e8..f0e91ddce8 100644 --- a/packages/catalog-model/src/kinds/GroupEntityV1alpha1.ts +++ b/packages/catalog-model/src/kinds/GroupEntityV1alpha1.ts @@ -32,21 +32,21 @@ const schema = yup.object>({ // one element and there is no simple workaround -_- // the cast is there to convince typescript that the array itself is // required without using .required() - // ancestors: yup.array(yup.string().required()).test({ - // name: 'isDefined', - // message: 'ancestors must be defined', - // test: v => Boolean(v), - // }) as yup.ArraySchema, - // children: yup.array(yup.string().required()).test({ - // name: 'isDefined', - // message: 'children must be defined', - // test: v => Boolean(v), - // }) as yup.ArraySchema, - // descendants: yup.array(yup.string().required()).test({ - // name: 'isDefined', - // message: 'descendants must be defined', - // test: v => Boolean(v), - // }) as yup.ArraySchema, + ancestors: yup.array(yup.string().required()).test({ + name: 'isDefined', + message: 'ancestors must be defined', + test: v => Boolean(v), + }) as yup.ArraySchema, + children: yup.array(yup.string().required()).test({ + name: 'isDefined', + message: 'children must be defined', + test: v => Boolean(v), + }) as yup.ArraySchema, + descendants: yup.array(yup.string().required()).test({ + name: 'isDefined', + message: 'descendants must be defined', + test: v => Boolean(v), + }) as yup.ArraySchema, }) .required(), }); diff --git a/packages/catalog-model/src/kinds/UserEntityV1alpha1.ts b/packages/catalog-model/src/kinds/UserEntityV1alpha1.ts index 293f87b319..6086b3776e 100644 --- a/packages/catalog-model/src/kinds/UserEntityV1alpha1.ts +++ b/packages/catalog-model/src/kinds/UserEntityV1alpha1.ts @@ -37,11 +37,11 @@ const schema = yup.object>({ // element and there is no simple workaround -_- // the cast is there to convince typescript that the array itself is // required without using .required() - // memberOf: yup.array(yup.string().required()).test({ - // name: 'isDefined', - // message: 'memberOf must be defined', - // test: v => Boolean(v), - // }) as yup.ArraySchema, + memberOf: yup.array(yup.string().required()).test({ + name: 'isDefined', + message: 'memberOf must be defined', + test: v => Boolean(v), + }) as yup.ArraySchema, }) .required(), }); diff --git a/packages/catalog-model/src/kinds/index.ts b/packages/catalog-model/src/kinds/index.ts index 6df4f1212a..4a3faf977b 100644 --- a/packages/catalog-model/src/kinds/index.ts +++ b/packages/catalog-model/src/kinds/index.ts @@ -44,3 +44,4 @@ export type { UserEntityV1alpha1 as UserEntity, UserEntityV1alpha1, } from './UserEntityV1alpha1'; +export * from './relations'; diff --git a/packages/catalog-model/src/kinds/relations.ts b/packages/catalog-model/src/kinds/relations.ts new file mode 100644 index 0000000000..3964da99f9 --- /dev/null +++ b/packages/catalog-model/src/kinds/relations.ts @@ -0,0 +1,55 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/* +Naming rules for relations in priority order: + +1. Use at most two words. One main verb and a specifier, e.g. "ownerOf" +2. Reading out " " should make sense in English. +3. Maintain symmetry between pairs, e.g. "ownedBy" and "ownerOf" rather than "owns". +*/ + +/** + * An ownership relation where the owner is usually an organizational + * entity (user or group), and the other entity can be anything. + */ +export const RELATION_OWNED_BY = 'ownedBy'; +export const RELATION_OWNER_OF = 'ownerOf'; + +/** + * A relation with an API entity, typically from a component or system + */ +export const RELATION_IMPLEMENTED_BY = 'implementedBy'; +export const RELATION_IMPLEMENTS = 'implements'; + +/** + * A relation denoting a dependency on another entity. + */ +export const RELATION_DEPENDS_ON = 'dependsOn'; +export const RELATION_DEPENDENCY_OF = 'dependencyOf'; + +/** + * A parent/child relation to build up a tree, used for example to describe + * the organizational structure between groups. + */ +export const RELATION_PARENT_OF = 'parentOf'; +export const RELATION_CHILD_OF = 'childOf'; + +/** + * A membership relation, typically for users in a group. + */ +export const RELATION_MEMBER_OF = 'memberOf'; +export const RELATION_HAS_MEMBER = 'hasMember'; diff --git a/plugins/catalog-backend/src/ingestion/processors/GroupPopulatorProcessor.ts b/plugins/catalog-backend/src/ingestion/processors/OwnerRelationProcessor.ts similarity index 52% rename from plugins/catalog-backend/src/ingestion/processors/GroupPopulatorProcessor.ts rename to plugins/catalog-backend/src/ingestion/processors/OwnerRelationProcessor.ts index a1b0c6f15e..62a02207f1 100644 --- a/plugins/catalog-backend/src/ingestion/processors/GroupPopulatorProcessor.ts +++ b/plugins/catalog-backend/src/ingestion/processors/OwnerRelationProcessor.ts @@ -17,66 +17,61 @@ import { Entity, ENTITY_DEFAULT_NAMESPACE, - GroupEntity, LocationSpec, + parseEntityRef, + ApiEntityV1alpha1, + ComponentEntityV1alpha1, + RELATION_OWNED_BY, + RELATION_OWNER_OF, } from '@backstage/catalog-model'; import { CatalogProcessor, CatalogProcessorEmit } from './types'; import * as result from './results'; -export class GroupPopulatorProcessor implements CatalogProcessor { +const includedKinds = new Set(['api', 'component']); + +export class OwnerRelationProcessor implements CatalogProcessor { async postProcessEntity( entity: Entity, _location: LocationSpec, emit: CatalogProcessorEmit, ): Promise { - if (entity.kind.toLowerCase() !== 'group') { + if (!includedKinds.has(entity.kind.toLowerCase())) { return entity; } - const spec = entity.spec as GroupEntity['spec']; + const apiOrComponentEntity = entity as + | ApiEntityV1alpha1 + | ComponentEntityV1alpha1; - const { parent, children } = spec; - const self = { - kind: entity.kind, - name: entity.metadata.name, - namespace: entity.metadata.namespace ?? ENTITY_DEFAULT_NAMESPACE, - }; + const owner = apiOrComponentEntity.spec?.owner; + if (owner) { + const namespace = entity.metadata.namespace ?? ENTITY_DEFAULT_NAMESPACE; + + const selfRef = { + kind: entity.kind, + name: entity.metadata.name, + namespace, + }; + const ownerRef = parseEntityRef(owner, { + defaultKind: 'group', + defaultNamespace: namespace, + }); - if (parent) { emit( result.relation({ - type: 'parent', - source: self, - target: { ...self, name: parent }, + type: RELATION_OWNED_BY, + source: selfRef, + target: ownerRef, }), ); emit( result.relation({ - type: 'child', - source: { ...self, name: parent }, - target: self, + type: RELATION_OWNER_OF, + source: ownerRef, + target: selfRef, }), ); } - if (children) { - for (const child of children) { - emit( - result.relation({ - type: 'child', - source: self, - target: { ...self, name: child }, - }), - ); - emit( - result.relation({ - type: 'parent', - source: { ...self, name: child }, - target: self, - }), - ); - } - } - return entity; } } diff --git a/plugins/catalog-backend/src/ingestion/processors/index.ts b/plugins/catalog-backend/src/ingestion/processors/index.ts index 2a09247e45..92303f04a3 100644 --- a/plugins/catalog-backend/src/ingestion/processors/index.ts +++ b/plugins/catalog-backend/src/ingestion/processors/index.ts @@ -28,7 +28,7 @@ export { GithubOrgReaderProcessor } from './GithubOrgReaderProcessor'; export { GithubReaderProcessor } from './GithubReaderProcessor'; export { GitlabApiReaderProcessor } from './GitlabApiReaderProcessor'; export { GitlabReaderProcessor } from './GitlabReaderProcessor'; -export { GroupPopulatorProcessor } from './GroupPopulatorProcessor'; +export { OwnerRelationProcessor } from './OwnerRelationProcessor'; export { LocationRefProcessor } from './LocationEntityProcessor'; export { PlaceholderProcessor } from './PlaceholderProcessor'; export type { PlaceholderResolver } from './PlaceholderProcessor'; diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index 52f10232ce..4ec2a0834d 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -52,7 +52,7 @@ import { GithubReaderProcessor, GitlabApiReaderProcessor, GitlabReaderProcessor, - GroupPopulatorProcessor, + OwnerRelationProcessor, HigherOrderOperation, HigherOrderOperations, LocationReaders, @@ -335,7 +335,7 @@ export class CatalogBuilder { new YamlProcessor(), new CodeOwnersProcessor({ reader }), new LocationRefProcessor(), - new GroupPopulatorProcessor(), + new OwnerRelationProcessor(), new AnnotateLocationEntityProcessor(), ]; From 27f87526c0bf5d595c5a3f3a642428d5f759a160 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 21 Oct 2020 14:11:26 +0200 Subject: [PATCH 5/8] catalog-backend: un-flaky flaky DatabaseEntitiesCatalog test Co-authored-by: blam --- .../src/catalog/DatabaseEntitiesCatalog.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts index 58cac90b7d..3e3a5a1972 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts @@ -273,7 +273,7 @@ describe('DatabaseEntitiesCatalog', () => { getVoidLogger(), ); const entities: EntityMutationRequest[] = []; - for (let i = 0; i < 500; ++i) { + for (let i = 0; i < 200; ++i) { entities.push({ entity: { apiVersion: 'a', @@ -286,23 +286,23 @@ describe('DatabaseEntitiesCatalog', () => { await catalog.batchAddOrUpdateEntities(entities); const afterFirst = await catalog.entities(); - expect(afterFirst.length).toBe(500); + expect(afterFirst.length).toBe(200); entities[40].entity.metadata.op = 'changed'; entities.push({ entity: { apiVersion: 'a', kind: 'k', - metadata: { name: `n500`, op: 'added' }, + metadata: { name: `n200`, op: 'added' }, }, relations: [], }); await catalog.batchAddOrUpdateEntities(entities); const afterSecond = await catalog.entities(); - expect(afterSecond.length).toBe(501); + expect(afterSecond.length).toBe(201); expect(afterSecond.find(e => e.metadata.op === 'changed')).toBeDefined(); expect(afterSecond.find(e => e.metadata.op === 'added')).toBeDefined(); - }); + }, 10000); }); }); From a7478736bdafd13929adf6032ce82e814eafdf95 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 21 Oct 2020 14:15:27 +0200 Subject: [PATCH 6/8] catalog-backend: review fixups from #2970 Co-authored-by: blam --- packages/catalog-model/src/entity/Entity.ts | 3 ++- .../migrations/20201019130742_add_relations_table.js | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/catalog-model/src/entity/Entity.ts b/packages/catalog-model/src/entity/Entity.ts index 9e7ece4122..ac43137546 100644 --- a/packages/catalog-model/src/entity/Entity.ts +++ b/packages/catalog-model/src/entity/Entity.ts @@ -143,13 +143,14 @@ export type EntityRelation = { }; /** - * Holds the relationship data for entities + * Holds the relation data for entities. */ export type EntityRelationSpec = { /** * The source entity of this relation. */ source: EntityName; + /** * The type of the relation. */ diff --git a/plugins/catalog-backend/migrations/20201019130742_add_relations_table.js b/plugins/catalog-backend/migrations/20201019130742_add_relations_table.js index accf9035e7..70150ff173 100644 --- a/plugins/catalog-backend/migrations/20201019130742_add_relations_table.js +++ b/plugins/catalog-backend/migrations/20201019130742_add_relations_table.js @@ -28,7 +28,7 @@ exports.up = async function up(knex) { .inTable('entities') .onDelete('CASCADE') .notNullable() - .comment('The originating entity of the relation'); + .comment('The entity that provided the relation'); table .string('source_full_name') .notNullable() From b2efa2020e7c527c5eaace68af027f1d834e8dd0 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 21 Oct 2020 17:23:11 +0200 Subject: [PATCH 7/8] catalog: use the ownerBy relation to display owners Co-authored-by: blam --- .../src/components/AboutCard/AboutCard.tsx | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/plugins/catalog/src/components/AboutCard/AboutCard.tsx b/plugins/catalog/src/components/AboutCard/AboutCard.tsx index 266d5fa843..be5d6b815b 100644 --- a/plugins/catalog/src/components/AboutCard/AboutCard.tsx +++ b/plugins/catalog/src/components/AboutCard/AboutCard.tsx @@ -14,7 +14,12 @@ * limitations under the License. */ -import { Entity, ENTITY_DEFAULT_NAMESPACE } from '@backstage/catalog-model'; +import { + Entity, + ENTITY_DEFAULT_NAMESPACE, + RELATION_OWNED_BY, + serializeEntityRef, +} from '@backstage/catalog-model'; import { Card, CardContent, @@ -139,7 +144,20 @@ export function AboutCard({ entity, variant }: AboutCardProps) { r.type === RELATION_OWNED_BY) + .map(({ target: { kind, name, namespace } }) => + // TODO(Rugvip): we want to provide some utils for this + serializeEntityRef({ + kind, + name, + namespace: + namespace === ENTITY_DEFAULT_NAMESPACE + ? undefined + : namespace, + }), + ) + .join(', ')} gridSizes={{ xs: 12, sm: 6, lg: 4 }} /> Date: Thu, 22 Oct 2020 17:36:56 +0200 Subject: [PATCH 8/8] catalog-backend: review fixes --- packages/catalog-model/src/kinds/relations.ts | 4 +-- .../catalog/DatabaseEntitiesCatalog.test.ts | 12 +++---- .../src/catalog/DatabaseEntitiesCatalog.ts | 36 +++++++++---------- plugins/catalog-backend/src/catalog/types.ts | 8 ++--- .../src/ingestion/HigherOrderOperations.ts | 4 --- .../processors/OwnerRelationProcessor.ts | 11 +++--- 6 files changed, 34 insertions(+), 41 deletions(-) diff --git a/packages/catalog-model/src/kinds/relations.ts b/packages/catalog-model/src/kinds/relations.ts index 3964da99f9..846313ec8c 100644 --- a/packages/catalog-model/src/kinds/relations.ts +++ b/packages/catalog-model/src/kinds/relations.ts @@ -32,8 +32,8 @@ export const RELATION_OWNER_OF = 'ownerOf'; /** * A relation with an API entity, typically from a component or system */ -export const RELATION_IMPLEMENTED_BY = 'implementedBy'; -export const RELATION_IMPLEMENTS = 'implements'; +export const RELATION_CONSUMES_API = 'consumesApi'; +export const RELATION_PROVIDES_API = 'providesApi'; /** * A relation denoting a dependency on another entity. diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts index 3e3a5a1972..a801015915 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts @@ -18,7 +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'; +import { EntityUpsertRequest } from './types'; describe('DatabaseEntitiesCatalog', () => { let db: jest.Mocked; @@ -272,8 +272,8 @@ describe('DatabaseEntitiesCatalog', () => { await DatabaseManager.createTestDatabase(), getVoidLogger(), ); - const entities: EntityMutationRequest[] = []; - for (let i = 0; i < 200; ++i) { + const entities: EntityUpsertRequest[] = []; + for (let i = 0; i < 300; ++i) { entities.push({ entity: { apiVersion: 'a', @@ -286,21 +286,21 @@ describe('DatabaseEntitiesCatalog', () => { await catalog.batchAddOrUpdateEntities(entities); const afterFirst = await catalog.entities(); - expect(afterFirst.length).toBe(200); + expect(afterFirst.length).toBe(300); entities[40].entity.metadata.op = 'changed'; entities.push({ entity: { apiVersion: 'a', kind: 'k', - metadata: { name: `n200`, op: 'added' }, + metadata: { name: `n300`, op: 'added' }, }, relations: [], }); await catalog.batchAddOrUpdateEntities(entities); const afterSecond = await catalog.entities(); - expect(afterSecond.length).toBe(201); + expect(afterSecond.length).toBe(301); expect(afterSecond.find(e => e.metadata.op === 'changed')).toBeDefined(); expect(afterSecond.find(e => e.metadata.op === 'added')).toBeDefined(); }, 10000); diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts index 18769cfdf7..6c3065aeaa 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts @@ -31,8 +31,8 @@ import type { Database, DbEntityResponse, EntityFilters } from '../database'; import { durationText } from '../util/timing'; import type { EntitiesCatalog, - EntityMutationRequest, - EntityMutationResponse, + EntityUpsertRequest, + EntityUpsertResponse, } from './types'; type BatchContext = { @@ -134,9 +134,9 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { * @param locationId The location that they all belong to */ async batchAddOrUpdateEntities( - requests: EntityMutationRequest[], + requests: EntityUpsertRequest[], locationId?: string, - ): Promise { + ): Promise { // Group the entities by unique kind+namespace combinations const entitiesByKindAndNamespace = groupBy(requests, ({ entity }) => { const name = getEntityName(entity); @@ -144,7 +144,7 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { }); const limiter = limiterFactory(BATCH_CONCURRENCY); - const tasks: Promise[] = []; + const tasks: Promise[] = []; for (const groupRequests of Object.values(entitiesByKindAndNamespace)) { const { kind, namespace } = getEntityName(groupRequests[0].entity); @@ -157,7 +157,7 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { limiter(async () => { const first = serializeEntityRef(batch[0].entity); const last = serializeEntityRef(batch[batch.length - 1].entity); - const modifiedEntityIds: EntityMutationResponse[] = []; + const modifiedEntityIds: EntityUpsertResponse[] = []; this.logger.debug( `Considering batch ${first}-${last} (${batch.length} entries)`, ); @@ -224,12 +224,12 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { // produce the list of entities to be added, and the list of entities to be // updated private async analyzeBatch( - requests: EntityMutationRequest[], + requests: EntityUpsertRequest[], { kind, namespace }: BatchContext, ): Promise<{ - toAdd: EntityMutationRequest[]; - toUpdate: EntityMutationRequest[]; - toIgnore: EntityMutationRequest[]; + toAdd: EntityUpsertRequest[]; + toUpdate: EntityUpsertRequest[]; + toIgnore: EntityUpsertRequest[]; }> { const markTimestamp = process.hrtime(); @@ -244,9 +244,9 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { oldEntities.map(e => [e.metadata.name, e]), ); - const toAdd: EntityMutationRequest[] = []; - const toUpdate: EntityMutationRequest[] = []; - const toIgnore: EntityMutationRequest[] = []; + const toAdd: EntityUpsertRequest[] = []; + const toUpdate: EntityUpsertRequest[] = []; + const toIgnore: EntityUpsertRequest[] = []; for (const request of requests) { const newEntity = request.entity; @@ -275,9 +275,9 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { // Efficiently adds the given entities to storage, under the assumption that // they do not conflict with any existing entities private async batchAdd( - requests: EntityMutationRequest[], + requests: EntityUpsertRequest[], { locationId }: BatchContext, - ): Promise { + ): Promise { const markTimestamp = process.hrtime(); const res = await this.database.transaction( @@ -306,11 +306,11 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { // Efficiently updates the given entities into storage, under the assumption // that there already exist entities with the same names private async batchUpdate( - requests: EntityMutationRequest[], + requests: EntityUpsertRequest[], { locationId }: BatchContext, - ): Promise { + ): Promise { const markTimestamp = process.hrtime(); - const responseIds: EntityMutationResponse[] = []; + const responseIds: EntityUpsertResponse[] = []; // TODO(freben): Still not batched for (const entity of requests) { const res = await this.addOrUpdateEntity(entity.entity, locationId); diff --git a/plugins/catalog-backend/src/catalog/types.ts b/plugins/catalog-backend/src/catalog/types.ts index c9c514c42f..4d821c428a 100644 --- a/plugins/catalog-backend/src/catalog/types.ts +++ b/plugins/catalog-backend/src/catalog/types.ts @@ -21,12 +21,12 @@ import type { EntityFilters } from '../database'; // Entities // -export type EntityMutationRequest = { +export type EntityUpsertRequest = { entity: Entity; relations: EntityRelationSpec[]; }; -export type EntityMutationResponse = { +export type EntityUpsertResponse = { entityId: string; }; @@ -41,9 +41,9 @@ export type EntitiesCatalog = { * @param locationId The location that they all belong to */ batchAddOrUpdateEntities( - entities: EntityMutationRequest[], + entities: EntityUpsertRequest[], locationId?: string, - ): Promise; + ): Promise; }; // diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts index 314b91cb13..44e11c551e 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts @@ -103,10 +103,6 @@ export class HigherOrderOperations implements HigherOrderOperation { 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/ingestion/processors/OwnerRelationProcessor.ts b/plugins/catalog-backend/src/ingestion/processors/OwnerRelationProcessor.ts index 62a02207f1..4c67589789 100644 --- a/plugins/catalog-backend/src/ingestion/processors/OwnerRelationProcessor.ts +++ b/plugins/catalog-backend/src/ingestion/processors/OwnerRelationProcessor.ts @@ -23,6 +23,7 @@ import { ComponentEntityV1alpha1, RELATION_OWNED_BY, RELATION_OWNER_OF, + getEntityName, } from '@backstage/catalog-model'; import { CatalogProcessor, CatalogProcessorEmit } from './types'; import * as result from './results'; @@ -46,11 +47,7 @@ export class OwnerRelationProcessor implements CatalogProcessor { if (owner) { const namespace = entity.metadata.namespace ?? ENTITY_DEFAULT_NAMESPACE; - const selfRef = { - kind: entity.kind, - name: entity.metadata.name, - namespace, - }; + const selfRef = getEntityName(entity); const ownerRef = parseEntityRef(owner, { defaultKind: 'group', defaultNamespace: namespace, @@ -58,15 +55,15 @@ export class OwnerRelationProcessor implements CatalogProcessor { emit( result.relation({ - type: RELATION_OWNED_BY, source: selfRef, + type: RELATION_OWNED_BY, target: ownerRef, }), ); emit( result.relation({ - type: RELATION_OWNER_OF, source: ownerRef, + type: RELATION_OWNER_OF, target: selfRef, }), );