From 7de5e10408368b20da45f6b650e8420e342ba017 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Fri, 12 Jun 2020 11:37:26 +0200 Subject: [PATCH 1/2] feat(catalog-backend): reject almost-same-name entities --- packages/backend/src/plugins/catalog.ts | 2 +- .../catalog/DatabaseLocationsCatalog.test.ts | 19 +-- .../src/database/CommonDatabase.test.ts | 161 ++++++++---------- .../src/database/CommonDatabase.ts | 12 +- .../src/database/DatabaseManager.ts | 38 ++++- .../20200612102317_unique_normalized.ts | 51 ++++++ plugins/catalog-backend/src/database/types.ts | 3 + .../src/service/standaloneServer.ts | 2 +- 8 files changed, 168 insertions(+), 120 deletions(-) create mode 100644 plugins/catalog-backend/src/database/migrations/20200612102317_unique_normalized.ts diff --git a/packages/backend/src/plugins/catalog.ts b/packages/backend/src/plugins/catalog.ts index 8d8ac5b47b..c472623c3d 100644 --- a/packages/backend/src/plugins/catalog.ts +++ b/packages/backend/src/plugins/catalog.ts @@ -31,7 +31,7 @@ export default async function createPlugin({ }: PluginEnvironment) { const locationReader = new LocationReaders(logger); - const db = await DatabaseManager.createDatabase(database, logger); + const db = await DatabaseManager.createDatabase(database, { logger }); const entitiesCatalog = new DatabaseEntitiesCatalog(db); const locationsCatalog = new DatabaseLocationsCatalog(db); const higherOrderOperation = new HigherOrderOperations( diff --git a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts index 907bdea2d5..8c7897fa2c 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts @@ -14,29 +14,14 @@ * limitations under the License. */ -import { getVoidLogger } from '@backstage/backend-common'; -import Knex from 'knex'; -import path from 'path'; -import { CommonDatabase } from '../database'; +import { DatabaseManager } from '../database'; import { DatabaseLocationsCatalog } from './DatabaseLocationsCatalog'; describe('DatabaseLocationsCatalog', () => { let catalog: DatabaseLocationsCatalog; beforeEach(async () => { - const knex = Knex({ - client: 'sqlite3', - connection: ':memory:', - useNullAsDefault: true, - }); - knex.client.pool.on('createSuccess', (_eventId: any, resource: any) => { - resource.run('PRAGMA foreign_keys = ON', () => {}); - }); - await knex.migrate.latest({ - directory: path.resolve(__dirname, '../database/migrations'), - loadExtensions: ['.ts'], - }); - const db = new CommonDatabase(knex, getVoidLogger()); + const db = await DatabaseManager.createTestDatabase(); catalog = new DatabaseLocationsCatalog(db); }); diff --git a/plugins/catalog-backend/src/database/CommonDatabase.test.ts b/plugins/catalog-backend/src/database/CommonDatabase.test.ts index 1802ea5e7d..169880e3c9 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.test.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.test.ts @@ -14,16 +14,10 @@ * limitations under the License. */ -import { - ConflictError, - getVoidLogger, - NotFoundError, -} from '@backstage/backend-common'; +import { ConflictError, NotFoundError } from '@backstage/backend-common'; import type { Entity, Location } from '@backstage/catalog-model'; -import Knex from 'knex'; -import path from 'path'; -import { CommonDatabase } from './CommonDatabase'; -import { DatabaseLocationUpdateLogStatus } from './types'; +import { DatabaseManager } from './DatabaseManager'; +import { Database, DatabaseLocationUpdateLogStatus } from './types'; import type { DbEntityRequest, DbEntityResponse, @@ -31,22 +25,12 @@ import type { } from './types'; describe('CommonDatabase', () => { - let knex: Knex; + let db: Database; let entityRequest: DbEntityRequest; let entityResponse: DbEntityResponse; beforeEach(async () => { - knex = Knex({ - client: 'sqlite3', - connection: ':memory:', - useNullAsDefault: true, - }); - - await knex.raw('PRAGMA foreign_keys = ON'); - await knex.migrate.latest({ - directory: path.resolve(__dirname, 'migrations'), - loadExtensions: ['.ts'], - }); + db = await DatabaseManager.createTestDatabase(); entityRequest = { entity: { @@ -84,7 +68,6 @@ describe('CommonDatabase', () => { }); it('manages locations', async () => { - const db = new CommonDatabase(knex, getVoidLogger()); const input: Location = { id: 'dd12620d-0436-422f-93bd-929aa0788123', type: 'a', @@ -115,55 +98,76 @@ describe('CommonDatabase', () => { describe('addEntity', () => { it('happy path: adds entity to empty database', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); - const added = await catalog.transaction(tx => - catalog.addEntity(tx, entityRequest), - ); + const added = await db.transaction(tx => db.addEntity(tx, entityRequest)); expect(added).toStrictEqual(entityResponse); expect(added.entity.metadata.generation).toBe(1); }); it('rejects adding the same-named entity twice', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); - await catalog.transaction(tx => catalog.addEntity(tx, entityRequest)); + await db.transaction(tx => db.addEntity(tx, entityRequest)); await expect( - catalog.transaction(tx => catalog.addEntity(tx, entityRequest)), + db.transaction(tx => db.addEntity(tx, entityRequest)), + ).rejects.toThrow(ConflictError); + }); + + it('rejects adding the almost-same-kind entity twice', async () => { + entityRequest.entity.kind = 'some-kind'; + await db.transaction(tx => db.addEntity(tx, entityRequest)); + entityRequest.entity.kind = 'SomeKind'; + await expect( + db.transaction(tx => db.addEntity(tx, entityRequest)), + ).rejects.toThrow(ConflictError); + }); + + it('rejects adding the almost-same-named entity twice', async () => { + entityRequest.entity.metadata.name = 'some-name'; + await db.transaction(tx => db.addEntity(tx, entityRequest)); + entityRequest.entity.metadata.name = 'SomeName'; + await expect( + db.transaction(tx => db.addEntity(tx, entityRequest)), + ).rejects.toThrow(ConflictError); + }); + + it('rejects adding the almost-same-namespace entity twice', async () => { + entityRequest.entity.metadata.namespace = undefined; + await db.transaction(tx => db.addEntity(tx, entityRequest)); + entityRequest.entity.metadata.namespace = ''; + await expect( + db.transaction(tx => db.addEntity(tx, entityRequest)), ).rejects.toThrow(ConflictError); }); it('accepts adding the same-named entity twice if on different namespaces', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); entityRequest.entity.metadata.namespace = 'namespace1'; - await catalog.transaction(tx => catalog.addEntity(tx, entityRequest)); + await db.transaction(tx => db.addEntity(tx, entityRequest)); entityRequest.entity.metadata.namespace = 'namespace2'; await expect( - catalog.transaction(tx => catalog.addEntity(tx, entityRequest)), + db.transaction(tx => db.addEntity(tx, entityRequest)), ).resolves.toBeDefined(); }); }); describe('locationHistory', () => { it('outputs the history correctly', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); const location: Location = { id: 'dd12620d-0436-422f-93bd-929aa0788123', type: 'a', target: 'b', }; - await catalog.addLocation(location); + await db.addLocation(location); - await catalog.addLocationUpdateLogEvent( + await db.addLocationUpdateLogEvent( 'dd12620d-0436-422f-93bd-929aa0788123', DatabaseLocationUpdateLogStatus.SUCCESS, ); - await catalog.addLocationUpdateLogEvent( + await db.addLocationUpdateLogEvent( 'dd12620d-0436-422f-93bd-929aa0788123', DatabaseLocationUpdateLogStatus.FAIL, undefined, 'Something went wrong', ); - const result = await catalog.locationHistory( + const result = await db.locationHistory( 'dd12620d-0436-422f-93bd-929aa0788123', ); expect(result).toEqual([ @@ -189,12 +193,9 @@ describe('CommonDatabase', () => { describe('updateEntity', () => { it('can read and no-op-update an entity', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); - const added = await catalog.transaction(tx => - catalog.addEntity(tx, entityRequest), - ); - const updated = await catalog.transaction(tx => - catalog.updateEntity(tx, { entity: added.entity }), + const added = await db.transaction(tx => db.addEntity(tx, entityRequest)); + const updated = await db.transaction(tx => + db.updateEntity(tx, { entity: added.entity }), ); expect(updated.entity.apiVersion).toEqual(added.entity.apiVersion); expect(updated.entity.kind).toEqual(added.entity.kind); @@ -211,77 +212,55 @@ describe('CommonDatabase', () => { }); it('can update name if uid matches', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); - const added = await catalog.transaction(tx => - catalog.addEntity(tx, entityRequest), - ); + const added = await db.transaction(tx => db.addEntity(tx, entityRequest)); added.entity.metadata.name! = 'new!'; - const updated = await catalog.transaction(tx => - catalog.updateEntity(tx, { entity: added.entity }), + const updated = await db.transaction(tx => + db.updateEntity(tx, { entity: added.entity }), ); expect(updated.entity.metadata.name).toEqual('new!'); }); it('can update fields if kind, name, and namespace match', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); - const added = await catalog.transaction(tx => - catalog.addEntity(tx, entityRequest), - ); + 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 catalog.transaction(tx => - catalog.updateEntity(tx, { entity: added.entity }), + 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 catalog = new CommonDatabase(knex, getVoidLogger()); - const added = await catalog.transaction(tx => - catalog.addEntity(tx, entityRequest), - ); + 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( - catalog.transaction(tx => - catalog.updateEntity(tx, { entity: added.entity }), - ), + 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 catalog = new CommonDatabase(knex, getVoidLogger()); - const added = await catalog.transaction(tx => - catalog.addEntity(tx, entityRequest), - ); + const added = await db.transaction(tx => db.addEntity(tx, entityRequest)); added.entity.metadata.etag = 'garbage'; await expect( - catalog.transaction(tx => - catalog.updateEntity(tx, { entity: added.entity }), - ), + db.transaction(tx => db.updateEntity(tx, { entity: added.entity })), ).rejects.toThrow(ConflictError); }); it('fails to update an entity if generation does not match', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); - const added = await catalog.transaction(tx => - catalog.addEntity(tx, entityRequest), - ); + const added = await db.transaction(tx => db.addEntity(tx, entityRequest)); added.entity.metadata.generation! += 100; await expect( - catalog.transaction(tx => - catalog.updateEntity(tx, { entity: added.entity }), - ), + db.transaction(tx => db.updateEntity(tx, { entity: added.entity })), ).rejects.toThrow(ConflictError); }); }); describe('entities', () => { it('can get all entities with empty filters list', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); const e1: Entity = { apiVersion: 'a', kind: 'k1', @@ -293,13 +272,11 @@ describe('CommonDatabase', () => { metadata: { name: 'n' }, spec: { c: null }, }; - await catalog.transaction(async tx => { - await catalog.addEntity(tx, { entity: e1 }); - await catalog.addEntity(tx, { entity: e2 }); + await db.transaction(async tx => { + await db.addEntity(tx, { entity: e1 }); + await db.addEntity(tx, { entity: e2 }); }); - const result = await catalog.transaction(async tx => - catalog.entities(tx, []), - ); + const result = await db.transaction(async tx => db.entities(tx, [])); expect(result.length).toEqual(2); expect(result).toEqual( expect.arrayContaining([ @@ -316,7 +293,6 @@ describe('CommonDatabase', () => { }); it('can get all specific entities for matching filters (naive case)', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); const entities: Entity[] = [ { apiVersion: 'a', kind: 'k1', metadata: { name: 'n' } }, { @@ -333,15 +309,15 @@ describe('CommonDatabase', () => { }, ]; - await catalog.transaction(async tx => { + await db.transaction(async tx => { for (const entity of entities) { - await catalog.addEntity(tx, { entity }); + await db.addEntity(tx, { entity }); } }); await expect( - catalog.transaction(async tx => - catalog.entities(tx, [ + db.transaction(async tx => + db.entities(tx, [ { key: 'kind', values: ['k2'] }, { key: 'spec.c', values: ['some'] }, ]), @@ -355,7 +331,6 @@ describe('CommonDatabase', () => { }); it('can get all specific entities for matching filters with nulls (both missing and literal null value)', async () => { - const catalog = new CommonDatabase(knex, getVoidLogger()); const entities: Entity[] = [ { apiVersion: 'a', kind: 'k1', metadata: { name: 'n' } }, { @@ -372,14 +347,14 @@ describe('CommonDatabase', () => { }, ]; - await catalog.transaction(async tx => { + await db.transaction(async tx => { for (const entity of entities) { - await catalog.addEntity(tx, { entity }); + await db.addEntity(tx, { entity }); } }); - const rows = await catalog.transaction(async tx => - catalog.entities(tx, [ + const rows = await db.transaction(async tx => + db.entities(tx, [ { key: 'apiVersion', values: ['a'] }, { key: 'spec.c', values: [null, 'some'] }, ]), diff --git a/plugins/catalog-backend/src/database/CommonDatabase.ts b/plugins/catalog-backend/src/database/CommonDatabase.ts index 2a1b33bbbe..67feb82fac 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.ts @@ -43,7 +43,6 @@ function getStrippedMetadata(metadata: EntityMeta): EntityMeta { delete output.uid; delete output.etag; delete output.generation; - return output; } @@ -62,6 +61,7 @@ function serializeSpec(spec: Entity['spec']): DbEntitiesRow['spec'] { function toEntityRow( locationId: string | undefined, entity: Entity, + normalize: (value: string) => string, ): DbEntitiesRow { return { id: entity.metadata.uid!, @@ -70,8 +70,11 @@ function toEntityRow( generation: entity.metadata.generation!, api_version: entity.apiVersion, kind: entity.kind, - name: entity.metadata.name || null, + name: entity.metadata.name, namespace: entity.metadata.namespace || null, + kind_normalized: normalize(entity.kind), + name_normalized: normalize(entity.metadata.name), + namespace_normalized: normalize(entity.metadata.namespace || ''), metadata: serializeMetadata(entity.metadata), spec: serializeSpec(entity.spec), }; @@ -124,6 +127,7 @@ function generateEtag(): string { export class CommonDatabase implements Database { constructor( private readonly database: Knex, + private readonly normalize: (value: string) => string, private readonly logger: Logger, ) {} @@ -166,7 +170,7 @@ export class CommonDatabase implements Database { generation: 1, }; - const newRow = toEntityRow(request.locationId, newEntity); + const newRow = toEntityRow(request.locationId, newEntity, this.normalize); await tx('entities').insert(newRow); await this.updateEntitiesSearch(tx, newRow.id, newEntity); @@ -257,7 +261,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, newEntity); + const newRow = toEntityRow(request.locationId, newEntity, this.normalize); const updatedRows = await tx('entities') .where({ id: oldRow.id, etag: oldRow.etag }) .update(newRow); diff --git a/plugins/catalog-backend/src/database/DatabaseManager.ts b/plugins/catalog-backend/src/database/DatabaseManager.ts index f0fbe92600..8710aede4c 100644 --- a/plugins/catalog-backend/src/database/DatabaseManager.ts +++ b/plugins/catalog-backend/src/database/DatabaseManager.ts @@ -14,26 +14,39 @@ * limitations under the License. */ +import { getVoidLogger } from '@backstage/backend-common'; +import { makeValidator } from '@backstage/catalog-model'; import Knex from 'knex'; import path from 'path'; import { Logger } from 'winston'; import { CommonDatabase } from './CommonDatabase'; import { Database } from './types'; +export type CreateDatabaseOptions = { + logger: Logger; + fieldNormalizer: (value: string) => string; +}; + +const defaultOptions: CreateDatabaseOptions = { + logger: getVoidLogger(), + fieldNormalizer: makeValidator().normalizeEntityName, +}; + export class DatabaseManager { public static async createDatabase( knex: Knex, - logger: Logger, + options: Partial = {}, ): Promise { await knex.migrate.latest({ directory: path.resolve(__dirname, 'migrations'), loadExtensions: ['.js'], }); - return new CommonDatabase(knex, logger); + const { logger, fieldNormalizer } = { ...defaultOptions, ...options }; + return new CommonDatabase(knex, fieldNormalizer, logger); } public static async createInMemoryDatabase( - logger: Logger, + options: Partial = {}, ): Promise { const knex = Knex({ client: 'sqlite3', @@ -43,6 +56,23 @@ export class DatabaseManager { knex.client.pool.on('createSuccess', (_eventId: any, resource: any) => { resource.run('PRAGMA foreign_keys = ON', () => {}); }); - return DatabaseManager.createDatabase(knex, logger); + return DatabaseManager.createDatabase(knex, options); + } + + public static async createTestDatabase(): Promise { + const knex = Knex({ + client: 'sqlite3', + connection: ':memory:', + useNullAsDefault: true, + }); + knex.client.pool.on('createSuccess', (_eventId: any, resource: any) => { + resource.run('PRAGMA foreign_keys = ON', () => {}); + }); + await knex.migrate.latest({ + directory: path.resolve(__dirname, 'migrations'), + loadExtensions: ['.ts'], + }); + const { logger, fieldNormalizer } = defaultOptions; + return new CommonDatabase(knex, fieldNormalizer, logger); } } diff --git a/plugins/catalog-backend/src/database/migrations/20200612102317_unique_normalized.ts b/plugins/catalog-backend/src/database/migrations/20200612102317_unique_normalized.ts new file mode 100644 index 0000000000..38ed7f6047 --- /dev/null +++ b/plugins/catalog-backend/src/database/migrations/20200612102317_unique_normalized.ts @@ -0,0 +1,51 @@ +/* + * 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 * as Knex from 'knex'; + +export async function up(knex: Knex): Promise { + return knex.schema.alterTable('entities', table => { + // these are added as nullable because sqlite does not support alter column + table + .string('kind_normalized') + .nullable() + .comment('The kind field of the entity, normalized'); + table + .string('name_normalized') + .nullable() + .comment('The metadata.name field of the entity, normalized'); + table + .string('namespace_normalized') + .nullable() + .comment('The metadata.namespace field of the entity, normalized'); + table.dropUnique([], 'entities_unique_name'); + table.unique( + ['kind_normalized', 'name_normalized', 'namespace_normalized'], + 'entities_unique_name_normalized', + ); + }); +} + +export async function down(knex: Knex): Promise { + return knex.schema.alterTable('entities', table => { + table.dropUnique([], 'entities_unique_name_normalized'); + table.unique(['kind', 'name', 'namespace'], 'entities_unique_name'); + table.dropColumns( + 'kind_normalized', + 'name_normalized', + 'namespace_normalized', + ); + }); +} diff --git a/plugins/catalog-backend/src/database/types.ts b/plugins/catalog-backend/src/database/types.ts index fc81e124ba..1cf949c4c2 100644 --- a/plugins/catalog-backend/src/database/types.ts +++ b/plugins/catalog-backend/src/database/types.ts @@ -23,6 +23,9 @@ export type DbEntitiesRow = { kind: string; name: string | null; namespace: string | null; + kind_normalized: string; + name_normalized: string; + namespace_normalized: string; etag: string; generation: number; metadata: string; diff --git a/plugins/catalog-backend/src/service/standaloneServer.ts b/plugins/catalog-backend/src/service/standaloneServer.ts index f19dfaf282..6a9755c0c4 100644 --- a/plugins/catalog-backend/src/service/standaloneServer.ts +++ b/plugins/catalog-backend/src/service/standaloneServer.ts @@ -36,7 +36,7 @@ export async function startStandaloneServer( const logger = options.logger.child({ service: 'catalog-backend' }); logger.debug('Creating application...'); - const db = await DatabaseManager.createInMemoryDatabase(logger); + const db = await DatabaseManager.createInMemoryDatabase({ logger }); const entitiesCatalog = new DatabaseEntitiesCatalog(db); const locationsCatalog = new DatabaseLocationsCatalog(db); const locationReader = new LocationReaders(); From 66fef7f2e42dd61d484d27c87d59cf457176bfe5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Fri, 12 Jun 2020 13:41:05 +0200 Subject: [PATCH 2/2] chore(catalog-backend): use code instead of db to implement similarity rule --- .../src/database/CommonDatabase.ts | 54 ++++++++++++++++--- .../20200612102317_unique_normalized.ts | 51 ------------------ plugins/catalog-backend/src/database/types.ts | 3 -- 3 files changed, 48 insertions(+), 60 deletions(-) delete mode 100644 plugins/catalog-backend/src/database/migrations/20200612102317_unique_normalized.ts diff --git a/plugins/catalog-backend/src/database/CommonDatabase.ts b/plugins/catalog-backend/src/database/CommonDatabase.ts index 67feb82fac..43eb66e9d7 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.ts @@ -61,7 +61,6 @@ function serializeSpec(spec: Entity['spec']): DbEntitiesRow['spec'] { function toEntityRow( locationId: string | undefined, entity: Entity, - normalize: (value: string) => string, ): DbEntitiesRow { return { id: entity.metadata.uid!, @@ -72,9 +71,6 @@ function toEntityRow( kind: entity.kind, name: entity.metadata.name, namespace: entity.metadata.namespace || null, - kind_normalized: normalize(entity.kind), - name_normalized: normalize(entity.metadata.name), - namespace_normalized: normalize(entity.metadata.namespace || ''), metadata: serializeMetadata(entity.metadata), spec: serializeSpec(entity.spec), }; @@ -162,6 +158,8 @@ export class CommonDatabase implements Database { throw new InputError('May not specify generation for new entities'); } + await this.ensureNoSimilarNames(tx, request.entity); + const newEntity = lodash.cloneDeep(request.entity); newEntity.metadata = { ...newEntity.metadata, @@ -170,7 +168,7 @@ export class CommonDatabase implements Database { generation: 1, }; - const newRow = toEntityRow(request.locationId, newEntity, this.normalize); + const newRow = toEntityRow(request.locationId, newEntity); await tx('entities').insert(newRow); await this.updateEntitiesSearch(tx, newRow.id, newEntity); @@ -259,9 +257,11 @@ export class CommonDatabase implements Database { } } + await this.ensureNoSimilarNames(tx, newEntity); + // 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, this.normalize); + const newRow = toEntityRow(request.locationId, newEntity); const updatedRows = await tx('entities') .where({ id: oldRow.id, etag: oldRow.etag }) .update(newRow); @@ -451,4 +451,46 @@ export class CommonDatabase implements Database { // we got around to writing the entries } } + + private async ensureNoSimilarNames( + tx: Knex.Transaction, + data: Entity, + ): Promise { + const newKind = data.kind; + const newName = data.metadata.name; + const newNamespace = data.metadata.namespace; + const newKindNorm = this.normalize(newKind); + const newNameNorm = this.normalize(newName); + const newNamespaceNorm = this.normalize(newNamespace || ''); + + for (const item of await this.entities(tx)) { + if (data.metadata.uid === item.entity.metadata.uid) { + continue; + } + + const oldKind = item.entity.kind; + const oldName = item.entity.metadata.name; + const oldNamespace = item.entity.metadata.namespace; + const oldKindNorm = this.normalize(oldKind); + const oldNameNorm = this.normalize(oldName); + const oldNamespaceNorm = this.normalize(oldNamespace || ''); + + if ( + oldKindNorm === newKindNorm && + oldNameNorm === newNameNorm && + oldNamespaceNorm === newNamespaceNorm + ) { + // Only throw if things were actually different - for completely equal + // things, we let the database handle the conflict + if ( + oldKind !== newKind || + oldName !== newName || + oldNamespace !== newNamespace + ) { + const message = `Kind, namespace, name are too similar to an existing entity`; + throw new ConflictError(message); + } + } + } + } } diff --git a/plugins/catalog-backend/src/database/migrations/20200612102317_unique_normalized.ts b/plugins/catalog-backend/src/database/migrations/20200612102317_unique_normalized.ts deleted file mode 100644 index 38ed7f6047..0000000000 --- a/plugins/catalog-backend/src/database/migrations/20200612102317_unique_normalized.ts +++ /dev/null @@ -1,51 +0,0 @@ -/* - * 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 * as Knex from 'knex'; - -export async function up(knex: Knex): Promise { - return knex.schema.alterTable('entities', table => { - // these are added as nullable because sqlite does not support alter column - table - .string('kind_normalized') - .nullable() - .comment('The kind field of the entity, normalized'); - table - .string('name_normalized') - .nullable() - .comment('The metadata.name field of the entity, normalized'); - table - .string('namespace_normalized') - .nullable() - .comment('The metadata.namespace field of the entity, normalized'); - table.dropUnique([], 'entities_unique_name'); - table.unique( - ['kind_normalized', 'name_normalized', 'namespace_normalized'], - 'entities_unique_name_normalized', - ); - }); -} - -export async function down(knex: Knex): Promise { - return knex.schema.alterTable('entities', table => { - table.dropUnique([], 'entities_unique_name_normalized'); - table.unique(['kind', 'name', 'namespace'], 'entities_unique_name'); - table.dropColumns( - 'kind_normalized', - 'name_normalized', - 'namespace_normalized', - ); - }); -} diff --git a/plugins/catalog-backend/src/database/types.ts b/plugins/catalog-backend/src/database/types.ts index 1cf949c4c2..fc81e124ba 100644 --- a/plugins/catalog-backend/src/database/types.ts +++ b/plugins/catalog-backend/src/database/types.ts @@ -23,9 +23,6 @@ export type DbEntitiesRow = { kind: string; name: string | null; namespace: string | null; - kind_normalized: string; - name_normalized: string; - namespace_normalized: string; etag: string; generation: number; metadata: string;