backend-common: Deprecate read in favour of readUrl in UrlReader

Signed-off-by: Johan Haals <johan.haals@gmail.com>

.changeset/calm-bottles-happen.md

@@ -0,0 +1,6 @@
+---
+'@backstage/backend-common': minor
+---
+
+**BREAKING CHANGE**: The `UrlReader` interface has been updated to require that `readUrl` is implemented. `readUrl` has previously been optional to implement but a warning has been logged when calling its predecessor `read`.
+The `read` method is now deprecated and will be removed in a future release.

.changeset/few-books-remember.md

@@ -0,0 +1,7 @@
+---
+'@backstage/plugin-catalog-backend': patch
+'@backstage/plugin-techdocs-node': patch
+'@backstage/plugin-techdocs-backend': patch
+---
+
+Replace usage of deprecataed `UrlReader.read` with `UrlReader.readUrl`.

packages/backend-common/src/reading/UrlReaderPredicateMux.ts

@@ -27,8 +27,6 @@ import {
   UrlReaderPredicateTuple,
 } from './types';
 
-const MIN_WARNING_INTERVAL_MS = 1000 * 60 * 15;
-
 function notAllowedMessage(url: string) {
   return (
     `Reading from '${url}' is not allowed. ` +
@@ -43,7 +41,6 @@ function notAllowedMessage(url: string) {
  */
 export class UrlReaderPredicateMux implements UrlReader {
   private readonly readers: UrlReaderPredicateTuple[] = [];
-  private readonly readerWarnings: Map<UrlReader, number> = new Map();
 
   constructor(private readonly logger: Logger) {}
 
@@ -71,23 +68,7 @@ export class UrlReaderPredicateMux implements UrlReader {
 
     for (const { predicate, reader } of this.readers) {
       if (predicate(parsed)) {
-        if (reader.readUrl) {
-          return reader.readUrl(url, options);
-        }
-        const now = Date.now();
-        const lastWarned = this.readerWarnings.get(reader) ?? 0;
-        if (now > lastWarned + MIN_WARNING_INTERVAL_MS) {
-          this.readerWarnings.set(reader, now);
-          this.logger.warn(
-            `No implementation of readUrl found for ${reader}, this method will be required in the ` +
-              `future and will replace the 'read' method. See the changelog for more details here: ` +
-              'https://github.com/backstage/backstage/blob/master/packages/backend-common/CHANGELOG.md#085',
-          );
-        }
-        const buffer = await reader.read(url);
-        return {
-          buffer: async () => buffer,
-        };
+        return reader.readUrl(url, options);
       }
     }
 

packages/backend-common/src/reading/types.ts

@@ -27,6 +27,7 @@ import { AbortSignal } from 'node-abort-controller';
 export type UrlReader = {
   /**
    * Reads a single file and return its content.
+   * @deprecated use readUrl instead.
    */
   read(url: string): Promise<Buffer>;
 
@@ -38,10 +39,9 @@ export type UrlReader = {
    * This is a replacement for the read method that supports options and
    * complex responses.
    *
-   * Use this whenever it is available, as the read method will be
-   * deprecated and eventually removed in a future release.
+   * Use this as the read method will be removed in a future release.
    */
-  readUrl?(url: string, options?: ReadUrlOptions): Promise<ReadUrlResponse>;
+  readUrl(url: string, options?: ReadUrlOptions): Promise<ReadUrlResponse>;
 
   /**
    * Reads a full or partial file tree.

plugins/catalog-backend-module-openapi/src/OpenApiRefProcessor.test.ts

@@ -48,6 +48,7 @@ describe('OpenApiRefProcessor', () => {
       const config = new ConfigReader({});
       const reader = {
         read: jest.fn(),
+        readUrl: jest.fn(),
         readTree: jest.fn(),
         search: jest.fn(),
       };

plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts

@@ -33,34 +33,24 @@ describe('CodeOwnersProcessor', () => {
     target: `https://github.com/backstage/backstage/blob/master/${basePath}catalog-info.yaml`,
   });
 
-  const mockReadResult = ({
-    error = undefined,
-    data = undefined,
-  }: {
-    error?: string;
-    data?: string;
-  } = {}) => {
-    if (error) {
-      throw Error(error);
-    }
-    return data;
-  };
-
   describe('preProcessEntity', () => {
     const setupTest = ({ kind = 'Component', spec = {} } = {}) => {
       const entity = { kind, spec };
-      const read = jest
-        .fn()
-        .mockResolvedValue(mockReadResult({ data: mockCodeOwnersText() }));
-
       const config = new ConfigReader({});
-      const reader = { read, readTree: jest.fn(), search: jest.fn() };
+      const reader = {
+        read: jest.fn(),
+        readTree: jest.fn(),
+        search: jest.fn(),
+        readUrl: jest.fn().mockResolvedValue({
+          buffer: jest.fn().mockResolvedValue(mockCodeOwnersText()),
+        }),
+      };
       const processor = CodeOwnersProcessor.fromConfig(config, {
         logger: getVoidLogger(),
         reader,
       });
 
-      return { entity, processor, read };
+      return { entity, processor };
     };
 
     it('should not modify existing owner', async () => {

plugins/catalog-backend/src/modules/codeowners/lib/read.test.ts

@@ -15,6 +15,7 @@
  */
 
 import { ConfigReader } from '@backstage/config';
+import { NotFoundError } from '@backstage/errors';
 import { ScmIntegrations } from '@backstage/integration';
 import { findCodeOwnerByTarget, readCodeOwners } from './read';
 
@@ -25,35 +26,32 @@ const mockCodeowners = `
 /docs   @acme/team-bar
 `;
 
-const mockReadResult = ({
-  error = undefined,
-  data = undefined,
-}: {
-  error?: string;
-  data?: string;
-} = {}) => {
-  if (error) {
-    throw Error(error);
-  }
-  return data;
-};
-
 describe('readCodeOwners', () => {
   it('should return found codeowners file', async () => {
-    const ownersText = mockCodeowners;
-    const read = jest
-      .fn()
-      .mockResolvedValue(mockReadResult({ data: ownersText }));
-    const reader = { read, readTree: jest.fn(), search: jest.fn() };
+    const reader = {
+      read: jest.fn(),
+      readUrl: jest.fn().mockResolvedValue({
+        buffer: jest.fn().mockResolvedValue(Buffer.from(mockCodeowners)),
+      }),
+      readTree: jest.fn(),
+      search: jest.fn(),
+    };
+
     const result = await readCodeOwners(reader, sourceUrl, [
       '.github/CODEOWNERS',
     ]);
-    expect(result).toEqual(ownersText);
+    expect(result).toEqual(mockCodeowners);
   });
 
   it('should return undefined when no codeowner', async () => {
-    const read = jest.fn().mockRejectedValue(mockReadResult());
-    const reader = { read, readTree: jest.fn(), search: jest.fn() };
+    const reader = {
+      read: jest.fn(),
+      readUrl: jest.fn().mockResolvedValue({
+        buffer: jest.fn().mockRejectedValue(undefined),
+      }),
+      readTree: jest.fn(),
+      search: jest.fn(),
+    };
 
     await expect(
       readCodeOwners(reader, sourceUrl, ['.github/CODEOWNERS']),
@@ -61,22 +59,31 @@ describe('readCodeOwners', () => {
   });
 
   it('should look at multiple locations', async () => {
-    const ownersText = mockCodeowners;
-    const read = jest
-      .fn()
-      .mockImplementationOnce(() => mockReadResult({ error: 'not found' }))
-      .mockResolvedValue(mockReadResult({ data: ownersText }));
-    const reader = { read, readTree: jest.fn(), search: jest.fn() };
+    const reader = {
+      read: jest.fn(),
+      readUrl: jest.fn().mockResolvedValue({
+        buffer: jest
+          .fn()
+          .mockRejectedValue(new NotFoundError('not found'))
+          .mockResolvedValue(mockCodeowners),
+      }),
+      readTree: jest.fn(),
+      search: jest.fn(),
+    };
 
     const result = await readCodeOwners(reader, sourceUrl, [
       '.github/CODEOWNERS',
       'docs/CODEOWNERS',
     ]);
 
-    expect(read.mock.calls.length).toBe(2);
-    expect(read.mock.calls[0]).toEqual([`${sourceUrl}.github/CODEOWNERS`]);
-    expect(read.mock.calls[1]).toEqual([`${sourceUrl}docs/CODEOWNERS`]);
-    expect(result).toEqual(ownersText);
+    expect(reader.readUrl.mock.calls.length).toBe(2);
+    expect(reader.readUrl.mock.calls[0]).toEqual([
+      `${sourceUrl}.github/CODEOWNERS`,
+    ]);
+    expect(reader.readUrl.mock.calls[1]).toEqual([
+      `${sourceUrl}docs/CODEOWNERS`,
+    ]);
+    expect(result).toEqual(mockCodeowners);
   });
 });
 
@@ -85,15 +92,18 @@ describe('findCodeOwnerByLocation', () => {
     target = 'https://github.com/backstage/backstage/blob/master/catalog-info.yaml',
     codeownersContents: codeOwnersContents = mockCodeowners,
   }: { target?: string; codeownersContents?: string } = {}) => {
-    const read = jest
-      .fn()
-      .mockResolvedValue(mockReadResult({ data: codeOwnersContents }));
-
     const scmIntegration = ScmIntegrations.fromConfig(
       new ConfigReader({}),
     ).byUrl(target);
 
-    const reader = { read, readTree: jest.fn(), search: jest.fn() };
+    const reader = {
+      read: jest.fn(),
+      readUrl: jest.fn().mockResolvedValue({
+        buffer: jest.fn().mockResolvedValue(codeOwnersContents),
+      }),
+      readTree: jest.fn(),
+      search: jest.fn(),
+    };
 
     return { target, reader, scmIntegration, codeOwnersContents };
   };

plugins/catalog-backend/src/modules/codeowners/lib/read.ts

@@ -28,14 +28,9 @@ export async function readCodeOwners(
 ): Promise<string | undefined> {
   const readOwnerLocation = async (path: string): Promise<string> => {
     const url = `${sourceUrl}${path}`;
-
-    if (reader.readUrl) {
-      const data = await reader.readUrl(url);
-      const buffer = await data.buffer();
-      return buffer.toString();
-    }
-    const data = await reader.read(url);
-    return data.toString();
+    const data = await reader.readUrl(url);
+    const buffer = await data.buffer();
+    return buffer.toString();
   };
 
   const candidates = codeownersPaths.map(readOwnerLocation);

plugins/catalog-backend/src/modules/core/PlaceholderProcessor.test.ts

@@ -31,8 +31,12 @@ import {
 const integrations = ScmIntegrations.fromConfig(new ConfigReader({}));
 
 describe('PlaceholderProcessor', () => {
-  const read: jest.MockedFunction<PlaceholderResolverRead> = jest.fn();
-  const reader: UrlReader = { read, readTree: jest.fn(), search: jest.fn() };
+  const reader: jest.Mocked<UrlReader> = {
+    read: jest.fn(),
+    readTree: jest.fn(),
+    search: jest.fn(),
+    readUrl: jest.fn(),
+  };
 
   beforeEach(() => {
     jest.resetAllMocks();
@@ -86,7 +90,7 @@ describe('PlaceholderProcessor', () => {
       spec: { a: [{ b: 'TEXT' }] },
     });
 
-    expect(read).not.toHaveBeenCalled();
+    expect(reader.readUrl).not.toHaveBeenCalled();
     expect(upperResolver).toHaveBeenCalledWith(
       expect.objectContaining({
         key: 'upper',
@@ -115,7 +119,7 @@ describe('PlaceholderProcessor', () => {
       processor.preProcessEntity(entity, { type: 'a', target: 'b' }, () => {}),
     ).resolves.toEqual(entity);
 
-    expect(read).not.toHaveBeenCalled();
+    expect(reader.readUrl).not.toHaveBeenCalled();
   });
 
   it('ignores unknown placeholders', async () => {
@@ -136,11 +140,13 @@ describe('PlaceholderProcessor', () => {
       processor.preProcessEntity(entity, { type: 'a', target: 'b' }, () => {}),
     ).resolves.toEqual(entity);
 
-    expect(read).not.toHaveBeenCalled();
+    expect(reader.readUrl).not.toHaveBeenCalled();
   });
 
   it('works with the text resolver', async () => {
-    read.mockResolvedValue(Buffer.from('TEXT', 'utf-8'));
+    reader.readUrl.mockResolvedValue({
+      buffer: jest.fn().mockResolvedValue(Buffer.from('TEXT', 'utf-8')),
+    });
     const processor = new PlaceholderProcessor({
       resolvers: { text: textPlaceholderResolver },
       reader,
@@ -169,15 +175,19 @@ describe('PlaceholderProcessor', () => {
       spec: { data: 'TEXT' },
     });
 
-    expect(read).toHaveBeenCalledWith(
+    expect(reader.readUrl).toHaveBeenCalledWith(
       'https://github.com/backstage/backstage/a/file.txt',
     );
   });
 
   it('works with the json resolver', async () => {
-    read.mockResolvedValue(
-      Buffer.from(JSON.stringify({ a: ['b', 7] }), 'utf-8'),
-    );
+    reader.readUrl.mockResolvedValue({
+      buffer: jest
+        .fn()
+        .mockResolvedValue(
+          Buffer.from(JSON.stringify({ a: ['b', 7] }), 'utf-8'),
+        ),
+    });
     const processor = new PlaceholderProcessor({
       resolvers: { json: jsonPlaceholderResolver },
       reader,
@@ -206,13 +216,17 @@ describe('PlaceholderProcessor', () => {
       spec: { data: { a: ['b', 7] } },
     });
 
-    expect(read).toHaveBeenCalledWith(
+    expect(reader.readUrl).toHaveBeenCalledWith(
       'https://github.com/backstage/backstage/a/b/file.json',
     );
   });
 
   it('works with the yaml resolver', async () => {
-    read.mockResolvedValue(Buffer.from('foo:\n  - bar: 7', 'utf-8'));
+    reader.readUrl.mockResolvedValue({
+      buffer: jest
+        .fn()
+        .mockResolvedValue(Buffer.from('foo:\n  - bar: 7', 'utf-8')),
+    });
     const processor = new PlaceholderProcessor({
       resolvers: { yaml: yamlPlaceholderResolver },
       reader,
@@ -241,13 +255,15 @@ describe('PlaceholderProcessor', () => {
       spec: { data: { foo: [{ bar: 7 }] } },
     });
 
-    expect(read).toHaveBeenCalledWith(
+    expect(reader.readUrl).toHaveBeenCalledWith(
       'https://github.com/backstage/backstage/a/file.yaml',
     );
   });
 
   it('resolves absolute path for absolute location', async () => {
-    read.mockResolvedValue(Buffer.from('TEXT', 'utf-8'));
+    reader.readUrl.mockResolvedValue({
+      buffer: jest.fn().mockResolvedValue(Buffer.from('TEXT', 'utf-8')),
+    });
     const processor = new PlaceholderProcessor({
       resolvers: { text: textPlaceholderResolver },
       reader,
@@ -280,13 +296,15 @@ describe('PlaceholderProcessor', () => {
       spec: { data: 'TEXT' },
     });
 
-    expect(read).toHaveBeenCalledWith(
+    expect(reader.readUrl).toHaveBeenCalledWith(
       'https://github.com/backstage/backstage/catalog-info.yaml',
     );
   });
 
   it('resolves absolute path for relative file location', async () => {
-    read.mockResolvedValue(Buffer.from('TEXT', 'utf-8'));
+    reader.readUrl.mockResolvedValue({
+      buffer: jest.fn().mockResolvedValue(Buffer.from('TEXT', 'utf-8')),
+    });
     const processor = new PlaceholderProcessor({
       resolvers: { text: textPlaceholderResolver },
       reader,
@@ -318,7 +336,7 @@ describe('PlaceholderProcessor', () => {
       spec: { data: 'TEXT' },
     });
 
-    expect(read).toHaveBeenCalledWith(
+    expect(reader.readUrl).toHaveBeenCalledWith(
       'https://github.com/backstage/backstage/catalog-info.yaml',
     );
   });
@@ -327,7 +345,9 @@ describe('PlaceholderProcessor', () => {
     // We explicitly don't support this case, as it would allow for file system
     // traversal attacks. If we want to implement this, we need to have additional
     // security measures in place!
-    read.mockResolvedValue(Buffer.from('TEXT', 'utf-8'));
+    reader.readUrl.mockResolvedValue({
+      buffer: jest.fn().mockResolvedValue(Buffer.from('TEXT', 'utf-8')),
+    });
     const processor = new PlaceholderProcessor({
       resolvers: { text: textPlaceholderResolver },
       reader,
@@ -356,12 +376,16 @@ describe('PlaceholderProcessor', () => {
       /^Placeholder \$text could not form a URL out of \.\/a\/b\/catalog-info\.yaml and \.\.\/c\/catalog-info\.yaml, TypeError \[ERR_INVALID_URL\]/,
     );
 
-    expect(read).not.toHaveBeenCalled();
+    expect(reader.readUrl).not.toHaveBeenCalled();
   });
   it('should emit the resolverValue as a refreshKey', async () => {
-    read.mockResolvedValue(
-      Buffer.from(JSON.stringify({ a: ['b', 7] }), 'utf-8'),
-    );
+    reader.readUrl.mockResolvedValue({
+      buffer: jest
+        .fn()
+        .mockResolvedValue(
+          Buffer.from(JSON.stringify({ a: ['b', 7] }), 'utf-8'),
+        ),
+    });
 
     const processor = new PlaceholderProcessor({
       resolvers: {

plugins/catalog-backend/src/modules/core/PlaceholderProcessor.ts

@@ -120,12 +120,9 @@ export class PlaceholderProcessor implements CatalogProcessor {
       }
 
       const read = async (url: string): Promise<Buffer> => {
-        if (this.options.reader.readUrl) {
-          const response = await this.options.reader.readUrl(url);
-          const buffer = await response.buffer();
-          return buffer;
-        }
-        return this.options.reader.read(url);
+        const response = await this.options.reader.readUrl(url);
+        const buffer = await response.buffer();
+        return buffer;
       };
 
       const resolveUrl = (url: string, base: string): string =>

plugins/catalog-backend/src/modules/core/UrlReaderProcessor.test.ts

@@ -195,6 +195,7 @@ describe('UrlReaderProcessor', () => {
 
     const reader: jest.Mocked<UrlReader> = {
       read: jest.fn(),
+      readUrl: jest.fn(),
       readTree: jest.fn(),
       search: jest.fn().mockImplementation(async () => []),
     };

plugins/catalog-backend/src/modules/core/UrlReaderProcessor.ts

@@ -135,16 +135,10 @@ export class UrlReaderProcessor implements CatalogProcessor {
       return { response: await Promise.all(output), etag: response.etag };
     }
 
-    // Otherwise do a plain read, prioritizing readUrl if available
-    if (this.options.reader.readUrl) {
-      const data = await this.options.reader.readUrl(location, { etag });
-      return {
-        response: [{ url: location, data: await data.buffer() }],
-        etag: data.etag,
-      };
-    }
-
-    const data = await this.options.reader.read(location);
-    return { response: [{ url: location, data }] };
+    const data = await this.options.reader.readUrl(location, { etag });
+    return {
+      response: [{ url: location, data: await data.buffer() }],
+      etag: data.etag,
+    };
   }
 }

plugins/scaffolder-backend-module-cookiecutter/src/actions/fetch/cookiecutter.test.ts

@@ -74,6 +74,7 @@ describe('fetch:cookiecutter', () => {
   };
 
   const mockReader: UrlReader = {
+    readUrl: jest.fn(),
     read: jest.fn(),
     readTree: jest.fn(),
     search: jest.fn(),

plugins/scaffolder-backend-module-rails/src/actions/fetch/rails/index.test.ts

@@ -75,6 +75,7 @@ describe('fetch:rails', () => {
 
   const mockReader: UrlReader = {
     read: jest.fn(),
+    readUrl: jest.fn(),
     readTree: jest.fn(),
     search: jest.fn(),
   };

plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/helpers.test.ts

@@ -40,6 +40,7 @@ describe('fetchContent helper', () => {
   const readTree = jest.fn();
   const reader: UrlReader = {
     read: jest.fn(),
+    readUrl: jest.fn(),
     readTree,
     search: jest.fn(),
   };

plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/plain.test.ts

@@ -33,6 +33,7 @@ describe('fetch:plain', () => {
     }),
   );
   const reader: UrlReader = {
+    readUrl: jest.fn(),
     read: jest.fn(),
     readTree: jest.fn(),
     search: jest.fn(),

plugins/techdocs-backend/src/service/standaloneServer.ts

@@ -54,6 +54,7 @@ export async function startStandaloneServer(
   const discovery = SingleHostDiscovery.fromConfig(config);
   const mockUrlReader: jest.Mocked<UrlReader> = {
     read: jest.fn(),
+    readUrl: jest.fn(),
     readTree: jest.fn(),
     search: jest.fn(),
   };

plugins/techdocs-node/src/helpers.test.ts

@@ -16,6 +16,8 @@
 
 import {
   ReadTreeResponse,
+  ReadUrlOptions,
+  ReadUrlResponse,
   SearchResponse,
   UrlReader,
 } from '@backstage/backend-common';
@@ -292,6 +294,13 @@ describe('getDocFilesFromRepository', () => {
         return Buffer.from('mock');
       }
 
+      async readUrl(
+        _url: string,
+        _options?: ReadUrlOptions | undefined,
+      ): Promise<ReadUrlResponse> {
+        throw new Error('Method not implemented.');
+      }
+
       async readTree(): Promise<ReadTreeResponse> {
         return {
           dir: async () => {

plugins/techdocs-node/src/stages/prepare/dir.test.ts

@@ -46,6 +46,7 @@ const createMockEntity = (annotations: {}) => {
 const mockConfig = new ConfigReader({});
 const mockUrlReader: jest.Mocked<UrlReader> = {
   read: jest.fn(),
+  readUrl: jest.fn(),
   readTree: jest.fn(),
   search: jest.fn(),
 };

plugins/todo-backend/src/lib/TodoReader/TodoScmReader.test.ts

@@ -30,6 +30,7 @@ function mockReader(): jest.Mocked<UrlReader> {
     read: jest.fn(),
     readTree: jest.fn(),
     search: jest.fn(),
+    readUrl: jest.fn(),
   } as jest.Mocked<UrlReader>;
 }