Merge pull request #1101 from spotify/freben/location-fetch-store

Move addLocation to new HigherOrderOperations

packages/backend/src/plugins/catalog.ts

@@ -23,6 +23,7 @@ import {
   LocationReaders,
   IngestionModels,
   runPeriodically,
+  HigherOrderOperations,
 } from '@backstage/plugin-catalog-backend';
 import { PluginEnvironment } from '../types';
 import { EntityPolicies } from '@backstage/catalog-model';
@@ -32,7 +33,7 @@ export default async function createPlugin({
   database,
 }: PluginEnvironment) {
   const policy = new EntityPolicies();
-  const ingestion = new IngestionModels(
+  const ingestionModel = new IngestionModels(
     new LocationReaders(),
     new DescriptorParsers(),
     new EntityPolicies(),
@@ -40,12 +41,22 @@ export default async function createPlugin({
 
   const db = await DatabaseManager.createDatabase(database, logger);
   runPeriodically(
-    () => DatabaseManager.refreshLocations(db, ingestion, policy, logger),
+    () => DatabaseManager.refreshLocations(db, ingestionModel, policy, logger),
     10000,
   );
 
-  const entitiesCatalog = new DatabaseEntitiesCatalog(db, policy);
-  const locationsCatalog = new DatabaseLocationsCatalog(db, ingestion);
+  const entitiesCatalog = new DatabaseEntitiesCatalog(db);
+  const locationsCatalog = new DatabaseLocationsCatalog(db);
+  const higherOrderOperation = new HigherOrderOperations(
+    entitiesCatalog,
+    locationsCatalog,
+    ingestionModel,
+  );
 
-  return await createRouter({ entitiesCatalog, locationsCatalog, logger });
+  return await createRouter({
+    entitiesCatalog,
+    locationsCatalog,
+    higherOrderOperation,
+    logger,
+  });
 }

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

@@ -14,15 +14,14 @@
  * limitations under the License.
  */
 
-import type { Entity, EntityPolicy } from '@backstage/catalog-model';
+import type { Entity } from '@backstage/catalog-model';
 import type { Database } from '../database';
 import { DatabaseEntitiesCatalog } from './DatabaseEntitiesCatalog';
 
 describe('DatabaseEntitiesCatalog', () => {
   let db: jest.Mocked<Database>;
-  let policy: EntityPolicy;
 
-  beforeEach(() => {
+  beforeAll(() => {
     db = {
       transaction: jest.fn(),
       addEntity: jest.fn(),
@@ -37,8 +36,11 @@ describe('DatabaseEntitiesCatalog', () => {
       locationHistory: jest.fn(),
       addLocationUpdateLogEvent: jest.fn(),
     };
+  });
+
+  beforeEach(() => {
+    jest.resetAllMocks();
     db.transaction.mockImplementation(async f => f('tx'));
-    policy = { enforce: jest.fn(async x => x) };
   });
 
   describe('addOrUpdateEntity', () => {
@@ -55,10 +57,9 @@ describe('DatabaseEntitiesCatalog', () => {
       db.entities.mockResolvedValue([]);
       db.addEntity.mockResolvedValue({ entity });
 
-      const catalog = new DatabaseEntitiesCatalog(db, policy);
+      const catalog = new DatabaseEntitiesCatalog(db);
       const result = await catalog.addOrUpdateEntity(entity);
 
-      expect(policy.enforce).toBeCalledWith(entity);
       expect(db.entities).toHaveBeenCalledTimes(1);
       expect(db.addEntity).toHaveBeenCalledTimes(1);
       expect(result).toBe(entity);
@@ -78,10 +79,9 @@ describe('DatabaseEntitiesCatalog', () => {
       db.entities.mockResolvedValue([]);
       db.updateEntity.mockResolvedValue({ entity });
 
-      const catalog = new DatabaseEntitiesCatalog(db, policy);
+      const catalog = new DatabaseEntitiesCatalog(db);
       const result = await catalog.addOrUpdateEntity(entity);
 
-      expect(policy.enforce).toBeCalledWith(entity);
       expect(db.entities).toHaveBeenCalledTimes(0);
       expect(db.updateEntity).toHaveBeenCalledTimes(1);
       expect(result).toBe(entity);
@@ -108,10 +108,9 @@ describe('DatabaseEntitiesCatalog', () => {
       db.entities.mockResolvedValue([{ entity: existing }]);
       db.updateEntity.mockResolvedValue({ entity: added });
 
-      const catalog = new DatabaseEntitiesCatalog(db, policy);
+      const catalog = new DatabaseEntitiesCatalog(db);
       const result = await catalog.addOrUpdateEntity(added);
 
-      expect(policy.enforce).toBeCalledWith(added);
       expect(db.entities).toHaveBeenCalledTimes(1);
       expect(db.updateEntity).toHaveBeenCalledTimes(1);
       expect(result).toEqual(existing);

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

@@ -14,15 +14,12 @@
  * limitations under the License.
  */
 
-import type { Entity, EntityPolicy } from '@backstage/catalog-model';
+import type { Entity } from '@backstage/catalog-model';
 import type { Database, DbEntityResponse, EntityFilters } from '../database';
 import type { EntitiesCatalog } from './types';
 
 export class DatabaseEntitiesCatalog implements EntitiesCatalog {
-  constructor(
-    private readonly database: Database,
-    private readonly policy: EntityPolicy,
-  ) {}
+  constructor(private readonly database: Database) {}
 
   async entities(filters?: EntityFilters): Promise<Entity[]> {
     const items = await this.database.transaction(tx =>
@@ -49,13 +46,15 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog {
     );
   }
 
-  async addOrUpdateEntity(entity: Entity): Promise<Entity> {
-    await this.policy.enforce(entity);
+  async addOrUpdateEntity(
+    entity: Entity,
+    locationId?: string,
+  ): Promise<Entity> {
     return await this.database.transaction(async tx => {
       let response: DbEntityResponse;
 
       if (entity.metadata.uid) {
-        response = await this.database.updateEntity(tx, { entity });
+        response = await this.database.updateEntity(tx, { locationId, entity });
       } else {
         const existing = await this.entityByNameInternal(
           tx,
@@ -64,9 +63,12 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog {
           entity.metadata.namespace,
         );
         if (existing) {
-          response = await this.database.updateEntity(tx, { entity });
+          response = await this.database.updateEntity(tx, {
+            locationId,
+            entity,
+          });
         } else {
-          response = await this.database.addEntity(tx, { entity });
+          response = await this.database.addEntity(tx, { locationId, entity });
         }
       }
 

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

@@ -13,73 +13,45 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 import { getVoidLogger } from '@backstage/backend-common';
-import type { Entity } from '@backstage/catalog-model';
 import Knex from 'knex';
 import path from 'path';
 import { CommonDatabase } from '../database';
-import type { Database } from '../database';
-import type { IngestionModel } from '../ingestion/types';
 import { DatabaseLocationsCatalog } from './DatabaseLocationsCatalog';
 
-class MockIngestionModel implements IngestionModel {
-  readLocation = jest.fn(async (type: string, target: string) => {
-    if (type !== 'valid_type') {
-      throw new Error(`Unknown location type ${type}`);
-    }
-    if (target === 'valid_target') {
-      return [{ type: 'data', data: {} as Entity } as const];
-    }
-    throw new Error(
-      `Can't read location at ${target} with error: Something is broken`,
-    );
-  });
-}
-
 describe('DatabaseLocationsCatalog', () => {
-  const knex = Knex({
-    client: 'sqlite3',
-    connection: ':memory:',
-    useNullAsDefault: true,
-  });
-  knex.client.pool.on('createSuccess', (_eventId: any, resource: any) => {
-    resource.run('PRAGMA foreign_keys = ON', () => {});
-  });
-  let db: Database;
   let catalog: DatabaseLocationsCatalog;
-  let ingestionModel: IngestionModel;
 
   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'],
     });
-    db = new CommonDatabase(knex, getVoidLogger());
-    ingestionModel = new MockIngestionModel();
-    catalog = new DatabaseLocationsCatalog(db, ingestionModel);
+    const db = new CommonDatabase(knex, getVoidLogger());
+    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(),
+  it('can add a location', async () => {
+    const location = {
+      id: 'dd12620d-0436-422f-93bd-929aa0788123',
       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.toThrow(/Unknown location type/);
-  });
-  it('rejects for unreadable target ', async () => {
-    const target = 'invalid_target';
-    return expect(
-      catalog.addLocation({ type: 'valid_type', target }),
-    ).rejects.toThrow(
-      `Can't read location at ${target} with error: Something is broken`,
-    );
+    };
+    await expect(catalog.addLocation(location)).resolves.toEqual(location);
+    await expect(
+      catalog.location('dd12620d-0436-422f-93bd-929aa0788123'),
+    ).resolves.toEqual(expect.objectContaining({ data: location }));
+    await expect(catalog.locations()).resolves.toEqual([
+      expect.objectContaining({ data: location }),
+    ]);
   });
 });

plugins/catalog-backend/src/catalog/DatabaseLocationsCatalog.ts

@@ -14,40 +14,15 @@
  * limitations under the License.
  */
 
+import { Location } from '@backstage/catalog-model';
 import type { Database } from '../database';
 import { DatabaseLocationUpdateLogEvent } from '../database/types';
-import { IngestionModel } from '../ingestion/types';
-import {
-  AddLocation,
-  Location,
-  LocationResponse,
-  LocationsCatalog,
-} from './types';
+import { LocationResponse, LocationsCatalog } from './types';
 
 export class DatabaseLocationsCatalog implements LocationsCatalog {
-  constructor(
-    private readonly database: Database,
-    private readonly ingestionModel: IngestionModel,
-  ) {}
-
-  async addLocation(location: AddLocation): Promise<Location> {
-    const outputs = await this.ingestionModel.readLocation(
-      location.type,
-      location.target,
-    );
-    if (!outputs) {
-      throw new Error(
-        `Unknown location type ${location.type} ${location.target}`,
-      );
-    }
-    outputs.forEach(output => {
-      if (output.type === 'error') {
-        throw new Error(
-          `Can't read location at ${location.target}, ${output.error}`,
-        );
-      }
-    });
+  constructor(private readonly database: Database) {}
 
+  async addLocation(location: Location): Promise<Location> {
     const added = await this.database.addLocation(location);
     return added;
   }

plugins/catalog-backend/src/catalog/index.ts

@@ -17,10 +17,4 @@
 export { DatabaseEntitiesCatalog } from './DatabaseEntitiesCatalog';
 export { DatabaseLocationsCatalog } from './DatabaseLocationsCatalog';
 export { StaticEntitiesCatalog } from './StaticEntitiesCatalog';
-export { addLocationSchema } from './types';
-export type {
-  AddLocation,
-  EntitiesCatalog,
-  Location,
-  LocationsCatalog,
-} from './types';
+export type { EntitiesCatalog, LocationsCatalog } from './types';

plugins/catalog-backend/src/catalog/types.ts

@@ -14,8 +14,7 @@
  * limitations under the License.
  */
 
-import { Entity } from '@backstage/catalog-model';
-import * as yup from 'yup';
+import { Entity, Location } from '@backstage/catalog-model';
 import type { EntityFilters } from '../database';
 
 //
@@ -30,7 +29,7 @@ export type EntitiesCatalog = {
     namespace: string | undefined,
     name: string,
   ): Promise<Entity | undefined>;
-  addOrUpdateEntity(entity: Entity): Promise<Entity>;
+  addOrUpdateEntity(entity: Entity, locationId?: string): Promise<Entity>;
   removeEntityByUid(uid: string): Promise<void>;
 };
 
@@ -52,31 +51,13 @@ export type LocationUpdateLogEvent = {
   message?: string;
 };
 
-export type Location = {
-  id: string;
-  type: string;
-  target: string;
-};
-
 export type LocationResponse = {
   data: Location;
   currentStatus: LocationUpdateStatus;
 };
 
-export type AddLocation = {
-  type: string;
-  target: string;
-};
-
-export const addLocationSchema: yup.Schema<AddLocation> = yup
-  .object({
-    type: yup.string().required(),
-    target: yup.string().required(),
-  })
-  .noUnknown();
-
 export type LocationsCatalog = {
-  addLocation(location: AddLocation): Promise<Location>;
+  addLocation(location: Location): Promise<Location>;
   removeLocation(id: string): Promise<void>;
   locations(): Promise<LocationResponse[]>;
   location(id: string): Promise<LocationResponse>;

plugins/catalog-backend/src/database/CommonDatabase.test.ts

@@ -19,16 +19,14 @@ 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 { CommonDatabase } from './CommonDatabase';
 import { DatabaseLocationUpdateLogStatus } from './types';
 import type {
-  AddDatabaseLocation,
   DbEntityRequest,
   DbEntityResponse,
-  DbLocationsRow,
   DbLocationsRowWithStatus,
 } from './types';
 
@@ -87,9 +85,13 @@ describe('CommonDatabase', () => {
 
   it('manages locations', async () => {
     const db = new CommonDatabase(knex, getVoidLogger());
-    const input: AddDatabaseLocation = { type: 'a', target: 'b' };
+    const input: Location = {
+      id: 'dd12620d-0436-422f-93bd-929aa0788123',
+      type: 'a',
+      target: 'b',
+    };
     const output: DbLocationsRowWithStatus = {
-      id: expect.anything(),
+      id: 'dd12620d-0436-422f-93bd-929aa0788123',
       type: 'a',
       target: 'b',
       message: null,
@@ -112,22 +114,6 @@ describe('CommonDatabase', () => {
     );
   });
 
-  it('instead of adding second location with the same target, returns existing one', async () => {
-    // Prepare
-    const catalog = new CommonDatabase(knex, getVoidLogger());
-    const input: AddDatabaseLocation = { type: 'a', target: 'b' };
-    const output1: DbLocationsRow = await catalog.addLocation(input);
-
-    // Try to insert the same location
-    const output2: DbLocationsRow = await catalog.addLocation(input);
-    const locations = await catalog.locations();
-
-    // Output is the same
-    expect(output2).toEqual(output1);
-    // Locations contain only one record
-    expect(locations[0]).toMatchObject(output1);
-  });
-
   describe('addEntity', () => {
     it('happy path: adds entity to empty database', async () => {
       const catalog = new CommonDatabase(knex, getVoidLogger());
@@ -160,27 +146,33 @@ describe('CommonDatabase', () => {
   describe('locationHistory', () => {
     it('outputs the history correctly', async () => {
       const catalog = new CommonDatabase(knex, getVoidLogger());
-      const location: AddDatabaseLocation = { type: 'a', target: 'b' };
-      const { id: locationId } = await catalog.addLocation(location);
+      const location: Location = {
+        id: 'dd12620d-0436-422f-93bd-929aa0788123',
+        type: 'a',
+        target: 'b',
+      };
+      await catalog.addLocation(location);
 
       await catalog.addLocationUpdateLogEvent(
-        locationId,
+        'dd12620d-0436-422f-93bd-929aa0788123',
         DatabaseLocationUpdateLogStatus.SUCCESS,
       );
       await catalog.addLocationUpdateLogEvent(
-        locationId,
+        'dd12620d-0436-422f-93bd-929aa0788123',
         DatabaseLocationUpdateLogStatus.FAIL,
         undefined,
         'Something went wrong',
       );
 
-      const result = await catalog.locationHistory(locationId);
+      const result = await catalog.locationHistory(
+        'dd12620d-0436-422f-93bd-929aa0788123',
+      );
       expect(result).toEqual([
         {
           created_at: expect.anything(),
           entity_name: null,
           id: expect.anything(),
-          location_id: locationId,
+          location_id: 'dd12620d-0436-422f-93bd-929aa0788123',
           message: null,
           status: DatabaseLocationUpdateLogStatus.SUCCESS,
         },
@@ -188,7 +180,7 @@ describe('CommonDatabase', () => {
           created_at: expect.anything(),
           entity_name: null,
           id: expect.anything(),
-          location_id: locationId,
+          location_id: 'dd12620d-0436-422f-93bd-929aa0788123',
           message: 'Something went wrong',
           status: DatabaseLocationUpdateLogStatus.FAIL,
         },

plugins/catalog-backend/src/database/CommonDatabase.ts

@@ -19,14 +19,13 @@ 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';
 import type { Logger } from 'winston';
 import { buildEntitySearch } from './search';
 import type {
-  AddDatabaseLocation,
   Database,
   DatabaseLocationUpdateLogEvent,
   DatabaseLocationUpdateLogStatus,
@@ -336,25 +335,15 @@ export class CommonDatabase implements Database {
     }
   }
 
-  async addLocation(location: AddDatabaseLocation): Promise<DbLocationsRow> {
+  async addLocation(location: Location): Promise<DbLocationsRow> {
     return await this.database.transaction<DbLocationsRow>(async tx => {
-      const existingLocation = await tx<DbLocationsRow>('locations')
-        .where({ target: location.target })
-        .select();
-
-      if (existingLocation?.[0]) {
-        return existingLocation[0];
-      }
-
-      const id = uuidv4();
-      const { type, target } = location;
-      await tx<DbLocationsRow>('locations').insert({
-        id,
-        type,
-        target,
-      });
-
-      return (await tx<DbLocationsRow>('locations').where({ id }).select())![0];
+      const row: DbLocationsRow = {
+        id: location.id,
+        type: location.type,
+        target: location.target,
+      };
+      await tx<DbLocationsRow>('locations').insert(row);
+      return row;
     });
   }
 

plugins/catalog-backend/src/database/migrations/20200511113813_init.ts

@@ -26,7 +26,11 @@ export async function up(knex: Knex): Promise<any> {
         table.comment(
           'Registered locations that shall be contiuously scanned for catalog item updates',
         );
-        table.uuid('id').primary().comment('Auto-generated ID of the location');
+        table
+          .uuid('id')
+          .primary()
+          .notNullable()
+          .comment('Auto-generated ID of the location');
         table.string('type').notNullable().comment('The type of location');
         table
           .string('target')

plugins/catalog-backend/src/database/types.ts

@@ -14,8 +14,7 @@
  * limitations under the License.
  */
 
-import type { Entity } from '@backstage/catalog-model';
-import * as yup from 'yup';
+import type { Entity, Location } from '@backstage/catalog-model';
 
 export type DbEntitiesRow = {
   id: string;
@@ -58,18 +57,6 @@ export type DbLocationsRowWithStatus = DbLocationsRow & {
   message: string | null;
 };
 
-export type AddDatabaseLocation = {
-  type: string;
-  target: string;
-};
-
-export const addDatabaseLocationSchema: yup.Schema<AddDatabaseLocation> = yup
-  .object({
-    type: yup.string().required(),
-    target: yup.string().required(),
-  })
-  .noUnknown();
-
 export enum DatabaseLocationUpdateLogStatus {
   FAIL = 'fail',
   SUCCESS = 'success',
@@ -145,7 +132,7 @@ export type Database = {
 
   removeEntity(tx: unknown, uid: string): Promise<void>;
 
-  addLocation(location: AddDatabaseLocation): Promise<DbLocationsRow>;
+  addLocation(location: Location): Promise<DbLocationsRow>;
 
   removeLocation(id: string): Promise<void>;
 

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

@@ -0,0 +1,114 @@
+/*
+ * 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 { InputError } from '@backstage/backend-common';
+import { Entity, Location, LocationSpec } from '@backstage/catalog-model';
+import { v4 as uuidv4 } from 'uuid';
+import { EntitiesCatalog, LocationsCatalog } from '../catalog';
+import { IngestionModel } from '../ingestion';
+import { AddLocationResult, HigherOrderOperation } from './types';
+
+const LOCATION_ANNOTATION = 'backstage.io/managed-by-location';
+
+/**
+ * Placeholder for operations that span several catalogs and/or stretches out
+ * in time.
+ *
+ * TODO(freben): Find a better home for these, possibly refactoring to use the
+ * database more directly.
+ */
+export class HigherOrderOperations implements HigherOrderOperation {
+  private readonly entitiesCatalog: EntitiesCatalog;
+  private readonly locationsCatalog: LocationsCatalog;
+  private readonly ingestionModel: IngestionModel;
+
+  constructor(
+    entitiesCatalog: EntitiesCatalog,
+    locationsCatalog: LocationsCatalog,
+    ingestionModel: IngestionModel,
+  ) {
+    this.entitiesCatalog = entitiesCatalog;
+    this.locationsCatalog = locationsCatalog;
+    this.ingestionModel = ingestionModel;
+  }
+
+  /**
+   * Adds a single location to the catalog.
+   *
+   * The location is inspected and fetched, and all of the resulting data is
+   * validated. If everything goes well, the location and entities are stored
+   * in the catalog.
+   *
+   * If the location already existed, the old location is returned instead and
+   * the catalog is left unchanged.
+   *
+   * @param spec The location to add
+   */
+  async addLocation(spec: LocationSpec): Promise<AddLocationResult> {
+    // Attempt to find a previous location matching the spec
+    const previousLocations = await this.locationsCatalog.locations();
+    const previousLocation = previousLocations.find(
+      l => spec.type === l.data.type && spec.target === l.data.target,
+    );
+    const location: Location = previousLocation
+      ? previousLocation.data
+      : {
+          id: uuidv4(),
+          type: spec.type,
+          target: spec.target,
+        };
+
+    // Read the location fully, bailing on any errors
+    const readerOutput = await this.ingestionModel.readLocation(
+      location.type,
+      location.target,
+    );
+    const inputEntities: Entity[] = [];
+    for (const entry of readerOutput) {
+      if (entry.type === 'error') {
+        throw new InputError(
+          `Failed to read location ${location.type} ${location.target}, ${entry.error}`,
+        );
+      } else {
+        // Append the location reference annotation
+        entry.data.metadata.annotations = {
+          ...entry.data.metadata.annotations,
+          [LOCATION_ANNOTATION]: location.id,
+        };
+        inputEntities.push(entry.data);
+      }
+    }
+
+    // TODO(freben): At this point, we could detect orphaned entities, by way
+    // of having a LOCATION_ANNOTATION pointing to the location but not being
+    // in the entities list. But we aren't sure what to do about those yet.
+
+    // Write
+    if (!previousLocation) {
+      await this.locationsCatalog.addLocation(location);
+    }
+    const outputEntities: Entity[] = [];
+    for (const entity of inputEntities) {
+      const out = await this.entitiesCatalog.addOrUpdateEntity(
+        entity,
+        location.id,
+      );
+      outputEntities.push(out);
+    }
+
+    return { location, entities: outputEntities };
+  }
+}

plugins/catalog-backend/src/ingestion/index.ts

@@ -15,6 +15,7 @@
  */
 
 export * from './descriptor';
+export { HigherOrderOperations } from './HigherOrderOperations';
 export { IngestionModels } from './IngestionModels';
 export * from './source';
 export type { IngestionModel } from './types';

plugins/catalog-backend/src/ingestion/types.ts

@@ -14,8 +14,18 @@
  * limitations under the License.
  */
 
-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;
+  entities: Entity[];
+};
 
 export type IngestionModel = {
   readLocation(type: string, target: string): Promise<ReaderOutput[]>;
 };
+
+export type HigherOrderOperation = {
+  addLocation(spec: LocationSpec): Promise<AddLocationResult>;
+};

plugins/catalog-backend/src/service/router.test.ts

@@ -15,45 +15,59 @@
  */
 
 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, Location, LocationsCatalog } from '../catalog';
+import { EntitiesCatalog, LocationsCatalog } from '../catalog';
+import { LocationResponse } from '../catalog/types';
+import { HigherOrderOperation } from '../ingestion/types';
 import { createRouter } from './router';
 
-class MockEntitiesCatalog implements EntitiesCatalog {
-  entities = jest.fn();
-  entityByUid = jest.fn();
-  entityByName = jest.fn();
-  addEntity = jest.fn();
-  addOrUpdateEntity = jest.fn();
-  removeEntityByUid = jest.fn();
-}
-
-class MockLocationsCatalog implements LocationsCatalog {
-  addLocation = jest.fn();
-  removeLocation = jest.fn();
-  locations = jest.fn();
-  location = jest.fn();
-  locationHistory = jest.fn();
-}
-
 describe('createRouter', () => {
+  let entitiesCatalog: jest.Mocked<EntitiesCatalog>;
+  let locationsCatalog: jest.Mocked<LocationsCatalog>;
+  let higherOrderOperation: jest.Mocked<HigherOrderOperation>;
+  let app: express.Express;
+
+  beforeAll(async () => {
+    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(),
+    };
+    higherOrderOperation = {
+      addLocation: jest.fn(),
+    };
+    const router = await createRouter({
+      entitiesCatalog,
+      locationsCatalog,
+      higherOrderOperation,
+      logger: getVoidLogger(),
+    });
+    app = express().use(router);
+  });
+
+  beforeEach(() => {
+    jest.resetAllMocks();
+  });
+
   describe('GET /entities', () => {
     it('happy path: lists entities', async () => {
       const entities: Entity[] = [
         { apiVersion: 'a', kind: 'b', metadata: { name: 'n' } },
       ];
 
-      const catalog = new MockEntitiesCatalog();
-      catalog.entities.mockResolvedValueOnce(entities);
+      entitiesCatalog.entities.mockResolvedValueOnce(entities);
 
-      const router = await createRouter({
-        entitiesCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
       const response = await request(app).get('/entities');
 
       expect(response.status).toEqual(200);
@@ -61,18 +75,10 @@ describe('createRouter', () => {
     });
 
     it('parses single and multiple request parameters and passes them down', async () => {
-      const catalog = new MockEntitiesCatalog();
-
-      const router = await createRouter({
-        entitiesCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
       const response = await request(app).get('/entities?a=1&a=&a=3&b=4&c=');
 
       expect(response.status).toEqual(200);
-      expect(catalog.entities).toHaveBeenCalledWith([
+      expect(entitiesCatalog.entities).toHaveBeenCalledWith([
         { key: 'a', values: ['1', null, '3'] },
         { key: 'b', values: ['4'] },
         { key: 'c', values: [null] },
@@ -89,15 +95,8 @@ describe('createRouter', () => {
           name: 'c',
         },
       };
-      const catalog = new MockEntitiesCatalog();
-      catalog.entityByUid.mockResolvedValue(entity);
+      entitiesCatalog.entityByUid.mockResolvedValue(entity);
 
-      const router = await createRouter({
-        entitiesCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
       const response = await request(app).get('/entities/by-uid/zzz');
 
       expect(response.status).toEqual(200);
@@ -105,15 +104,7 @@ describe('createRouter', () => {
     });
 
     it('responds with a 404 for missing entities', async () => {
-      const catalog = new MockEntitiesCatalog();
-      catalog.entityByUid.mockResolvedValue(undefined);
-
-      const router = await createRouter({
-        entitiesCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
+      entitiesCatalog.entityByUid.mockResolvedValue(undefined);
       const response = await request(app).get('/entities/by-uid/zzz');
 
       expect(response.status).toEqual(404);
@@ -131,15 +122,8 @@ describe('createRouter', () => {
           namespace: 'd',
         },
       };
-      const catalog = new MockEntitiesCatalog();
-      catalog.entityByName.mockResolvedValue(entity);
+      entitiesCatalog.entityByName.mockResolvedValue(entity);
 
-      const router = await createRouter({
-        entitiesCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
       const response = await request(app).get('/entities/by-name/b/d/c');
 
       expect(response.status).toEqual(200);
@@ -147,15 +131,8 @@ describe('createRouter', () => {
     });
 
     it('responds with a 404 for missing entities', async () => {
-      const catalog = new MockEntitiesCatalog();
-      catalog.entityByName.mockResolvedValue(undefined);
+      entitiesCatalog.entityByName.mockResolvedValue(undefined);
 
-      const router = await createRouter({
-        entitiesCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
       const response = await request(app).get('/entities/by-name//b/d/c');
 
       expect(response.status).toEqual(404);
@@ -165,13 +142,6 @@ describe('createRouter', () => {
 
   describe('POST /entities', () => {
     it('requires a body', async () => {
-      const catalog = new MockEntitiesCatalog();
-      const router = await createRouter({
-        entitiesCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
       const response = await request(app)
         .post('/entities')
         .set('Content-Type', 'application/json')
@@ -179,7 +149,7 @@ describe('createRouter', () => {
 
       expect(response.status).toEqual(400);
       expect(response.text).toMatch(/body/);
-      expect(catalog.addOrUpdateEntity).not.toHaveBeenCalled();
+      expect(entitiesCatalog.addOrUpdateEntity).not.toHaveBeenCalled();
     });
 
     it('passes the body down', async () => {
@@ -192,15 +162,8 @@ describe('createRouter', () => {
         },
       };
 
-      const catalog = new MockEntitiesCatalog();
-      catalog.addOrUpdateEntity.mockResolvedValue(entity);
+      entitiesCatalog.addOrUpdateEntity.mockResolvedValue(entity);
 
-      const router = await createRouter({
-        entitiesCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
       const response = await request(app)
         .post('/entities')
         .send(entity)
@@ -208,58 +171,46 @@ describe('createRouter', () => {
 
       expect(response.status).toEqual(200);
       expect(response.body).toEqual(entity);
-      expect(catalog.addOrUpdateEntity).toHaveBeenCalledTimes(1);
-      expect(catalog.addOrUpdateEntity).toHaveBeenNthCalledWith(1, entity);
+      expect(entitiesCatalog.addOrUpdateEntity).toHaveBeenCalledTimes(1);
+      expect(entitiesCatalog.addOrUpdateEntity).toHaveBeenNthCalledWith(
+        1,
+        entity,
+      );
     });
   });
 
   describe('DELETE /entities/by-uid/:uid', () => {
     it('can remove', async () => {
-      const catalog = new MockEntitiesCatalog();
-      catalog.removeEntityByUid.mockResolvedValue(undefined);
+      entitiesCatalog.removeEntityByUid.mockResolvedValue(undefined);
 
-      const router = await createRouter({
-        entitiesCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
       const response = await request(app).delete('/entities/by-uid/apa');
 
       expect(response.status).toEqual(204);
-      expect(catalog.removeEntityByUid).toHaveBeenCalledTimes(1);
+      expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledTimes(1);
     });
 
     it('responds with a 404 for missing entities', async () => {
-      const catalog = new MockEntitiesCatalog();
-      catalog.removeEntityByUid.mockRejectedValue(new NotFoundError('nope'));
+      entitiesCatalog.removeEntityByUid.mockRejectedValue(
+        new NotFoundError('nope'),
+      );
 
-      const router = await createRouter({
-        entitiesCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
       const response = await request(app).delete('/entities/by-uid/apa');
 
       expect(response.status).toEqual(404);
-      expect(catalog.removeEntityByUid).toHaveBeenCalledTimes(1);
+      expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledTimes(1);
     });
   });
 
   describe('GET /locations', () => {
     it('happy path: lists locations', async () => {
-      const locations: Location[] = [{ id: 'a', type: 'b', target: 'c' }];
+      const locations: LocationResponse[] = [
+        {
+          currentStatus: { timestamp: '', status: '', message: '' },
+          data: { id: 'a', type: 'b', target: 'c' },
+        },
+      ];
+      locationsCatalog.locations.mockResolvedValueOnce(locations);
 
-      const catalog = new MockLocationsCatalog();
-      catalog.locations.mockResolvedValueOnce(locations);
-
-      const router = await createRouter({
-        locationsCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
       const response = await request(app).get('/locations');
 
       expect(response.status).toEqual(200);
@@ -269,22 +220,33 @@ describe('createRouter', () => {
 
   describe('POST /locations', () => {
     it('rejects malformed locations', async () => {
-      const location = ({
-        id: 'a',
+      const spec = ({
         typez: 'b',
         target: 'c',
-      } as unknown) as Location;
+      } as unknown) as LocationSpec;
 
-      const catalog = new MockLocationsCatalog();
-      const router = await createRouter({
-        locationsCatalog: catalog,
-        logger: getVoidLogger(),
-      });
-
-      const app = express().use(router);
-      const response = await request(app).post('/locations').send(location);
+      const response = await request(app).post('/locations').send(spec);
 
       expect(response.status).toEqual(400);
+      expect(higherOrderOperation.addLocation).not.toHaveBeenCalled();
+    });
+
+    it('passes the body down', async () => {
+      const spec: LocationSpec = {
+        type: 'b',
+        target: 'c',
+      };
+
+      higherOrderOperation.addLocation.mockResolvedValue({
+        location: { id: 'a', ...spec },
+        entities: [],
+      });
+
+      const response = await request(app).post('/locations').send(spec);
+
+      expect(response.status).toEqual(201);
+      expect(higherOrderOperation.addLocation).toHaveBeenCalledTimes(1);
+      expect(higherOrderOperation.addLocation).toHaveBeenCalledWith(spec);
     });
   });
 });

plugins/catalog-backend/src/service/router.ts

@@ -15,28 +15,27 @@
  */
 
 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 {
-  addLocationSchema,
-  EntitiesCatalog,
-  LocationsCatalog,
-} from '../catalog';
+import { EntitiesCatalog, LocationsCatalog } from '../catalog';
 import { EntityFilters } from '../database';
+import { HigherOrderOperation } from '../ingestion/types';
 import { requireRequestBody, validateRequestBody } from './util';
 
 export interface RouterOptions {
   entitiesCatalog?: EntitiesCatalog;
   locationsCatalog?: LocationsCatalog;
+  higherOrderOperation?: HigherOrderOperation;
   logger: Logger;
 }
 
 export async function createRouter(
   options: RouterOptions,
 ): Promise<express.Router> {
-  const { entitiesCatalog, locationsCatalog } = options;
+  const { entitiesCatalog, locationsCatalog, higherOrderOperation } = options;
 
   const router = Router();
   router.use(express.json());
@@ -84,13 +83,16 @@ export async function createRouter(
       });
   }
 
+  if (higherOrderOperation) {
+    router.post('/locations', async (req, res) => {
+      const input = await validateRequestBody(req, locationSpecSchema);
+      const output = await higherOrderOperation.addLocation(input);
+      res.status(201).send(output);
+    });
+  }
+
   if (locationsCatalog) {
     router
-      .post('/locations', async (req, res) => {
-        const input = await validateRequestBody(req, addLocationSchema);
-        const output = await locationsCatalog.addLocation(input);
-        res.status(201).send(output);
-      })
       .get('/locations', async (_req, res) => {
         const output = await locationsCatalog.locations();
         res.status(200).send(output);