Merge pull request #2641 from spotify/freben/easier-read

feat(catalog-backend): simplify the read function in processors

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

@@ -258,10 +258,9 @@ export class LocationReaders implements LocationReader {
     }
   }
 
-  private async readLocation(
-    location: LocationSpec,
-  ): Promise<LocationProcessorResult> {
-    let locationResult: LocationProcessorResult | undefined;
+  private async readLocation(location: LocationSpec): Promise<Buffer> {
+    let data: Buffer | undefined = undefined;
+    let error: Error | undefined = undefined;
 
     await this.handleLocation(
       {
@@ -269,13 +268,29 @@ export class LocationReaders implements LocationReader {
         location,
         optional: false,
       },
-      r => (locationResult = r),
+      output => {
+        if (output.type === 'error' && !error) {
+          error = output.error;
+        } else if (output.type === 'data') {
+          if (data) {
+            if (!error) {
+              error = new Error(
+                'More than one piece of data loaded unexpectedly',
+              );
+            }
+          } else {
+            data = output.data;
+          }
+        }
+      },
     );
 
-    if (!locationResult) {
-      throw new Error('No location loaded');
+    if (error) {
+      throw error;
+    } else if (!data) {
+      throw new Error('No data loaded');
     }
 
-    return locationResult;
+    return data;
   }
 }

plugins/catalog-backend/src/ingestion/processors/ApiDefinitionAtLocationProcessor.test.ts

@@ -14,9 +14,7 @@ import { ApiEntity, Entity, LocationSpec } from '@backstage/catalog-model';
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-
 import { ApiDefinitionAtLocationProcessor } from './ApiDefinitionAtLocationProcessor';
-import { LocationProcessorResult } from './types';
 
 describe('ApiDefinitionAtLocationProcessor', () => {
   let processor: ApiDefinitionAtLocationProcessor;
@@ -45,11 +43,7 @@ describe('ApiDefinitionAtLocationProcessor', () => {
   });
 
   it('should skip entities without annotation', async () => {
-    const read = jest.fn(
-      (): Promise<LocationProcessorResult> => {
-        throw new Error();
-      },
-    );
+    const read = jest.fn().mockRejectedValue(new Error('boo'));
 
     const generated = (await processor.processEntity(
       entity,
@@ -67,14 +61,7 @@ describe('ApiDefinitionAtLocationProcessor', () => {
         'url:http://example.com/openapi.yaml',
     };
 
-    const read = jest.fn(
-      (l: LocationSpec): Promise<LocationProcessorResult> =>
-        Promise.resolve({
-          type: 'data',
-          data: Buffer.from('Hello'),
-          location: l,
-        }),
-    );
+    const read = jest.fn().mockResolvedValue(Buffer.from('Hello'));
 
     const generated = (await processor.processEntity(
       entity,
@@ -95,38 +82,12 @@ describe('ApiDefinitionAtLocationProcessor', () => {
       'backstage.io/definition-at-location': 'missing',
     };
 
-    const read = jest.fn(
-      (l: LocationSpec): Promise<LocationProcessorResult> =>
-        Promise.resolve({
-          type: 'error',
-          error: new Error('Failed to load location'),
-          location: l,
-        }),
-    );
+    const read = jest
+      .fn()
+      .mockRejectedValue(new Error('Failed to load location'));
 
     await expect(
       processor.processEntity(entity, location, () => {}, read),
-    ).rejects.toThrow('Failed to read location: Failed to load location');
-  });
-
-  it('should throw errors if location read has wrong type', async () => {
-    entity.metadata.annotations = {
-      'backstage.io/definition-at-location': 'wrong',
-    };
-
-    const read = jest.fn(
-      (l: LocationSpec): Promise<LocationProcessorResult> =>
-        Promise.resolve({
-          type: 'location',
-          optional: false,
-          location: l,
-        }),
-    );
-
-    await expect(
-      processor.processEntity(entity, location, () => {}, read),
-    ).rejects.toThrow(
-      `Only supports location processor results of type 'data', but got 'location'`,
-    );
+    ).rejects.toThrow('Failed to load location');
   });
 });

plugins/catalog-backend/src/ingestion/processors/ApiDefinitionAtLocationProcessor.ts

@@ -41,19 +41,8 @@ export class ApiDefinitionAtLocationProcessor implements LocationProcessor {
     const reference =
       entity.metadata.annotations[DEFINITION_AT_LOCATION_ANNOTATION];
     const { type, target } = extractReference(reference);
-    const result = await read({ type, target });
-
-    if (result.type === 'error') {
-      throw new Error(`Failed to read location: ${result.error.message}`);
-    }
-
-    if (result.type !== 'data') {
-      throw new Error(
-        `Only supports location processor results of type 'data', but got '${result.type}'`,
-      );
-    }
-
-    const definition = result.data.toString();
+    const data = await read({ type, target });
+    const definition = data.toString();
     const apiEntity = entity as ApiEntity;
     apiEntity.spec.definition = definition;
 

plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.test.ts

@@ -22,9 +22,16 @@ import {
   ResolverParams,
   yamlPlaceholderResolver,
 } from './PlaceholderProcessor';
-import { LocationProcessorRead } from './types';
+import { LocationProcessorEmit, LocationProcessorRead } from './types';
 
 describe('PlaceholderProcessor', () => {
+  const emit: LocationProcessorEmit = jest.fn();
+  const read: jest.MockedFunction<LocationProcessorRead> = jest.fn();
+
+  beforeEach(() => {
+    jest.resetAllMocks();
+  });
+
   it('returns placeholder-free data unchanged', async () => {
     const input: Entity = {
       apiVersion: 'a',
@@ -35,18 +42,11 @@ describe('PlaceholderProcessor', () => {
       foo: async () => 'replaced',
     });
     await expect(
-      processor.processEntity(
-        input,
-        { type: 't', target: 'l' },
-        jest.fn(),
-        jest.fn(),
-      ),
+      processor.processEntity(input, { type: 't', target: 'l' }, emit, read),
     ).resolves.toBe(input);
   });
 
   it('replaces placeholders deep in the data', async () => {
-    const emit = jest.fn();
-    const read = jest.fn();
     const upperResolver: PlaceholderResolver = jest.fn(async ({ value }) =>
       value!.toString().toUpperCase(),
     );
@@ -84,8 +84,6 @@ describe('PlaceholderProcessor', () => {
   });
 
   it('rejects multiple placeholders', async () => {
-    const emit = jest.fn();
-    const read: LocationProcessorRead = jest.fn();
     const processor = new PlaceholderProcessor({
       foo: jest.fn(),
       bar: jest.fn(),
@@ -111,8 +109,6 @@ describe('PlaceholderProcessor', () => {
   });
 
   it('rejects unknown placeholders', async () => {
-    const emit = jest.fn();
-    const read: LocationProcessorRead = jest.fn();
     const processor = new PlaceholderProcessor({
       bar: jest.fn(),
     });
@@ -135,14 +131,7 @@ describe('PlaceholderProcessor', () => {
   });
 
   it('has builtin text support', async () => {
-    const emit = jest.fn();
-    const read: LocationProcessorRead = jest
-      .fn()
-      .mockImplementation(async location => ({
-        type: 'data',
-        location,
-        data: Buffer.from('TEXT', 'utf-8'),
-      }));
+    read.mockResolvedValue(Buffer.from('TEXT', 'utf-8'));
     const processor = PlaceholderProcessor.default();
 
     await expect(
@@ -175,14 +164,9 @@ describe('PlaceholderProcessor', () => {
   });
 
   it('has builtin json support', async () => {
-    const emit = jest.fn();
-    const read: LocationProcessorRead = jest
-      .fn()
-      .mockImplementation(async location => ({
-        type: 'data',
-        location,
-        data: Buffer.from(JSON.stringify({ a: ['b', 7] }), 'utf-8'),
-      }));
+    read.mockResolvedValue(
+      Buffer.from(JSON.stringify({ a: ['b', 7] }), 'utf-8'),
+    );
     const processor = PlaceholderProcessor.default();
 
     await expect(
@@ -215,14 +199,7 @@ describe('PlaceholderProcessor', () => {
   });
 
   it('has builtin yaml support', async () => {
-    const emit = jest.fn();
-    const read: LocationProcessorRead = jest
-      .fn()
-      .mockImplementation(async location => ({
-        type: 'data',
-        location,
-        data: Buffer.from('foo:\n  - bar: 7', 'utf-8'),
-      }));
+    read.mockResolvedValue(Buffer.from('foo:\n  - bar: 7', 'utf-8'));
     const processor = PlaceholderProcessor.default();
 
     await expect(
@@ -256,65 +233,46 @@ describe('PlaceholderProcessor', () => {
 });
 
 describe('yamlPlaceholderResolver', () => {
-  let read: jest.MockedFunction<LocationProcessorRead>;
-  let params: ResolverParams;
+  const read: jest.MockedFunction<LocationProcessorRead> = jest.fn();
+  const params: ResolverParams = {
+    key: 'a',
+    value: './file.yaml',
+    location: {
+      type: 'github',
+      target: 'https://github.com/spotify/backstage/a/b/catalog-info.yaml',
+    },
+    read,
+  };
 
   beforeEach(() => {
-    read = jest.fn();
-    params = {
-      key: 'a',
-      value: './file.yaml',
-      location: {
-        type: 'github',
-        target: 'https://github.com/spotify/backstage/a/b/catalog-info.yaml',
-      },
-      read,
-    };
+    jest.resetAllMocks();
   });
 
   it('parses valid yaml', async () => {
-    read.mockImplementation(async location => ({
-      type: 'data',
-      location,
-      data: Buffer.from('foo:\n  - bar: 7', 'utf-8'),
-    }));
-
+    read.mockResolvedValue(Buffer.from('foo:\n  - bar: 7', 'utf-8'));
     await expect(yamlPlaceholderResolver(params)).resolves.toEqual({
       foo: [{ bar: 7 }],
     });
   });
 
   it('rejects invalid yaml', async () => {
-    read.mockImplementation(async location => ({
-      type: 'data',
-      location,
-      data: Buffer.from('a: 1\n----\n', 'utf-8'),
-    }));
-
+    read.mockResolvedValue(Buffer.from('a: 1\n----\n', 'utf-8'));
     await expect(yamlPlaceholderResolver(params)).rejects.toThrow(
       'Placeholder $a found an error in the data at ./file.yaml, YAMLSemanticError: Implicit map keys need to be followed by map values',
     );
   });
 
   it('rejects multi-document yaml', async () => {
-    read.mockImplementation(async location => ({
-      type: 'data',
-      location,
-      data: Buffer.from('foo: 1\n---\nbar: 2\n', 'utf-8'),
-    }));
-
+    read.mockResolvedValue(Buffer.from('foo: 1\n---\nbar: 2\n', 'utf-8'));
     await expect(yamlPlaceholderResolver(params)).rejects.toThrow(
       'Placeholder $a expected to find exactly one document of data at ./file.yaml, found 2',
     );
   });
 
   it('parses valid json', async () => {
-    read.mockImplementation(async location => ({
-      type: 'data',
-      location,
-      data: Buffer.from(JSON.stringify({ a: ['b', 7] }), 'utf-8'),
-    }));
-
+    read.mockResolvedValue(
+      Buffer.from(JSON.stringify({ a: ['b', 7] }), 'utf-8'),
+    );
     await expect(yamlPlaceholderResolver(params)).resolves.toEqual({
       a: ['b', 7],
     });
@@ -322,41 +280,32 @@ describe('yamlPlaceholderResolver', () => {
 });
 
 describe('jsonPlaceholderResolver', () => {
-  let read: jest.MockedFunction<LocationProcessorRead>;
-  let params: ResolverParams;
+  const read: jest.MockedFunction<LocationProcessorRead> = jest.fn();
+  const params: ResolverParams = {
+    key: 'a',
+    value: './file.json',
+    location: {
+      type: 'github',
+      target: 'https://github.com/spotify/backstage/a/b/catalog-info.yaml',
+    },
+    read,
+  };
 
   beforeEach(() => {
-    read = jest.fn();
-    params = {
-      key: 'a',
-      value: './file.json',
-      location: {
-        type: 'github',
-        target: 'https://github.com/spotify/backstage/a/b/catalog-info.yaml',
-      },
-      read,
-    };
+    jest.resetAllMocks();
   });
 
   it('parses valid json', async () => {
-    read.mockImplementation(async location => ({
-      type: 'data',
-      location,
-      data: Buffer.from(JSON.stringify({ a: ['b', 7] }), 'utf-8'),
-    }));
-
+    read.mockResolvedValue(
+      Buffer.from(JSON.stringify({ a: ['b', 7] }), 'utf-8'),
+    );
     await expect(jsonPlaceholderResolver(params)).resolves.toEqual({
       a: ['b', 7],
     });
   });
 
   it('rejects invalid json', async () => {
-    read.mockImplementation(async location => ({
-      type: 'data',
-      location,
-      data: Buffer.from('}', 'utf-8'),
-    }));
-
+    read.mockResolvedValue(Buffer.from('}', 'utf-8'));
     await expect(jsonPlaceholderResolver(params)).rejects.toThrow(
       'Placeholder $a failed to parse JSON data at ./file.json, SyntaxError: Unexpected token } in JSON at position 0',
     );

plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.ts

@@ -174,11 +174,8 @@ async function readTextLocation(params: ResolverParams): Promise<string> {
   const newLocation = relativeLocation(params);
 
   try {
-    const response = await params.read(newLocation);
-    if (response.type !== 'data') {
-      throw new Error(`Expected data, got ${response.type}`);
-    }
-    return response.data.toString('utf-8');
+    const data = await params.read(newLocation);
+    return data.toString('utf-8');
   } catch (e) {
     throw new Error(
       `Placeholder \$${params.key} could not read location ${params.value}, ${e}`,

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

@@ -66,7 +66,7 @@ export type LocationProcessor = {
    *
    * @param error The error
    * @param location The location where the error occurred
-   * @param emit A sink for items resulting from this handilng
+   * @param emit A sink for items resulting from this handling
    * @returns Nothing
    */
   handleError?(
@@ -110,6 +110,4 @@ export type LocationProcessorResult =
   | LocationProcessorEntityResult
   | LocationProcessorErrorResult;
 
-export type LocationProcessorRead = (
-  location: LocationSpec,
-) => Promise<LocationProcessorResult>;
+export type LocationProcessorRead = (location: LocationSpec) => Promise<Buffer>;