paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 49 Packages Projects Releases Wiki Activity

Merge pull request #1265 from spotify/freben/i-am-almost-unique

Browse Source 
feat(catalog-backend): reject almost-same-name entities
This commit is contained in:
Fredrik Adelöw
2020-06-12 13:54:04 +02:00
committed by GitHub
parent 5e034d687b 66fef7f2e4
commit 6c642a60c0
6 changed files with 154 additions and 118 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/backend/src/plugins/catalog.ts
+1 -1
View File
@@ -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(
plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts
+2 -17
View File
@@ -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);
});
plugins/catalog-backend/src/database/CommonDatabase.test.ts
+68 -93
View File
@@ -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'] },
]),
plugins/catalog-backend/src/database/CommonDatabase.ts
+48 -2
View File
@@ -43,7 +43,6 @@ function getStrippedMetadata(metadata: EntityMeta): EntityMeta {
delete output.uid;
delete output.etag;
delete output.generation;
return output;
}
@@ -70,7 +69,7 @@ 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,
metadata: serializeMetadata(entity.metadata),
spec: serializeSpec(entity.spec),
@@ -124,6 +123,7 @@ function generateEtag(): string {
export class CommonDatabase implements Database {
constructor(
private readonly database: Knex,
private readonly normalize: (value: string) => string,
private readonly logger: Logger,
) {}
@@ -158,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,
@@ -255,6 +257,8 @@ 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);
@@ -447,4 +451,46 @@ export class CommonDatabase implements Database {
// we got around to writing the entries
}
}
private async ensureNoSimilarNames(
tx: Knex.Transaction<any, any>,
data: Entity,
): Promise<void> {
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);
}
}
}
}
}
plugins/catalog-backend/src/database/DatabaseManager.ts
+34 -4
View File
@@ -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<CreateDatabaseOptions> = {},
): Promise<Database> {
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<CreateDatabaseOptions> = {},
): Promise<Database> {
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<Database> {
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);
}
}
plugins/catalog-backend/src/service/standaloneServer.ts
+1 -1
View File
@@ -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();