From 8d961027a90184776e004cd008c3ecb4b76fe597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Wed, 17 Jun 2020 11:25:21 +0200 Subject: [PATCH 1/2] chore(catalog-model): move shared entity model logic here --- packages/catalog-model/package.json | 1 + packages/catalog-model/src/entity/Entity.ts | 2 +- packages/catalog-model/src/entity/index.ts | 6 + .../catalog-model/src/entity/util.test.ts | 204 ++++++++++++++++++ packages/catalog-model/src/entity/util.ts | 140 ++++++++++++ .../catalog/DatabaseEntitiesCatalog.test.ts | 69 +++++- .../src/catalog/DatabaseEntitiesCatalog.ts | 49 +++-- .../src/database/CommonDatabase.test.ts | 38 +--- .../src/database/CommonDatabase.ts | 124 +++-------- plugins/catalog-backend/src/database/types.ts | 18 +- .../src/ingestion/HigherOrderOperations.ts | 66 +----- 11 files changed, 507 insertions(+), 210 deletions(-) create mode 100644 packages/catalog-model/src/entity/util.test.ts create mode 100644 packages/catalog-model/src/entity/util.ts diff --git a/packages/catalog-model/package.json b/packages/catalog-model/package.json index 64f49e34e6..a9bece27ab 100644 --- a/packages/catalog-model/package.json +++ b/packages/catalog-model/package.json @@ -22,6 +22,7 @@ "dependencies": { "@types/yup": "^0.28.2", "lodash": "^4.17.15", + "uuid": "^8.0.0", "yup": "^0.28.5" }, "devDependencies": { diff --git a/packages/catalog-model/src/entity/Entity.ts b/packages/catalog-model/src/entity/Entity.ts index 70d2ba4d2b..d4f10ab883 100644 --- a/packages/catalog-model/src/entity/Entity.ts +++ b/packages/catalog-model/src/entity/Entity.ts @@ -48,7 +48,7 @@ export type Entity = { * @see https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#objectmeta-v1-meta * @see https://kubernetes.io/docs/concepts/overview/working-with-objects/kubernetes-objects/ */ -export type EntityMeta = { +export type EntityMeta = Record & { /** * A globally unique ID for the entity. * diff --git a/packages/catalog-model/src/entity/index.ts b/packages/catalog-model/src/entity/index.ts index 9e96021336..a1aed856ff 100644 --- a/packages/catalog-model/src/entity/index.ts +++ b/packages/catalog-model/src/entity/index.ts @@ -16,3 +16,9 @@ export type { Entity, EntityMeta } from './Entity'; export * from './policies'; +export { + entityHasChanges, + generateEntityEtag, + generateEntityUid, + generateUpdatedEntity, +} from './util'; diff --git a/packages/catalog-model/src/entity/util.test.ts b/packages/catalog-model/src/entity/util.test.ts new file mode 100644 index 0000000000..e4399bbe05 --- /dev/null +++ b/packages/catalog-model/src/entity/util.test.ts @@ -0,0 +1,204 @@ +/* + * 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 lodash from 'lodash'; +import { + generateEntityEtag, + generateEntityUid, + entityHasChanges, + generateUpdatedEntity, +} from './util'; +import { Entity } from './Entity'; + +describe('util', () => { + describe('generateEntityUid', () => { + it('generates randomness', () => { + expect(generateEntityUid()).not.toEqual(''); + expect(generateEntityUid()).not.toEqual(generateEntityUid()); + }); + }); + + describe('generateEntityEtag', () => { + it('generates randomness', () => { + expect(generateEntityEtag()).not.toEqual(''); + expect(generateEntityEtag()).not.toEqual(generateEntityEtag()); + }); + }); + + describe('entityHasChanges', () => { + let a: Entity; + beforeEach(() => { + a = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'name', + custom: 'custom', + labels: { + labelKey: 'labelValue', + }, + annotations: { + annotationKey: 'annotationValue', + }, + }, + spec: { + a: 'a', + }, + }; + }); + + it('happy path: clone has no changes', () => { + const b = lodash.cloneDeep(a); + expect(entityHasChanges(a, b)).toBe(false); + }); + + it('detects root field changes', () => { + let b: any = lodash.cloneDeep(a); + b.apiVersion += 'a'; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + delete b.apiVersion; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + b.kind += 'a'; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + delete b.kind; + expect(entityHasChanges(a, b)).toBe(true); + }); + + it('detects metadata changes', () => { + let b: any = lodash.cloneDeep(a); + b.metadata.name += 'a'; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + delete b.metadata.custom; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + delete b.metadata.custom; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + b.metadata.labels.n = 'n'; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + b.metadata.labels.labelKey += 'a'; + expect(entityHasChanges(a, b)).toBe(true); + }); + + it('detects annotation changes, but not removals', () => { + let b: any = lodash.cloneDeep(a); + b.metadata.annotations.annotationKey += 'a'; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + b.metadata.annotations.n = 'n'; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + delete b.metadata.annotations.annotationKey; + expect(entityHasChanges(a, b)).toBe(false); + }); + + it('detects spec changes', () => { + let b: any = lodash.cloneDeep(a); + b.spec.a += 'a'; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + delete b.spec.a; + expect(entityHasChanges(a, b)).toBe(true); + b = lodash.cloneDeep(a); + b.spec.n = 'n'; + expect(entityHasChanges(a, b)).toBe(true); + }); + }); + + describe('generateUpdatedEntity', () => { + let a: Entity; + let b: any; + beforeEach(() => { + a = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + uid: 'da921f56-f655-4e6e-9b8b-bb19a57818d8', + etag: 'NzY5NDA5NzQtYmEwNC00MDY0LWFiYmItNTYxYzQxM2JhZDcx', + generation: 2, + name: 'name', + custom: 'custom', + labels: { + labelKey: 'labelValue', + }, + annotations: { + annotationKey: 'annotationValue', + }, + }, + spec: { + a: 'a', + }, + }; + b = lodash.cloneDeep(a); + delete b.metadata.uid; + delete b.metadata.etag; + delete b.metadata.generation; + }); + + it('happy path: running on itself leaves it unchanged', () => { + const result = generateUpdatedEntity(a, b); + expect(result).toEqual(a); + }); + + it('bumps etag and generation when spec is changed', () => { + b.spec.a += 'a'; + const result = generateUpdatedEntity(a, b); + expect(result.metadata.uid).toEqual(a.metadata.uid); + expect(result.metadata.etag).not.toEqual(a.metadata.etag); + expect(result.metadata.generation).toEqual(a.metadata.generation! + 1); + expect(result.spec).toEqual({ a: 'aa' }); + }); + + it('bumps only etag when other things than spec are changed', () => { + b.metadata.n = 'n'; + const result = generateUpdatedEntity(a, b); + expect(result.metadata.uid).toEqual(a.metadata.uid); + expect(result.metadata.etag).not.toEqual(a.metadata.etag); + expect(result.metadata.generation).toEqual(a.metadata.generation); + expect(result.metadata.n).toEqual('n'); + }); + + it('retains new annotations', () => { + b.metadata.annotations.annotationKey = 'changedValue'; + b.metadata.annotations.newKey = 'newValue'; + const result = generateUpdatedEntity(a, b); + expect(result.metadata.uid).toEqual(a.metadata.uid); + expect(result.metadata.etag).not.toEqual(a.metadata.etag); + expect(result.metadata.generation).toEqual(a.metadata.generation); + expect(result.metadata.annotations).toEqual({ + annotationKey: 'changedValue', + newKey: 'newValue', + }); + }); + + it('retains old annotations', () => { + b.metadata.annotations.newKey = 'newValue'; + const result = generateUpdatedEntity(a, b); + expect(result.metadata.uid).toEqual(a.metadata.uid); + expect(result.metadata.etag).not.toEqual(a.metadata.etag); + expect(result.metadata.generation).toEqual(a.metadata.generation); + expect(result.metadata.annotations).toEqual({ + annotationKey: 'annotationValue', + newKey: 'newValue', + }); + }); + }); +}); diff --git a/packages/catalog-model/src/entity/util.ts b/packages/catalog-model/src/entity/util.ts new file mode 100644 index 0000000000..2a196a65d8 --- /dev/null +++ b/packages/catalog-model/src/entity/util.ts @@ -0,0 +1,140 @@ +/* + * 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 lodash from 'lodash'; +import { v4 as uuidv4 } from 'uuid'; +import { Entity } from './Entity'; + +/** + * Generates a new random UID for an entity. + * + * @returns A string with enough randomness to uniquely identify an entity + */ +export function generateEntityUid(): string { + return uuidv4(); +} + +/** + * Generates a new random Etag for an entity. + * + * @returns A string with enough randomness to uniquely identify an entity + * revision + */ +export function generateEntityEtag(): string { + return Buffer.from(uuidv4(), 'utf8').toString('base64').replace(/[^\w]/g, ''); +} + +/** + * Checks whether there are any significant changes going from the previous to + * the next version of this entity. + * + * Significance, in this case, means that we do not compare generated fields + * such as uid, etag and generation, and we only check that no new annotations + * are added or existing annotations were changed (since they are effectively + * merged when doing updates). + * + * @param previous The old state of the entity + * @param next The new state of the entity + */ +export function entityHasChanges(previous: Entity, next: Entity): boolean { + if (entityHasAnnotationChanges(previous, next)) { + return true; + } + + const e1 = lodash.cloneDeep(previous); + const e2 = lodash.cloneDeep(next); + + if (!e1.metadata.labels) { + e1.metadata.labels = {}; + } + if (!e2.metadata.labels) { + e2.metadata.labels = {}; + } + + // Remove generated fields + delete e1.metadata.uid; + delete e1.metadata.etag; + delete e1.metadata.generation; + delete e2.metadata.uid; + delete e2.metadata.etag; + delete e2.metadata.generation; + + // Remove already compared things + delete e1.metadata.annotations; + delete e2.metadata.annotations; + + return !lodash.isEqual(e1, e2); +} + +/** + * Takes an old revision of an entity and a new desired state, and merges + * them into a complete new state. + * + * The previous revision is expected to be a complete model loaded from the + * catalog, including the uid, etag and generation fields. + * + * @param previous The old state of the entity + * @param next The new state of the entity + * @returns An entity with the merged state of both + */ +export function generateUpdatedEntity(previous: Entity, next: Entity): Entity { + const { uid, etag, generation } = previous.metadata; + if (!uid || !etag || !generation) { + throw new Error('Previous entity must have uid, etag and generation'); + } + + const result = lodash.cloneDeep(next); + + // Annotations are merged, with the new ones taking precedence + if (previous.metadata.annotations) { + next.metadata.annotations = { + ...previous.metadata.annotations, + ...next.metadata.annotations, + }; + } + + // Generated fields are copied and updated + const bumpEtag = entityHasChanges(previous, result); + const bumpGeneration = !lodash.isEqual(previous.spec, result.spec); + result.metadata.uid = uid; + result.metadata.etag = bumpEtag ? generateEntityEtag() : etag; + result.metadata.generation = bumpGeneration ? generation + 1 : generation; + + return result; +} + +function entityHasAnnotationChanges(previous: Entity, next: Entity): boolean { + // Since the next annotations get merged into the previous, extract only + // the overlapping keys and check if their values match. + if (next.metadata.annotations) { + if (!previous.metadata.annotations) { + return true; + } + if ( + !lodash.isEqual( + next.metadata.annotations, + lodash.pick( + previous.metadata.annotations, + Object.keys(next.metadata.annotations), + ), + ) + ) { + return true; + } + } + + return false; +} diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts index 30b480f348..5d35fe814e 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts @@ -62,6 +62,11 @@ describe('DatabaseEntitiesCatalog', () => { const result = await catalog.addOrUpdateEntity(entity); expect(db.entities).toHaveBeenCalledTimes(1); + expect(db.entities).toHaveBeenCalledWith(expect.anything(), [ + { key: 'kind', values: ['b'] }, + { key: 'name', values: ['c'] }, + { key: 'namespace', values: ['d'] }, + ]); expect(db.addEntity).toHaveBeenCalledTimes(1); expect(result).toBe(entity); }); @@ -71,20 +76,52 @@ describe('DatabaseEntitiesCatalog', () => { apiVersion: 'a', kind: 'b', metadata: { - uid: 'uuuu', + uid: 'u', name: 'c', namespace: 'd', }, }; - db.entities.mockResolvedValue([]); + db.entityByUid.mockResolvedValue({ + entity: { + apiVersion: 'a', + kind: 'b', + metadata: { + uid: 'u', + etag: 'e', + generation: 1, + name: 'c', + namespace: 'd', + }, + }, + }); db.updateEntity.mockResolvedValue({ entity }); const catalog = new DatabaseEntitiesCatalog(db); const result = await catalog.addOrUpdateEntity(entity); expect(db.entities).toHaveBeenCalledTimes(0); + expect(db.entityByUid).toHaveBeenCalledTimes(1); + expect(db.entityByUid).toHaveBeenCalledWith(expect.anything(), 'u'); expect(db.updateEntity).toHaveBeenCalledTimes(1); + expect(db.updateEntity).toHaveBeenCalledWith( + expect.anything(), + { + entity: { + apiVersion: 'a', + kind: 'b', + metadata: { + uid: 'u', + etag: 'e', + generation: 1, + name: 'c', + namespace: 'd', + }, + }, + }, + 'e', + 1, + ); expect(result).toBe(entity); }); @@ -101,19 +138,45 @@ describe('DatabaseEntitiesCatalog', () => { apiVersion: 'a', kind: 'b', metadata: { + uid: 'u', + etag: 'e', + generation: 1, name: 'c', namespace: 'd', }, }; db.entities.mockResolvedValue([{ entity: existing }]); - db.updateEntity.mockResolvedValue({ entity: added }); + db.updateEntity.mockResolvedValue({ entity: existing }); const catalog = new DatabaseEntitiesCatalog(db); const result = await catalog.addOrUpdateEntity(added); expect(db.entities).toHaveBeenCalledTimes(1); + expect(db.entities).toHaveBeenCalledWith(expect.anything(), [ + { key: 'kind', values: ['b'] }, + { key: 'name', values: ['c'] }, + { key: 'namespace', values: ['d'] }, + ]); expect(db.updateEntity).toHaveBeenCalledTimes(1); + expect(db.updateEntity).toHaveBeenCalledWith( + expect.anything(), + { + entity: { + apiVersion: 'a', + kind: 'b', + metadata: { + uid: 'u', + etag: 'e', + generation: 1, + name: 'c', + namespace: 'd', + }, + }, + }, + 'e', + 1, + ); expect(result).toEqual(existing); }); }); diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts index 7c4b964b1b..c7cc4249db 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts @@ -15,7 +15,10 @@ */ import { NotFoundError } from '@backstage/backend-common'; -import { LOCATION_ANNOTATION } from '@backstage/catalog-model'; +import { + generateUpdatedEntity, + LOCATION_ANNOTATION, +} from '@backstage/catalog-model'; import type { Entity } from '@backstage/catalog-model'; import type { Database, DbEntityResponse, EntityFilters } from '../database'; import type { EntitiesCatalog } from './types'; @@ -43,9 +46,10 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { namespace: string | undefined, name: string, ): Promise { - return await this.database.transaction(tx => + const response = await this.database.transaction(tx => this.entityByNameInternal(tx, kind, name, namespace), ); + return response ? response.entity : undefined; } async addOrUpdateEntity( @@ -53,25 +57,30 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { locationId?: string, ): Promise { return await this.database.transaction(async tx => { - let response: DbEntityResponse; + // Find a matching (by uid, or by compound name, depending on the given + // entity) existing entity, to know whether to update or add + const existing = entity.metadata.uid + ? await this.database.entityByUid(tx, entity.metadata.uid) + : await this.entityByNameInternal( + tx, + entity.kind, + entity.metadata.name, + entity.metadata.namespace, + ); - if (entity.metadata.uid) { - response = await this.database.updateEntity(tx, { locationId, entity }); - } else { - const existing = await this.entityByNameInternal( + // If it's an update, run the algorithm for annotation merging, updating + // etag/generation, etc. + let response: DbEntityResponse; + if (existing) { + const updated = generateUpdatedEntity(existing.entity, entity); + response = await this.database.updateEntity( tx, - entity.kind, - entity.metadata.name, - entity.metadata.namespace, + { locationId, entity: updated }, + existing.entity.metadata.etag, + existing.entity.metadata.generation, ); - if (existing) { - response = await this.database.updateEntity(tx, { - locationId, - entity, - }); - } else { - response = await this.database.addEntity(tx, { locationId, entity }); - } + } else { + response = await this.database.addEntity(tx, { locationId, entity }); } return response.entity; @@ -110,7 +119,7 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { kind: string, name: string, namespace: string | undefined, - ): Promise { + ): Promise { const matches = await this.database.entities(tx, [ { key: 'kind', values: [kind] }, { key: 'name', values: [name] }, @@ -123,6 +132,6 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { }, ]); - return matches.length ? matches[0].entity : undefined; + return matches.length ? matches[0] : undefined; } } diff --git a/plugins/catalog-backend/src/database/CommonDatabase.test.ts b/plugins/catalog-backend/src/database/CommonDatabase.test.ts index 169880e3c9..3d1455a886 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.test.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { ConflictError, NotFoundError } from '@backstage/backend-common'; +import { ConflictError } from '@backstage/backend-common'; import type { Entity, Location } from '@backstage/catalog-model'; import { DatabaseManager } from './DatabaseManager'; import { Database, DatabaseLocationUpdateLogStatus } from './types'; @@ -199,9 +199,7 @@ describe('CommonDatabase', () => { ); expect(updated.entity.apiVersion).toEqual(added.entity.apiVersion); expect(updated.entity.kind).toEqual(added.entity.kind); - expect(updated.entity.metadata.etag).not.toEqual( - added.entity.metadata.etag, - ); + expect(updated.entity.metadata.etag).toEqual(added.entity.metadata.etag); expect(updated.entity.metadata.generation).toEqual( added.entity.metadata.generation, ); @@ -220,41 +218,21 @@ describe('CommonDatabase', () => { expect(updated.entity.metadata.name).toEqual('new!'); }); - it('can update fields if kind, name, and namespace match', async () => { - const added = await db.transaction(tx => db.addEntity(tx, entityRequest)); - added.entity.apiVersion = 'something.new'; - delete added.entity.metadata.uid; - delete added.entity.metadata.generation; - const updated = await db.transaction(tx => - db.updateEntity(tx, { entity: added.entity }), - ); - expect(updated.entity.apiVersion).toEqual('something.new'); - }); - - it('rejects if kind, name, but not namespace match', async () => { - const added = await db.transaction(tx => db.addEntity(tx, entityRequest)); - added.entity.apiVersion = 'something.new'; - delete added.entity.metadata.uid; - delete added.entity.metadata.generation; - added.entity.metadata.namespace = 'something.wrong'; - await expect( - db.transaction(tx => db.updateEntity(tx, { entity: added.entity })), - ).rejects.toThrow(NotFoundError); - }); - it('fails to update an entity if etag does not match', async () => { const added = await db.transaction(tx => db.addEntity(tx, entityRequest)); - added.entity.metadata.etag = 'garbage'; await expect( - db.transaction(tx => db.updateEntity(tx, { entity: added.entity })), + db.transaction(tx => + db.updateEntity(tx, { entity: added.entity }, 'garbage'), + ), ).rejects.toThrow(ConflictError); }); it('fails to update an entity if generation does not match', async () => { const added = await db.transaction(tx => db.addEntity(tx, entityRequest)); - added.entity.metadata.generation! += 100; await expect( - db.transaction(tx => db.updateEntity(tx, { entity: added.entity })), + db.transaction(tx => + db.updateEntity(tx, { entity: added.entity }, undefined, 1e20), + ), ).rejects.toThrow(ConflictError); }); }); diff --git a/plugins/catalog-backend/src/database/CommonDatabase.ts b/plugins/catalog-backend/src/database/CommonDatabase.ts index 24e9fd3d5e..e986d25c34 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.ts @@ -19,7 +19,13 @@ import { InputError, NotFoundError, } from '@backstage/backend-common'; -import { Entity, EntityMeta, Location } from '@backstage/catalog-model'; +import { + Entity, + EntityMeta, + generateEntityEtag, + generateEntityUid, + Location, +} from '@backstage/catalog-model'; import Knex from 'knex'; import lodash from 'lodash'; import { v4 as uuidv4 } from 'uuid'; @@ -38,16 +44,12 @@ import type { EntityFilters, } from './types'; -function getStrippedMetadata(metadata: EntityMeta): EntityMeta { - const output = lodash.cloneDeep(metadata); - delete output.uid; - delete output.etag; - delete output.generation; - return output; -} - function serializeMetadata(metadata: EntityMeta): string { - return JSON.stringify(getStrippedMetadata(metadata)); + const withoutGeneratedFields = lodash.cloneDeep(metadata); + delete withoutGeneratedFields.uid; + delete withoutGeneratedFields.etag; + delete withoutGeneratedFields.generation; + return JSON.stringify(withoutGeneratedFields); } function serializeSpec(spec: Entity['spec']): DbEntitiesRow['spec'] { @@ -99,27 +101,6 @@ function toEntityResponse(row: DbEntitiesRow): DbEntityResponse { }; } -function specsAreEqual( - first: string | null, - second: object | undefined, -): boolean { - if (!first && !second) { - return true; - } else if (!first || !second) { - return false; - } - - return lodash.isEqual(JSON.parse(first), second); -} - -function generateUid(): string { - return uuidv4(); -} - -function generateEtag(): string { - return Buffer.from(uuidv4(), 'utf8').toString('base64').replace(/[^\w]/g, ''); -} - export class CommonDatabase implements Database { constructor( private readonly database: Knex, @@ -163,8 +144,8 @@ export class CommonDatabase implements Database { const newEntity = lodash.cloneDeep(request.entity); newEntity.metadata = { ...newEntity.metadata, - uid: generateUid(), - etag: generateEtag(), + uid: generateEntityUid(), + etag: generateEntityEtag(), generation: 1, }; @@ -178,35 +159,20 @@ export class CommonDatabase implements Database { async updateEntity( txOpaque: unknown, request: DbEntityRequest, + matchingEtag?: string, + matchingGeneration?: number, ): Promise { const tx = txOpaque as Knex.Transaction; - const { kind } = request.entity; - const { - uid, - etag: expectedOldEtag, - generation: expectedOldGeneration, - name, - namespace, - } = request.entity.metadata ?? {}; + const { uid } = request.entity.metadata; - // Find existing entities that match the given metadata - let entitySelector: Partial; - if (uid) { - entitySelector = { id: uid }; - } else if (kind && name) { - entitySelector = { - kind, - name: name, - namespace: namespace || null, - }; - } else { - throw new InputError( - 'Must specify either uid, or kind + name + namespace to be able to identify an entity', - ); + if (uid === undefined) { + throw new InputError('Must specify uid when updating entities'); } + + // Find existing entity const oldRows = await tx('entities') - .where(entitySelector) + .where({ id: uid }) .select(); if (oldRows.length !== 1) { throw new NotFoundError('No matching entity found'); @@ -217,51 +183,26 @@ export class CommonDatabase implements Database { // The Number cast is here because sqlite reads it as a string, no matter // what the table actually says oldRow.generation = Number(oldRow.generation); - if (expectedOldEtag) { - if (expectedOldEtag !== oldRow.etag) { + if (matchingEtag) { + if (matchingEtag !== oldRow.etag) { throw new ConflictError( - `Etag mismatch, expected="${expectedOldEtag}" found="${oldRow.etag}"`, + `Etag mismatch, expected="${matchingEtag}" found="${oldRow.etag}"`, ); } } - if (expectedOldGeneration) { - if (expectedOldGeneration !== oldRow.generation) { + if (matchingGeneration) { + if (matchingGeneration !== oldRow.generation) { throw new ConflictError( - `Generation mismatch, expected="${expectedOldGeneration}" found="${oldRow.generation}"`, + `Generation mismatch, expected="${matchingGeneration}" found="${oldRow.generation}"`, ); } } - // Build the new shape of the entity - const newEtag = generateEtag(); - const newGeneration = specsAreEqual(oldRow.spec, request.entity.spec) - ? oldRow.generation - : oldRow.generation + 1; - const newEntity = lodash.cloneDeep(request.entity); - newEntity.metadata = { - ...newEntity.metadata, - uid: oldRow.id, - etag: newEtag, - generation: newGeneration, - }; - - // Preserve annotations that were set on the old version of the entity, - // unless the new version overwrites them - if (oldRow.metadata) { - const oldMetadata = JSON.parse(oldRow.metadata) as EntityMeta; - if (oldMetadata.annotations) { - newEntity.metadata.annotations = { - ...oldMetadata.annotations, - ...newEntity.metadata.annotations, - }; - } - } - - await this.ensureNoSimilarNames(tx, newEntity); + await this.ensureNoSimilarNames(tx, request.entity); // Store the updated entity; select on the old etag to ensure that we do // not lose to another writer - const newRow = toEntityRow(request.locationId, newEntity); + const newRow = toEntityRow(request.locationId, request.entity); const updatedRows = await tx('entities') .where({ id: oldRow.id, etag: oldRow.etag }) .update(newRow); @@ -271,8 +212,9 @@ export class CommonDatabase implements Database { throw new ConflictError(`Failed to update entity`); } - await this.updateEntitiesSearch(tx, oldRow.id, newEntity); - return { locationId: request.locationId, entity: newEntity }; + await this.updateEntitiesSearch(tx, oldRow.id, request.entity); + + return request; } async entities( diff --git a/plugins/catalog-backend/src/database/types.ts b/plugins/catalog-backend/src/database/types.ts index fc81e124ba..0537b87800 100644 --- a/plugins/catalog-backend/src/database/types.ts +++ b/plugins/catalog-backend/src/database/types.ts @@ -104,21 +104,27 @@ export type Database = { /** * Updates an existing entity in the catalog. * - * The given entity must contain enough information to identify an already - * stored entity in the catalog - either by uid, or by kind + namespace + - * name. If no matching entity is found, the operation fails. + * The given entity must contain an uid to identify an already stored entity + * in the catalog. If it is missing or if no matching entity is found, the + * operation fails. * - * If etag or generation are given, they are taken into account. Attempts to - * update a matching entity, but where the etag and/or generation are not - * equal to the passed values, will fail. + * If matchingEtag or matchingGeneration are given, they are taken into + * account. Attempts to update a matching entity, but where the etag and/or + * generation are not equal to the passed values, will fail. * * @param tx An ongoing transaction * @param request The entity being updated + * @param matchingEtag If specified, reject with ConflictError if not + * matching the entry in the database + * @param matchingGeneration If specified, reject with ConflictError if not + * matching the entry in the database * @returns The updated entity */ updateEntity( tx: unknown, request: DbEntityRequest, + matchingEtag?: string, + matchingGeneration?: number, ): Promise; entities(tx: unknown, filters?: EntityFilters): Promise; diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts index 2080b1c878..0224dc7f1d 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts @@ -15,8 +15,12 @@ */ import { InputError } from '@backstage/backend-common'; -import { Entity, Location, LocationSpec } from '@backstage/catalog-model'; -import lodash from 'lodash'; +import { + Entity, + entityHasChanges, + Location, + LocationSpec, +} from '@backstage/catalog-model'; import { v4 as uuidv4 } from 'uuid'; import { Logger } from 'winston'; import { EntitiesCatalog, LocationsCatalog } from '../catalog'; @@ -173,7 +177,7 @@ export class HigherOrderOperations implements HigherOrderOperation { if (!previous) { this.logger.debug(`No such entity found, adding`); await this.entitiesCatalog.addOrUpdateEntity(entity, location.id); - } else if (!this.entitiesAreEqual(previous, entity)) { + } else if (entityHasChanges(previous, entity)) { this.logger.debug(`Different from existing entity, updating`); await this.entitiesCatalog.addOrUpdateEntity(entity, location.id); } else { @@ -199,60 +203,4 @@ export class HigherOrderOperations implements HigherOrderOperation { } } } - - // Compares entities, ignoring generated and irrelevant data - private entitiesAreEqual(previous: Entity, next: Entity): boolean { - if ( - previous.apiVersion !== next.apiVersion || - previous.kind !== next.kind || - !lodash.isEqual(previous.spec, next.spec) // Accept that {} !== undefined - ) { - return false; - } - - // Since the next annotations get merged into the previous, extract only - // the overlapping keys and check if their values match. - if (next.metadata.annotations) { - if (!previous.metadata.annotations) { - return false; - } - if ( - !lodash.isEqual( - next.metadata.annotations, - lodash.pick( - previous.metadata.annotations, - Object.keys(next.metadata.annotations), - ), - ) - ) { - return false; - } - } - - const e1 = lodash.cloneDeep(previous); - const e2 = lodash.cloneDeep(next); - - if (!e1.metadata.labels) { - e1.metadata.labels = {}; - } - if (!e2.metadata.labels) { - e2.metadata.labels = {}; - } - - // Remove generated fields - delete e1.metadata.uid; - delete e1.metadata.etag; - delete e1.metadata.generation; - delete e2.metadata.uid; - delete e2.metadata.etag; - delete e2.metadata.generation; - - // Remove already compared things - delete e1.metadata.annotations; - delete e1.spec; - delete e2.metadata.annotations; - delete e2.spec; - - return lodash.isEqual(e1, e2); - } } From 3bff1d6a683ed22330e49869cff295dfa973df21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 18 Jun 2020 11:54:12 +0200 Subject: [PATCH 2/2] address comments --- packages/catalog-model/package.json | 1 + packages/catalog-model/src/entity/Entity.ts | 11 +- packages/catalog-model/src/entity/index.ts | 1 + packages/config-loader/src/lib/reader.ts | 15 ++- packages/config-loader/src/lib/secrets.ts | 3 +- packages/config/src/types.ts | 2 +- .../src/catalog/DatabaseEntitiesCatalog.ts | 2 +- .../src/database/CommonDatabase.ts | 114 ++++++++---------- .../RegisterComponentResultDialog.test.tsx | 13 +- 9 files changed, 83 insertions(+), 79 deletions(-) diff --git a/packages/catalog-model/package.json b/packages/catalog-model/package.json index a9bece27ab..2516263e8a 100644 --- a/packages/catalog-model/package.json +++ b/packages/catalog-model/package.json @@ -20,6 +20,7 @@ "clean": "backstage-cli clean" }, "dependencies": { + "@backstage/config": "^0.1.1-alpha.9", "@types/yup": "^0.28.2", "lodash": "^4.17.15", "uuid": "^8.0.0", diff --git a/packages/catalog-model/src/entity/Entity.ts b/packages/catalog-model/src/entity/Entity.ts index d4f10ab883..4f245da8b7 100644 --- a/packages/catalog-model/src/entity/Entity.ts +++ b/packages/catalog-model/src/entity/Entity.ts @@ -14,6 +14,8 @@ * limitations under the License. */ +import { JsonObject } from '@backstage/config'; + /** * The format envelope that's common to all versions/kinds of entity. * @@ -39,7 +41,7 @@ export type Entity = { /** * The specification data describing the entity itself. */ - spec?: object; + spec?: JsonObject; }; /** @@ -48,7 +50,7 @@ export type Entity = { * @see https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#objectmeta-v1-meta * @see https://kubernetes.io/docs/concepts/overview/working-with-objects/kubernetes-objects/ */ -export type EntityMeta = Record & { +export type EntityMeta = JsonObject & { /** * A globally unique ID for the entity. * @@ -112,3 +114,8 @@ export type EntityMeta = Record & { */ annotations?: Record; }; + +/** + * The keys of EntityMeta that are auto-generated. + */ +export const entityMetaGeneratedFields = ['uid', 'etag', 'generation'] as const; diff --git a/packages/catalog-model/src/entity/index.ts b/packages/catalog-model/src/entity/index.ts index a1aed856ff..380f5458cc 100644 --- a/packages/catalog-model/src/entity/index.ts +++ b/packages/catalog-model/src/entity/index.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +export { entityMetaGeneratedFields } from './Entity'; export type { Entity, EntityMeta } from './Entity'; export * from './policies'; export { diff --git a/packages/config-loader/src/lib/reader.ts b/packages/config-loader/src/lib/reader.ts index 4efaf1986a..584d0e0591 100644 --- a/packages/config-loader/src/lib/reader.ts +++ b/packages/config-loader/src/lib/reader.ts @@ -14,11 +14,11 @@ * limitations under the License. */ -import yaml from 'yaml'; +import { AppConfig, JsonObject, JsonValue } from '@backstage/config'; import { basename } from 'path'; -import { isObject } from './utils'; -import { JsonValue, JsonObject, AppConfig } from '@backstage/config'; +import yaml from 'yaml'; import { ReaderContext } from './types'; +import { isObject } from './utils'; /** * Reads and parses, and validates, and transforms a single config file. @@ -67,9 +67,12 @@ export async function readConfigFile( const out: JsonObject = {}; for (const [key, value] of Object.entries(obj)) { - const result = await transform(value, `${path}.${key}`); - if (result !== undefined) { - out[key] = result; + // undefined covers optional fields + if (value !== undefined) { + const result = await transform(value, `${path}.${key}`); + if (result !== undefined) { + out[key] = result; + } } } diff --git a/packages/config-loader/src/lib/secrets.ts b/packages/config-loader/src/lib/secrets.ts index c4157120ae..348a41db58 100644 --- a/packages/config-loader/src/lib/secrets.ts +++ b/packages/config-loader/src/lib/secrets.ts @@ -122,7 +122,7 @@ export async function readSecret( const { path } = secret; const parts = typeof path === 'string' ? path.split('.') : path; - let value: JsonValue = await parser(content); + let value: JsonValue | undefined = await parser(content); for (const [index, part] of parts.entries()) { if (!isObject(value)) { const errPath = parts.slice(0, index).join('.'); @@ -132,6 +132,7 @@ export async function readSecret( } value = value[part]; } + return String(value); } diff --git a/packages/config/src/types.ts b/packages/config/src/types.ts index aaadcd70dc..3e46874c1d 100644 --- a/packages/config/src/types.ts +++ b/packages/config/src/types.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -export type JsonObject = { [key in string]: JsonValue }; +export type JsonObject = { [key in string]?: JsonValue }; export type JsonArray = JsonValue[]; export type JsonValue = | JsonObject diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts index c7cc4249db..d6809e468f 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts @@ -49,7 +49,7 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { const response = await this.database.transaction(tx => this.entityByNameInternal(tx, kind, name, namespace), ); - return response ? response.entity : undefined; + return response?.entity; } async addOrUpdateEntity( diff --git a/plugins/catalog-backend/src/database/CommonDatabase.ts b/plugins/catalog-backend/src/database/CommonDatabase.ts index e986d25c34..c860f52d1e 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.ts @@ -22,6 +22,7 @@ import { import { Entity, EntityMeta, + entityMetaGeneratedFields, generateEntityEtag, generateEntityUid, Location, @@ -44,63 +45,9 @@ import type { EntityFilters, } from './types'; -function serializeMetadata(metadata: EntityMeta): string { - const withoutGeneratedFields = lodash.cloneDeep(metadata); - delete withoutGeneratedFields.uid; - delete withoutGeneratedFields.etag; - delete withoutGeneratedFields.generation; - return JSON.stringify(withoutGeneratedFields); -} - -function serializeSpec(spec: Entity['spec']): DbEntitiesRow['spec'] { - if (!spec) { - return null; - } - - return JSON.stringify(spec); -} - -function toEntityRow( - locationId: string | undefined, - entity: Entity, -): DbEntitiesRow { - return { - id: entity.metadata.uid!, - location_id: locationId || null, - etag: entity.metadata.etag!, - generation: entity.metadata.generation!, - api_version: entity.apiVersion, - kind: entity.kind, - name: entity.metadata.name, - namespace: entity.metadata.namespace || null, - metadata: serializeMetadata(entity.metadata), - spec: serializeSpec(entity.spec), - }; -} - -function toEntityResponse(row: DbEntitiesRow): DbEntityResponse { - const entity: Entity = { - apiVersion: row.api_version, - kind: row.kind, - metadata: { - ...(JSON.parse(row.metadata) as Entity['metadata']), - uid: row.id, - etag: row.etag, - generation: Number(row.generation), // cast because of sqlite - }, - }; - - if (row.spec) { - const spec = JSON.parse(row.spec); - entity.spec = spec; - } - - return { - locationId: row.location_id || undefined, - entity, - }; -} - +/** + * The core database implementation. + */ export class CommonDatabase implements Database { constructor( private readonly database: Knex, @@ -149,7 +96,7 @@ export class CommonDatabase implements Database { generation: 1, }; - const newRow = toEntityRow(request.locationId, newEntity); + const newRow = this.toEntityRow(request.locationId, newEntity); await tx('entities').insert(newRow); await this.updateEntitiesSearch(tx, newRow.id, newEntity); @@ -202,7 +149,7 @@ export class CommonDatabase implements Database { // Store the updated entity; select on the old etag to ensure that we do // not lose to another writer - const newRow = toEntityRow(request.locationId, request.entity); + const newRow = this.toEntityRow(request.locationId, request.entity); const updatedRows = await tx('entities') .where({ id: oldRow.id, etag: oldRow.etag }) .update(newRow); @@ -270,7 +217,7 @@ export class CommonDatabase implements Database { .orderBy('name', 'asc') .groupBy('id'); - return rows.map(row => toEntityResponse(row)); + return rows.map(row => this.toEntityResponse(row)); } async entity( @@ -289,7 +236,7 @@ export class CommonDatabase implements Database { return undefined; } - return toEntityResponse(rows[0]); + return this.toEntityResponse(rows[0]); } async entityByUid( @@ -304,7 +251,7 @@ export class CommonDatabase implements Database { return undefined; } - return toEntityResponse(rows[0]); + return this.toEntityResponse(rows[0]); } async removeEntity(txOpaque: unknown, uid: string): Promise { @@ -466,4 +413,47 @@ export class CommonDatabase implements Database { } } } + + private toEntityRow( + locationId: string | undefined, + entity: Entity, + ): DbEntitiesRow { + return { + id: entity.metadata.uid!, + location_id: locationId || null, + etag: entity.metadata.etag!, + generation: entity.metadata.generation!, + api_version: entity.apiVersion, + kind: entity.kind, + name: entity.metadata.name, + namespace: entity.metadata.namespace || null, + metadata: JSON.stringify( + lodash.omit(entity.metadata, ...entityMetaGeneratedFields), + ), + spec: entity.spec ? JSON.stringify(entity.spec) : null, + }; + } + + private toEntityResponse(row: DbEntitiesRow): DbEntityResponse { + const entity: Entity = { + apiVersion: row.api_version, + kind: row.kind, + metadata: { + ...(JSON.parse(row.metadata) as EntityMeta), + uid: row.id, + etag: row.etag, + generation: Number(row.generation), // cast because of sqlite + }, + }; + + if (row.spec) { + const spec = JSON.parse(row.spec); + entity.spec = spec; + } + + return { + locationId: row.location_id || undefined, + entity, + }; + } } diff --git a/plugins/register-component/src/components/RegisterComponentResultDialog/RegisterComponentResultDialog.test.tsx b/plugins/register-component/src/components/RegisterComponentResultDialog/RegisterComponentResultDialog.test.tsx index d918e72489..71933a0302 100644 --- a/plugins/register-component/src/components/RegisterComponentResultDialog/RegisterComponentResultDialog.test.tsx +++ b/plugins/register-component/src/components/RegisterComponentResultDialog/RegisterComponentResultDialog.test.tsx @@ -14,13 +14,12 @@ * limitations under the License. */ -import React, { ComponentProps } from 'react'; -import { render, cleanup } from '@testing-library/react'; -import { RegisterComponentResultDialog } from './RegisterComponentResultDialog'; -import { ThemeProvider } from '@material-ui/core'; import { lightTheme } from '@backstage/theme'; +import { ThemeProvider } from '@material-ui/core'; +import { cleanup, render } from '@testing-library/react'; +import React, { ComponentProps } from 'react'; import { MemoryRouter } from 'react-router-dom'; -import { Entity } from '@backstage/catalog-model'; +import { RegisterComponentResultDialog } from './RegisterComponentResultDialog'; const setup = ( props?: Partial>, @@ -52,6 +51,7 @@ it('should show a list of components if success', async () => { const { rendered } = setup({ entities: [ { + apiVersion: 'backstage.io/v1alpha1', kind: 'Component', metadata: { name: 'Component1', @@ -61,6 +61,7 @@ it('should show a list of components if success', async () => { }, }, { + apiVersion: 'backstage.io/v1alpha1', kind: 'Component', metadata: { name: 'Component2', @@ -69,7 +70,7 @@ it('should show a list of components if success', async () => { type: 'service', }, }, - ] as Entity[], + ], }); expect(