Address comments and add tests

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';

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';

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;

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<LocationSpec>({
+    type: yup.string().required(),
+    target: yup.string().required(),
+  })
+  .noUnknown();
+
+export const locationSchema = yup
+  .object<Location>({
+    id: yup.string().required(),
+    type: yup.string().required(),
+    target: yup.string().required(),
+  })
+  .noUnknown();

plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.test.ts

@@ -21,7 +21,7 @@ import { DatabaseEntitiesCatalog } from './DatabaseEntitiesCatalog';
 describe('DatabaseEntitiesCatalog', () => {
   let db: jest.Mocked<Database>;
 
-  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'));
   });
 

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

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';

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;

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 {

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

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;

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<EntitiesCatalog>;
+  let locationsCatalog: jest.Mocked<LocationsCatalog>;
+  let ingestionModel: jest.Mocked<IngestionModel>;
+  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();
+    });
+  });
+});

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';
 

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;

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<HigherOrderOperation>;
   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[] = [

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<LocationSpec>({
-    type: yup.string().required(),
-    target: yup.string().required(),
-  })
-  .noUnknown();
-
 export async function createRouter(
   options: RouterOptions,
 ): Promise<express.Router> {
@@ -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);
     });