From 8e4e6d0abaf1ee9a9520790a3ef5ef5c88c2698b Mon Sep 17 00:00:00 2001 From: Nikita Nek Dudnik Date: Mon, 25 May 2020 15:41:52 +0200 Subject: [PATCH 1/7] feature: implement location validation on addLocation --- .../catalog/DatabaseLocationsCatalog.test.ts | 69 +++++++++++++++++++ .../src/catalog/DatabaseLocationsCatalog.ts | 13 ++++ .../ingestion/__mocks__/LocationReaders.ts | 43 ++++++++++++ 3 files changed, 125 insertions(+) create mode 100644 plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts create mode 100644 plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts diff --git a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts new file mode 100644 index 0000000000..9692d55150 --- /dev/null +++ b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts @@ -0,0 +1,69 @@ +/* + * 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 { DatabaseLocationsCatalog } from './DatabaseLocationsCatalog'; +jest.mock('../ingestion/LocationReaders'); + +import knex from 'knex'; +import path from 'path'; + +import { Database } from '../database'; + +describe('DatabaseLocationsCatalog', () => { + const database = knex({ + client: 'sqlite3', + connection: ':memory:', + useNullAsDefault: true, + }); + database.client.pool.on('createSuccess', (_eventId: any, resource: any) => { + resource.run('PRAGMA foreign_keys = ON', () => {}); + }); + let db: Database; + let catalog: DatabaseLocationsCatalog; + + beforeEach(async () => { + await database.migrate.latest({ + directory: path.resolve(__dirname, '../database/migrations'), + loadExtensions: ['.ts'], + }); + db = new Database(database); + catalog = new DatabaseLocationsCatalog(db); + }); + it('resolves to location with id', async () => { + return expect( + catalog.addLocation({ type: 'valid_type', target: 'valid_target' }), + ).resolves.toEqual({ + id: expect.anything(), + type: 'valid_type', + target: 'valid_target', + }); + }); + it('rejects for invalid type', async () => { + const type = 'invalid_type'; + return expect( + catalog.addLocation({ type, target: 'valid_target' }), + ).rejects.toEqual(new Error(`Unknown location type ${type}`)); + }); + it('rejects for unreadable target ', async () => { + const target = 'invalid_target'; + return expect( + catalog.addLocation({ type: 'valid_type', target }), + ).rejects.toEqual( + new Error( + `Can't read location at ${target} with error: Something is broken`, + ), + ); + }); +}); diff --git a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts index e9f1512074..4b2a3a8006 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts @@ -16,11 +16,24 @@ import { Database } from '../database'; import { AddLocation, Location, LocationsCatalog } from './types'; +import { LocationReaders } from '../ingestion'; export class DatabaseLocationsCatalog implements LocationsCatalog { constructor(private readonly database: Database) {} async addLocation(location: AddLocation): Promise { + const outputs = await LocationReaders.create().read( + location.type, + location.target, + ); + outputs.forEach(output => { + if (output.type === 'error') { + throw new Error( + `Can't read location at ${location.target} with error: ${output.error.message}`, + ); + } + }); + const added = await this.database.addLocation(location); return added; } diff --git a/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts b/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts new file mode 100644 index 0000000000..1bfa815280 --- /dev/null +++ b/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts @@ -0,0 +1,43 @@ +/* + * 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 { LocationSource } from '../sources/types'; +import { LocationReader, ReaderOutput } from '../types'; + +export class LocationReaders implements LocationReader { + static create(): LocationReader { + return { + read: (type, target) => { + if (type !== 'valid_type') { + throw new Error(`Unknown location type ${type}`); + } + if (target === 'valid_target') { + return Promise.resolve([{ type: 'data', data: {} }]); + } + throw new Error( + `Can't read location at ${target} with error: Something is broken`, + ); + }, + }; + } + + constructor(private readonly sources: Record) {} + + // eslint-disable-next-line + async read(type: string, target: string): Promise { + return []; + } +} From dfcad875179a18ce3caa6e65dfdc373e06b9937c Mon Sep 17 00:00:00 2001 From: Nikita Nek Dudnik Date: Mon, 25 May 2020 18:37:47 +0200 Subject: [PATCH 2/7] fix: tests after merge --- .../src/catalog/DatabaseLocationsCatalog.test.ts | 3 ++- .../catalog-backend/src/ingestion/__mocks__/LocationReaders.ts | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts index 9692d55150..af1e1fdf2f 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts @@ -20,6 +20,7 @@ import knex from 'knex'; import path from 'path'; import { Database } from '../database'; +import { getVoidLogger } from '../../../../packages/backend-common/src/logging/voidLogger'; describe('DatabaseLocationsCatalog', () => { const database = knex({ @@ -38,7 +39,7 @@ describe('DatabaseLocationsCatalog', () => { directory: path.resolve(__dirname, '../database/migrations'), loadExtensions: ['.ts'], }); - db = new Database(database); + db = new Database(database, getVoidLogger()); catalog = new DatabaseLocationsCatalog(db); }); it('resolves to location with id', async () => { diff --git a/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts b/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts index 1bfa815280..c6ac92ed0f 100644 --- a/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts +++ b/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts @@ -14,8 +14,7 @@ * limitations under the License. */ -import { LocationSource } from '../sources/types'; -import { LocationReader, ReaderOutput } from '../types'; +import { LocationSource, LocationReader, ReaderOutput } from '../types'; export class LocationReaders implements LocationReader { static create(): LocationReader { From fabcec2c0fbe62ea22dfb5caefa1cd95e6f239b5 Mon Sep 17 00:00:00 2001 From: Nikita Nek Dudnik Date: Tue, 26 May 2020 09:13:42 +0200 Subject: [PATCH 3/7] fix: add eslint ignore to module mock file --- .../catalog-backend/src/ingestion/__mocks__/LocationReaders.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts b/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts index c6ac92ed0f..2fa29ccce3 100644 --- a/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts +++ b/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts @@ -32,7 +32,7 @@ export class LocationReaders implements LocationReader { }, }; } - + // eslint-disable-next-line constructor(private readonly sources: Record) {} // eslint-disable-next-line From 885eaac0a98831d5baddcbdb61c924e27253082e Mon Sep 17 00:00:00 2001 From: Nikita Nek Dudnik Date: Tue, 26 May 2020 10:02:02 +0200 Subject: [PATCH 4/7] fix: disable typechecking for mock --- .../catalog-backend/src/ingestion/__mocks__/LocationReaders.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts b/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts index 2fa29ccce3..9459a858f7 100644 --- a/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts +++ b/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +// @ts-nocheck import { LocationSource, LocationReader, ReaderOutput } from '../types'; export class LocationReaders implements LocationReader { From 33b73723637da5b138a2fe4bfd19e574d58cd009 Mon Sep 17 00:00:00 2001 From: Nikita Nek Dudnik Date: Tue, 26 May 2020 10:51:07 +0200 Subject: [PATCH 5/7] fix: parametrize location reader and update test --- .../catalog/DatabaseLocationsCatalog.test.ts | 18 +++++++- .../src/catalog/DatabaseLocationsCatalog.ts | 12 +++--- .../ingestion/__mocks__/LocationReaders.ts | 43 ------------------- 3 files changed, 23 insertions(+), 50 deletions(-) delete mode 100644 plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts diff --git a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts index af1e1fdf2f..e04d0ad5c0 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts @@ -21,6 +21,7 @@ import path from 'path'; import { Database } from '../database'; import { getVoidLogger } from '../../../../packages/backend-common/src/logging/voidLogger'; +import { ReaderOutput } from '../ingestion/types'; describe('DatabaseLocationsCatalog', () => { const database = knex({ @@ -34,14 +35,29 @@ describe('DatabaseLocationsCatalog', () => { let db: Database; let catalog: DatabaseLocationsCatalog; + const mockLocationReader = { + read: async (type: string, target: string): Promise => { + if (type !== 'valid_type') { + throw new Error(`Unknown location type ${type}`); + } + if (target === 'valid_target') { + return Promise.resolve([{ type: 'data', data: {} }]); + } + throw new Error( + `Can't read location at ${target} with error: Something is broken`, + ); + }, + }; + beforeEach(async () => { await database.migrate.latest({ directory: path.resolve(__dirname, '../database/migrations'), loadExtensions: ['.ts'], }); db = new Database(database, getVoidLogger()); - catalog = new DatabaseLocationsCatalog(db); + catalog = new DatabaseLocationsCatalog(db, mockLocationReader); }); + it('resolves to location with id', async () => { return expect( catalog.addLocation({ type: 'valid_type', target: 'valid_target' }), diff --git a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts index 4b2a3a8006..14d8cf7dec 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts @@ -16,16 +16,16 @@ import { Database } from '../database'; import { AddLocation, Location, LocationsCatalog } from './types'; -import { LocationReaders } from '../ingestion'; +import { LocationReader } from '../ingestion'; export class DatabaseLocationsCatalog implements LocationsCatalog { - constructor(private readonly database: Database) {} + constructor( + private readonly database: Database, + private readonly reader: LocationReader, + ) {} async addLocation(location: AddLocation): Promise { - const outputs = await LocationReaders.create().read( - location.type, - location.target, - ); + const outputs = await this.reader.read(location.type, location.target); outputs.forEach(output => { if (output.type === 'error') { throw new Error( diff --git a/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts b/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts deleted file mode 100644 index 9459a858f7..0000000000 --- a/plugins/catalog-backend/src/ingestion/__mocks__/LocationReaders.ts +++ /dev/null @@ -1,43 +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. - */ - -// @ts-nocheck -import { LocationSource, LocationReader, ReaderOutput } from '../types'; - -export class LocationReaders implements LocationReader { - static create(): LocationReader { - return { - read: (type, target) => { - if (type !== 'valid_type') { - throw new Error(`Unknown location type ${type}`); - } - if (target === 'valid_target') { - return Promise.resolve([{ type: 'data', data: {} }]); - } - throw new Error( - `Can't read location at ${target} with error: Something is broken`, - ); - }, - }; - } - // eslint-disable-next-line - constructor(private readonly sources: Record) {} - - // eslint-disable-next-line - async read(type: string, target: string): Promise { - return []; - } -} From 920613533ef8ef746890181a61fdbf71e6818f1c Mon Sep 17 00:00:00 2001 From: Nikita Nek Dudnik Date: Tue, 26 May 2020 10:56:38 +0200 Subject: [PATCH 6/7] fix: add missing reader argument --- packages/backend/src/plugins/catalog.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/plugins/catalog.ts b/packages/backend/src/plugins/catalog.ts index e18d021395..687fd9157a 100644 --- a/packages/backend/src/plugins/catalog.ts +++ b/packages/backend/src/plugins/catalog.ts @@ -36,7 +36,7 @@ export default async function ({ logger, database }: PluginEnvironment) { ); const entitiesCatalog = new DatabaseEntitiesCatalog(db); - const locationsCatalog = new DatabaseLocationsCatalog(db); + const locationsCatalog = new DatabaseLocationsCatalog(db, reader); return await createRouter({ entitiesCatalog, locationsCatalog, logger }); } From bbdd383ebe89d0e30253fc7d629fd16b21782a9d Mon Sep 17 00:00:00 2001 From: Nikita Nek Dudnik Date: Tue, 26 May 2020 11:31:23 +0200 Subject: [PATCH 7/7] fix: PR review clean up --- .../src/catalog/DatabaseLocationsCatalog.test.ts | 12 ++++-------- .../src/catalog/DatabaseLocationsCatalog.ts | 2 +- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts index e04d0ad5c0..0181b7fc28 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.test.ts @@ -14,14 +14,12 @@ * limitations under the License. */ import { DatabaseLocationsCatalog } from './DatabaseLocationsCatalog'; -jest.mock('../ingestion/LocationReaders'); - import knex from 'knex'; import path from 'path'; import { Database } from '../database'; -import { getVoidLogger } from '../../../../packages/backend-common/src/logging/voidLogger'; import { ReaderOutput } from '../ingestion/types'; +import { getVoidLogger } from '@backstage/backend-common'; describe('DatabaseLocationsCatalog', () => { const database = knex({ @@ -71,16 +69,14 @@ describe('DatabaseLocationsCatalog', () => { const type = 'invalid_type'; return expect( catalog.addLocation({ type, target: 'valid_target' }), - ).rejects.toEqual(new Error(`Unknown location type ${type}`)); + ).rejects.toThrow(/Unknown location type/); }); it('rejects for unreadable target ', async () => { const target = 'invalid_target'; return expect( catalog.addLocation({ type: 'valid_type', target }), - ).rejects.toEqual( - new Error( - `Can't read location at ${target} with error: Something is broken`, - ), + ).rejects.toThrow( + `Can't read location at ${target} with error: Something is broken`, ); }); }); diff --git a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts index 14d8cf7dec..3f5739eed0 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts @@ -29,7 +29,7 @@ export class DatabaseLocationsCatalog implements LocationsCatalog { outputs.forEach(output => { if (output.type === 'error') { throw new Error( - `Can't read location at ${location.target} with error: ${output.error.message}`, + `Can't read location at ${location.target}, ${output.error}`, ); } });