feat: add config flag

Signed-off-by: Thomas Cardonne <thomas.cardonne@adevinta.com>

.changeset/tiny-spoons-repeat.md

@@ -3,9 +3,13 @@
 '@backstage/plugin-catalog-backend': minor
 ---
 
-**BREAKING:** The `UrlReaderProccessor` now always calls the `search` method of the `UrlReaders`. Previous behavior was to call the `search` method only if the parsed Git URL's filename contained a wildcard and use `readUrl` otherwise. `UrlReaderService` must implement this logic in the `search` method instead.
+**BREAKING**: The `UrlReaderProccessor` accepts a new config flag `catalog.useUrlReadersSearch` to always call the `search` method of `UrlReaders`.
+
+This flag currently defaults to `false`, but adopters are encouraged to enable it as this behavior will be the default in a future release.
+
+Previous behavior was to call the `search` method only if the parsed Git URL's filename contained a wildcard and use `readUrl` otherwise. `UrlReaderService` must implement this logic in the `search` method instead.
 
 This allows each `UrlReaderService` implementation to check whether it's a search URL (that contains a wildcard pattern) or not using logic that is specific to each provider.
 
 In the different `UrlReadersService`, the `search` method have been updated to use the `readUrl` if the given URL doesn't contain a pattern.
-For `UrlReaders` that didn't implement the `search` method, `readUrl` is called internally.
+For `UrlReaders` that didn't implement the `search` method, `readUrl` is now called internally.

app-config.yaml

@@ -244,6 +244,7 @@ integrations:
       secretAccessKey: ${AWS_SECRET_ACCESS_KEY}
 
 catalog:
+  useUrlReadersSearch: true
   import:
     entityFilename: catalog-info.yaml
     pullRequestBranchName: backstage-integration

plugins/catalog-backend/config.d.ts

@@ -202,5 +202,19 @@ export interface Config {
      * housing catalog-info files.
      */
     processingInterval?: HumanDuration | false;
+
+    /**
+     * Defines if the UrlReaderProcessor should always call the search method of the
+     * different UrlReaders.
+     *
+     * If set to false, the UrlReaderProcessor will use the legacy behavior that tries to
+     * parse a Git URL and calls search if there's wildcard patterns and readUrl otherwise.
+     *
+     * If set to true, the UrlReaderProcessor always call the search method and lets each UrlReader
+     * determine if it's a search pattern or not.
+     *
+     * This flag is temporary and will be enabled by default in future releases.
+     */
+    useUrlReadersSearch?: boolean;
   };
 }

plugins/catalog-backend/package.json

@@ -103,6 +103,7 @@
     "@backstage/cli": "workspace:^",
     "@backstage/plugin-permission-common": "workspace:^",
     "@backstage/repo-tools": "workspace:^",
+    "@backstage/test-utils": "workspace:^",
     "@types/core-js": "^2.5.4",
     "@types/git-url-parse": "^9.0.0",
     "@types/glob": "^8.0.0",

plugins/catalog-backend/report.api.md

@@ -494,7 +494,11 @@ export function transformLegacyPolicyToProcessor(
 
 // @public (undocumented)
 export class UrlReaderProcessor implements CatalogProcessor_2 {
-  constructor(options: { reader: UrlReaderService; logger: LoggerService });
+  constructor(options: {
+    reader: UrlReaderService;
+    logger: LoggerService;
+    config?: Config;
+  });
   // (undocumented)
   getProcessorName(): string;
   // (undocumented)

plugins/catalog-backend/src/processors/UrlReaderProcessor.test.ts

@@ -31,6 +31,8 @@ import {
 import { defaultEntityDataParser } from '../util/parse';
 import { UrlReaderProcessor } from './UrlReaderProcessor';
 import { UrlReaders } from '@backstage/backend-defaults/urlReader';
+import { UrlReaderService } from '@backstage/backend-plugin-api';
+import { mockApis } from '@backstage/test-utils';
 
 describe('UrlReaderProcessor', () => {
   const mockApiOrigin = 'http://localhost';
@@ -184,7 +186,7 @@ describe('UrlReaderProcessor', () => {
     expect(generated.location).toBe(spec);
     expect(generated.error.name).toBe('NotFoundError');
     expect(generated.error.message).toBe(
-      `Unable to read url, no matching files found for ${mockApiOrigin}/component-notfound.yaml`,
+      `Unable to read url, NotFoundError: could not read ${mockApiOrigin}/component-notfound.yaml, 404 Not Found`,
     );
   });
 
@@ -209,4 +211,36 @@ describe('UrlReaderProcessor', () => {
 
     expect(reader.search).toHaveBeenCalledTimes(1);
   });
+
+  it('uses search when catalog.useUrlReadersSearch flag is set to true', async () => {
+    const logger = mockServices.logger.mock();
+
+    const reader: jest.Mocked<UrlReaderService> = {
+      readUrl: jest.fn(),
+      readTree: jest.fn(),
+      search: jest.fn().mockImplementation(async () => []),
+    };
+
+    const config = mockApis.config({
+      data: {
+        catalog: {
+          useUrlReadersSearch: true,
+        },
+      },
+    });
+
+    const processor = new UrlReaderProcessor({ reader, logger, config });
+
+    const emit = jest.fn();
+
+    await processor.readLocation(
+      { type: 'url', target: 'https://github.com/a/b/blob/x/b.yaml' },
+      false,
+      emit,
+      defaultEntityDataParser,
+      mockCache,
+    );
+
+    expect(reader.search).toHaveBeenCalledTimes(1);
+  });
 });

plugins/catalog-backend/src/processors/UrlReaderProcessor.ts

@@ -18,6 +18,7 @@ import { Entity } from '@backstage/catalog-model';
 import { assertError } from '@backstage/errors';
 import limiterFactory, { Limit } from 'p-limit';
 import { LocationSpec } from '@backstage/plugin-catalog-common';
+import parseGitUrl from 'git-url-parse';
 import {
   CatalogProcessor,
   CatalogProcessorCache,
@@ -28,6 +29,7 @@ import {
   processingResult,
 } from '@backstage/plugin-catalog-node';
 import { LoggerService, UrlReaderService } from '@backstage/backend-plugin-api';
+import { Config } from '@backstage/config';
 
 const CACHE_KEY = 'v1';
 
@@ -46,14 +48,25 @@ export class UrlReaderProcessor implements CatalogProcessor {
   // This limiter is used for only consuming a limited number of read streams
   // concurrently.
   #limiter: Limit;
+  #useUrlReadersSearch: boolean;
 
   constructor(
     private readonly options: {
       reader: UrlReaderService;
       logger: LoggerService;
+      config?: Config;
     },
   ) {
     this.#limiter = limiterFactory(5);
+
+    this.#useUrlReadersSearch =
+      this.options.config?.getOptionalBoolean('catalog.useUrlReadersSearch') ||
+      false;
+    if (!this.#useUrlReadersSearch) {
+      this.options.logger.warn(
+        'UrlReaderProcessor uses the legacy readUrl/search behavior which will be removed in a future release. Set catalog.useUrlReadersSearch to true to adopt the new behavior.',
+      );
+    }
   }
 
   getProcessorName() {
@@ -120,6 +133,10 @@ export class UrlReaderProcessor implements CatalogProcessor {
         }
         emit(processingResult.refresh(`${location.type}:${location.target}`));
         await cache.set(CACHE_KEY, cacheItem);
+      } else if (error.name === 'NotFoundError') {
+        if (!optional) {
+          emit(processingResult.notFoundError(location, message));
+        }
       } else {
         emit(processingResult.generalError(location, message));
       }
@@ -132,13 +149,35 @@ export class UrlReaderProcessor implements CatalogProcessor {
     location: string,
     etag?: string,
   ): Promise<{ response: { data: Buffer; url: string }[]; etag?: string }> {
-    const response = await this.options.reader.search(location, { etag });
+    // New behavior: always use the search method
+    if (this.#useUrlReadersSearch) {
+      const response = await this.options.reader.search(location, { etag });
 
-    const output = response.files.map(async file => ({
-      url: file.url,
-      data: await this.#limiter(file.content),
-    }));
+      const output = response.files.map(async file => ({
+        url: file.url,
+        data: await this.#limiter(file.content),
+      }));
 
-    return { response: await Promise.all(output), etag: response.etag };
+      return { response: await Promise.all(output), etag: response.etag };
+    }
+
+    // Old behavior: Does it contain globs? I.e. does it contain asterisks or question marks
+    // (no curly braces for now)
+
+    const { filepath } = parseGitUrl(location);
+    if (filepath?.match(/[*?]/)) {
+      const response = await this.options.reader.search(location, { etag });
+      const output = response.files.map(async file => ({
+        url: file.url,
+        data: await this.#limiter(file.content),
+      }));
+      return { response: await Promise.all(output), etag: response.etag };
+    }
+
+    const data = await this.options.reader.readUrl(location, { etag });
+    return {
+      response: [{ url: location, data: await data.buffer() }],
+      etag: data.etag,
+    };
   }
 }

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

@@ -380,7 +380,7 @@ export class CatalogBuilder {
 
     return [
       new FileReaderProcessor(),
-      new UrlReaderProcessor({ reader, logger }),
+      new UrlReaderProcessor({ reader, logger, config }),
       CodeOwnersProcessor.fromConfig(config, { logger, reader }),
       new AnnotateLocationEntityProcessor({ integrations }),
     ];

yarn.lock

@@ -6001,6 +6001,7 @@ __metadata:
     "@backstage/plugin-search-backend-module-catalog": "workspace:^"
     "@backstage/plugin-search-common": "workspace:^"
     "@backstage/repo-tools": "workspace:^"
+    "@backstage/test-utils": "workspace:^"
     "@backstage/types": "workspace:^"
     "@opentelemetry/api": ^1.9.0
     "@types/core-js": ^2.5.4