Merge pull request #1334 from spotify/freben/entity-logic

chore(catalog-model): move shared entity model logic here

packages/catalog-model/package.json

@@ -20,8 +20,10 @@
     "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",
     "yup": "^0.28.5"
   },
   "devDependencies": {

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 = {
+export type EntityMeta = JsonObject & {
   /**
    * A globally unique ID for the entity.
    *
@@ -112,3 +114,8 @@ export type EntityMeta = {
    */
   annotations?: Record<string, string>;
 };
+
+/**
+ * The keys of EntityMeta that are auto-generated.
+ */
+export const entityMetaGeneratedFields = ['uid', 'etag', 'generation'] as const;

packages/catalog-model/src/entity/index.ts

@@ -14,5 +14,12 @@
  * limitations under the License.
  */
 
+export { entityMetaGeneratedFields } from './Entity';
 export type { Entity, EntityMeta } from './Entity';
 export * from './policies';
+export {
+  entityHasChanges,
+  generateEntityEtag,
+  generateEntityUid,
+  generateUpdatedEntity,
+} from './util';

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',
+      });
+    });
+  });
+});

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;
+}

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;
+        }
       }
     }
 

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);
   }
 

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

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);
     });
   });

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<Entity | undefined> {
-    return await this.database.transaction(tx =>
+    const response = await this.database.transaction(tx =>
       this.entityByNameInternal(tx, kind, name, namespace),
     );
+    return response?.entity;
   }
 
   async addOrUpdateEntity(
@@ -53,25 +57,30 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog {
     locationId?: string,
   ): Promise<Entity> {
     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<Entity | undefined> {
+  ): Promise<DbEntityResponse | undefined> {
     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;
   }
 }

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);
     });
   });

plugins/catalog-backend/src/database/CommonDatabase.ts

@@ -19,7 +19,14 @@ import {
   InputError,
   NotFoundError,
 } from '@backstage/backend-common';
-import { Entity, EntityMeta, Location } from '@backstage/catalog-model';
+import {
+  Entity,
+  EntityMeta,
+  entityMetaGeneratedFields,
+  generateEntityEtag,
+  generateEntityUid,
+  Location,
+} from '@backstage/catalog-model';
 import Knex from 'knex';
 import lodash from 'lodash';
 import { v4 as uuidv4 } from 'uuid';
@@ -38,88 +45,9 @@ 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));
-}
-
-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,
-  };
-}
-
-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, '');
-}
-
+/**
+ * The core database implementation.
+ */
 export class CommonDatabase implements Database {
   constructor(
     private readonly database: Knex,
@@ -163,12 +91,12 @@ 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,
     };
 
-    const newRow = toEntityRow(request.locationId, newEntity);
+    const newRow = this.toEntityRow(request.locationId, newEntity);
     await tx<DbEntitiesRow>('entities').insert(newRow);
     await this.updateEntitiesSearch(tx, newRow.id, newEntity);
 
@@ -178,35 +106,20 @@ export class CommonDatabase implements Database {
   async updateEntity(
     txOpaque: unknown,
     request: DbEntityRequest,
+    matchingEtag?: string,
+    matchingGeneration?: number,
   ): Promise<DbEntityResponse> {
     const tx = txOpaque as Knex.Transaction<any, any>;
 
-    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<DbEntitiesRow>;
-    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<DbEntitiesRow>('entities')
-      .where(entitySelector)
+      .where({ id: uid })
       .select();
     if (oldRows.length !== 1) {
       throw new NotFoundError('No matching entity found');
@@ -217,51 +130,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 = this.toEntityRow(request.locationId, request.entity);
     const updatedRows = await tx<DbEntitiesRow>('entities')
       .where({ id: oldRow.id, etag: oldRow.etag })
       .update(newRow);
@@ -271,8 +159,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(
@@ -328,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(
@@ -347,7 +236,7 @@ export class CommonDatabase implements Database {
       return undefined;
     }
 
-    return toEntityResponse(rows[0]);
+    return this.toEntityResponse(rows[0]);
   }
 
   async entityByUid(
@@ -362,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<void> {
@@ -524,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,
+    };
+  }
 }

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<DbEntityResponse>;
 
   entities(tx: unknown, filters?: EntityFilters): Promise<DbEntityResponse[]>;

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);
-  }
 }

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<ComponentProps<typeof RegisterComponentResultDialog>>,
@@ -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(