From 8d0e750bc2ab32676214645aeb8c5699b7dd66a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Tue, 2 Jun 2020 10:49:23 +0200 Subject: [PATCH] Address comments and add tests --- packages/catalog-model/src/index.ts | 1 + packages/catalog-model/src/location/index.ts | 18 +++ packages/catalog-model/src/location/types.ts | 24 +++ .../catalog-model/src/location/validation.ts | 33 ++++ .../catalog/DatabaseEntitiesCatalog.test.ts | 6 +- .../src/catalog/DatabaseLocationsCatalog.ts | 3 +- plugins/catalog-backend/src/catalog/index.ts | 7 +- plugins/catalog-backend/src/catalog/types.ts | 11 +- .../src/database/CommonDatabase.test.ts | 3 +- .../src/database/CommonDatabase.ts | 3 +- plugins/catalog-backend/src/database/types.ts | 3 +- .../ingestion/HigherOrderOperations.test.ts | 143 ++++++++++++++++++ .../src/ingestion/HigherOrderOperations.ts | 9 +- .../catalog-backend/src/ingestion/types.ts | 5 +- .../src/service/router.test.ts | 10 +- plugins/catalog-backend/src/service/router.ts | 15 +- 16 files changed, 246 insertions(+), 48 deletions(-) create mode 100644 packages/catalog-model/src/location/index.ts create mode 100644 packages/catalog-model/src/location/types.ts create mode 100644 packages/catalog-model/src/location/validation.ts create mode 100644 plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts diff --git a/packages/catalog-model/src/index.ts b/packages/catalog-model/src/index.ts index fb51461053..f149b8c9b4 100644 --- a/packages/catalog-model/src/index.ts +++ b/packages/catalog-model/src/index.ts @@ -17,5 +17,6 @@ export * from './entity'; export { EntityPolicies } from './EntityPolicies'; export * from './kinds'; +export * from './location'; export type { EntityPolicy } from './types'; export * from './validation'; diff --git a/packages/catalog-model/src/location/index.ts b/packages/catalog-model/src/location/index.ts new file mode 100644 index 0000000000..60465bff9b --- /dev/null +++ b/packages/catalog-model/src/location/index.ts @@ -0,0 +1,18 @@ +/* + * 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. + */ + +export type { Location, LocationSpec } from './types'; +export { locationSchema, locationSpecSchema } from './validation'; diff --git a/packages/catalog-model/src/location/types.ts b/packages/catalog-model/src/location/types.ts new file mode 100644 index 0000000000..50e6e82a54 --- /dev/null +++ b/packages/catalog-model/src/location/types.ts @@ -0,0 +1,24 @@ +/* + * 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. + */ + +export type LocationSpec = { + type: string; + target: string; +}; + +export type Location = { + id: string; +} & LocationSpec; diff --git a/packages/catalog-model/src/location/validation.ts b/packages/catalog-model/src/location/validation.ts new file mode 100644 index 0000000000..5fad47bdd0 --- /dev/null +++ b/packages/catalog-model/src/location/validation.ts @@ -0,0 +1,33 @@ +/* + * 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 yup from 'yup'; +import { LocationSpec, Location } from './types'; + +export const locationSpecSchema = yup + .object({ + type: yup.string().required(), + target: yup.string().required(), + }) + .noUnknown(); + +export const locationSchema = yup + .object({ + id: yup.string().required(), + type: yup.string().required(), + target: yup.string().required(), + }) + .noUnknown(); diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts index b185075002..eb9cb2439c 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts @@ -21,7 +21,7 @@ import { DatabaseEntitiesCatalog } from './DatabaseEntitiesCatalog'; describe('DatabaseEntitiesCatalog', () => { let db: jest.Mocked; - beforeEach(() => { + beforeAll(() => { db = { transaction: jest.fn(), addEntity: jest.fn(), @@ -36,6 +36,10 @@ describe('DatabaseEntitiesCatalog', () => { locationHistory: jest.fn(), addLocationUpdateLogEvent: jest.fn(), }; + }); + + beforeEach(() => { + jest.resetAllMocks(); db.transaction.mockImplementation(async f => f('tx')); }); diff --git a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts index 2a5208c95a..066fab8464 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts @@ -14,9 +14,10 @@ * limitations under the License. */ +import { Location } from '@backstage/catalog-model'; import type { Database } from '../database'; import { DatabaseLocationUpdateLogEvent } from '../database/types'; -import { Location, LocationResponse, LocationsCatalog } from './types'; +import { LocationResponse, LocationsCatalog } from './types'; export class DatabaseLocationsCatalog implements LocationsCatalog { constructor(private readonly database: Database) {} diff --git a/plugins/catalog-backend/src/catalog/index.ts b/plugins/catalog-backend/src/catalog/index.ts index dc0bb2e84a..308078b1fc 100644 --- a/plugins/catalog-backend/src/catalog/index.ts +++ b/plugins/catalog-backend/src/catalog/index.ts @@ -17,9 +17,4 @@ export { DatabaseEntitiesCatalog } from './DatabaseEntitiesCatalog'; export { DatabaseLocationsCatalog } from './DatabaseLocationsCatalog'; export { StaticEntitiesCatalog } from './StaticEntitiesCatalog'; -export type { - EntitiesCatalog, - Location, - LocationsCatalog, - LocationSpec, -} from './types'; +export type { EntitiesCatalog, LocationsCatalog } from './types'; diff --git a/plugins/catalog-backend/src/catalog/types.ts b/plugins/catalog-backend/src/catalog/types.ts index a8c3c7a15e..0499b8a409 100644 --- a/plugins/catalog-backend/src/catalog/types.ts +++ b/plugins/catalog-backend/src/catalog/types.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Entity } from '@backstage/catalog-model'; +import { Entity, Location } from '@backstage/catalog-model'; import type { EntityFilters } from '../database'; // @@ -51,15 +51,6 @@ export type LocationUpdateLogEvent = { message?: string; }; -export type LocationSpec = { - type: string; - target: string; -}; - -export type Location = { - id: string; -} & LocationSpec; - export type LocationResponse = { data: Location; currentStatus: LocationUpdateStatus; diff --git a/plugins/catalog-backend/src/database/CommonDatabase.test.ts b/plugins/catalog-backend/src/database/CommonDatabase.test.ts index 8f62969bd1..f1723e3733 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.test.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.test.ts @@ -19,10 +19,9 @@ import { getVoidLogger, NotFoundError, } from '@backstage/backend-common'; -import type { Entity } from '@backstage/catalog-model'; +import type { Entity, Location } from '@backstage/catalog-model'; import Knex from 'knex'; import path from 'path'; -import { Location } from '../catalog'; import { CommonDatabase } from './CommonDatabase'; import { DatabaseLocationUpdateLogStatus } from './types'; import type { diff --git a/plugins/catalog-backend/src/database/CommonDatabase.ts b/plugins/catalog-backend/src/database/CommonDatabase.ts index 084f6ea662..dd965c89ef 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.ts @@ -19,7 +19,7 @@ import { InputError, NotFoundError, } from '@backstage/backend-common'; -import type { Entity, EntityMeta } from '@backstage/catalog-model'; +import type { Entity, EntityMeta, Location } from '@backstage/catalog-model'; import Knex from 'knex'; import lodash from 'lodash'; import { v4 as uuidv4 } from 'uuid'; @@ -37,7 +37,6 @@ import type { DbLocationsRowWithStatus, EntityFilters, } from './types'; -import { Location } from '../catalog'; function getStrippedMetadata(metadata: EntityMeta): EntityMeta { const output = lodash.cloneDeep(metadata); diff --git a/plugins/catalog-backend/src/database/types.ts b/plugins/catalog-backend/src/database/types.ts index 4bcc66af1c..17da373ae9 100644 --- a/plugins/catalog-backend/src/database/types.ts +++ b/plugins/catalog-backend/src/database/types.ts @@ -14,8 +14,7 @@ * limitations under the License. */ -import type { Entity } from '@backstage/catalog-model'; -import { Location } from '../catalog'; +import type { Entity, Location } from '@backstage/catalog-model'; export type DbEntitiesRow = { id: string; diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts new file mode 100644 index 0000000000..7f1e890fbf --- /dev/null +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts @@ -0,0 +1,143 @@ +/* + * 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 { EntitiesCatalog, LocationsCatalog } from '../catalog'; +import { IngestionModel } from './types'; +import { HigherOrderOperations } from './HigherOrderOperations'; +import { Entity } from '@backstage/catalog-model'; + +describe('HigherOrderOperations', () => { + let entitiesCatalog: jest.Mocked; + let locationsCatalog: jest.Mocked; + let ingestionModel: jest.Mocked; + let higherOrderOperation: HigherOrderOperations; + + beforeAll(() => { + entitiesCatalog = { + entities: jest.fn(), + entityByUid: jest.fn(), + entityByName: jest.fn(), + addOrUpdateEntity: jest.fn(), + removeEntityByUid: jest.fn(), + }; + locationsCatalog = { + addLocation: jest.fn(), + removeLocation: jest.fn(), + locations: jest.fn(), + location: jest.fn(), + locationHistory: jest.fn(), + }; + ingestionModel = { + readLocation: jest.fn(), + }; + higherOrderOperation = new HigherOrderOperations( + entitiesCatalog, + locationsCatalog, + ingestionModel, + ); + }); + + beforeEach(() => { + jest.resetAllMocks(); + }); + + describe('addLocation', () => { + it('just inserts the location when there are no entities to read', async () => { + const spec = { + type: 'a', + target: 'b', + }; + locationsCatalog.addLocation.mockImplementation(x => Promise.resolve(x)); + locationsCatalog.locations.mockResolvedValue([]); + ingestionModel.readLocation.mockResolvedValue([]); + + const result = await higherOrderOperation.addLocation(spec); + + expect(result.location).toEqual( + expect.objectContaining({ + id: expect.anything(), + ...spec, + }), + ); + expect(result.entities).toEqual([]); + expect(locationsCatalog.locations).toBeCalledTimes(1); + expect(ingestionModel.readLocation).toBeCalledTimes(1); + expect(ingestionModel.readLocation).toBeCalledWith('a', 'b'); + expect(entitiesCatalog.addOrUpdateEntity).not.toBeCalled(); + expect(locationsCatalog.addLocation).toBeCalledTimes(1); + expect(locationsCatalog.addLocation).toBeCalledWith( + expect.objectContaining({ + id: expect.anything(), + ...spec, + }), + ); + }); + + it('reuses the location if a match already existed', async () => { + const spec = { + type: 'a', + target: 'b', + }; + const location = { + id: 'dd12620d-0436-422f-93bd-929aa0788123', + ...spec, + }; + + locationsCatalog.locations.mockResolvedValue([ + { + currentStatus: { timestamp: '', status: '', message: '' }, + data: location, + }, + ]); + ingestionModel.readLocation.mockResolvedValue([]); + + const result = await higherOrderOperation.addLocation(spec); + + expect(result.location).toEqual(location); + expect(result.entities).toEqual([]); + expect(locationsCatalog.locations).toBeCalledTimes(1); + expect(ingestionModel.readLocation).toBeCalledTimes(1); + expect(ingestionModel.readLocation).toBeCalledWith('a', 'b'); + expect(entitiesCatalog.addOrUpdateEntity).not.toBeCalled(); + expect(locationsCatalog.addLocation).not.toBeCalled(); + }); + + it('rejects the whole operation if any entity could not be read', async () => { + const spec = { + type: 'a', + target: 'b', + }; + const entity: Entity = { + apiVersion: 'a', + kind: 'b', + metadata: { name: 'n' }, + }; + + locationsCatalog.locations.mockResolvedValue([]); + ingestionModel.readLocation.mockResolvedValue([ + { type: 'data', data: entity }, + { type: 'error', error: new Error('abcd') }, + ]); + + await expect(higherOrderOperation.addLocation(spec)).rejects.toThrow( + /abcd/, + ); + expect(locationsCatalog.locations).toBeCalledTimes(1); + expect(entitiesCatalog.addOrUpdateEntity).not.toBeCalled(); + expect(locationsCatalog.addLocation).not.toBeCalled(); + }); + }); +}); diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts index 0314556e27..e28d1e722a 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts @@ -15,14 +15,9 @@ */ import { InputError } from '@backstage/backend-common'; -import { Entity } from '@backstage/catalog-model'; +import { Entity, Location, LocationSpec } from '@backstage/catalog-model'; import { v4 as uuidv4 } from 'uuid'; -import { - EntitiesCatalog, - Location, - LocationsCatalog, - LocationSpec, -} from '../catalog'; +import { EntitiesCatalog, LocationsCatalog } from '../catalog'; import { IngestionModel } from '../ingestion'; import { AddLocationResult, HigherOrderOperation } from './types'; diff --git a/plugins/catalog-backend/src/ingestion/types.ts b/plugins/catalog-backend/src/ingestion/types.ts index 45dd0d9f74..018784bd0f 100644 --- a/plugins/catalog-backend/src/ingestion/types.ts +++ b/plugins/catalog-backend/src/ingestion/types.ts @@ -14,9 +14,8 @@ * limitations under the License. */ -import { Entity } from '@backstage/catalog-model'; -import { Location, LocationSpec } from '../catalog'; -import { ReaderOutput } from './descriptor/parsers/types'; +import type { Entity, Location, LocationSpec } from '@backstage/catalog-model'; +import type { ReaderOutput } from './descriptor/parsers/types'; export type AddLocationResult = { location: Location; diff --git a/plugins/catalog-backend/src/service/router.test.ts b/plugins/catalog-backend/src/service/router.test.ts index e03cb3dc98..d26476bca0 100644 --- a/plugins/catalog-backend/src/service/router.test.ts +++ b/plugins/catalog-backend/src/service/router.test.ts @@ -15,10 +15,10 @@ */ import { getVoidLogger, NotFoundError } from '@backstage/backend-common'; -import type { Entity } from '@backstage/catalog-model'; +import type { Entity, LocationSpec } from '@backstage/catalog-model'; import express from 'express'; import request from 'supertest'; -import { EntitiesCatalog, LocationsCatalog, LocationSpec } from '../catalog'; +import { EntitiesCatalog, LocationsCatalog } from '../catalog'; import { LocationResponse } from '../catalog/types'; import { HigherOrderOperation } from '../ingestion/types'; import { createRouter } from './router'; @@ -29,7 +29,7 @@ describe('createRouter', () => { let higherOrderOperation: jest.Mocked; let app: express.Express; - beforeEach(async () => { + beforeAll(async () => { entitiesCatalog = { entities: jest.fn(), entityByUid: jest.fn(), @@ -56,6 +56,10 @@ describe('createRouter', () => { app = express().use(router); }); + beforeEach(() => { + jest.resetAllMocks(); + }); + describe('GET /entities', () => { it('happy path: lists entities', async () => { const entities: Entity[] = [ diff --git a/plugins/catalog-backend/src/service/router.ts b/plugins/catalog-backend/src/service/router.ts index 95ea068f58..22d1f73730 100644 --- a/plugins/catalog-backend/src/service/router.ts +++ b/plugins/catalog-backend/src/service/router.ts @@ -15,12 +15,12 @@ */ import { errorHandler, InputError } from '@backstage/backend-common'; -import { Entity } from '@backstage/catalog-model'; +import { locationSpecSchema } from '@backstage/catalog-model'; +import type { Entity } from '@backstage/catalog-model'; import express from 'express'; import Router from 'express-promise-router'; import { Logger } from 'winston'; -import * as yup from 'yup'; -import { EntitiesCatalog, LocationsCatalog, LocationSpec } from '../catalog'; +import { EntitiesCatalog, LocationsCatalog } from '../catalog'; import { EntityFilters } from '../database'; import { HigherOrderOperation } from '../ingestion/types'; import { requireRequestBody, validateRequestBody } from './util'; @@ -32,13 +32,6 @@ export interface RouterOptions { logger: Logger; } -const addLocationSchema = yup - .object({ - type: yup.string().required(), - target: yup.string().required(), - }) - .noUnknown(); - export async function createRouter( options: RouterOptions, ): Promise { @@ -92,7 +85,7 @@ export async function createRouter( if (higherOrderOperation) { router.post('/locations', async (req, res) => { - const input = await validateRequestBody(req, addLocationSchema); + const input = await validateRequestBody(req, locationSpecSchema); const output = await higherOrderOperation.addLocation(input); res.status(201).send(output); });