feat(catalog)!: remove catalog.useUrlReadersSearch and always use search

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

.changeset/cyan-islands-swim.md

@@ -0,0 +1,14 @@
+---
+'@backstage/plugin-catalog-backend': major
+'@backstage/create-app': patch
+---
+
+**BREAKING:** The experimental `catalog.useUrlReadersSearch` configuration flag (introduced in v1.36) has been removed.
+
+The `UrlReaderProcessor` now always uses the `search` method of `UrlReaders`. Built-in `UrlReaderService` implementations have been updated accordingly.
+If you use custom `UrlReaderService` implementations, you need to adapt their `search` method to correctly handle both specific URLs and potential
+search patterns (see changes on built-in readers [in the original PR](https://github.com/backstage/backstage/pull/28379/files#diff-68b0452f173ee54bdd40f7b5e047a9cb8bb59200425622c212c217b76dac1d1b)).
+
+Previous behavior was to call the `search` method only if the parsed Git URL's filename contained a wildcard and use `readUrl` otherwise. Each `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.

app-config.yaml

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

packages/create-app/templates/default-app/app-config.yaml.hbs

@@ -103,9 +103,6 @@ catalog:
     #   target: https://github.com/backstage/backstage/blob/master/packages/catalog-model/examples/acme-corp.yaml
     #   rules:
     #     - allow: [User, Group]
-  # Experimental: Always use the search method in UrlReaderProcessor.
-  # New adopters are encouraged to enable it as this behavior will be the default in a future release.
-  useUrlReadersSearch: true
 
 kubernetes:
   # see https://backstage.io/docs/features/kubernetes/configuration for kubernetes configuration options

plugins/catalog-backend/config.d.ts

@@ -218,19 +218,5 @@ 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

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

plugins/catalog-backend/report.api.md

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

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

@@ -30,8 +30,6 @@ 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';
@@ -191,11 +189,11 @@ describe('UrlReaderProcessor', () => {
     expect(generated.location).toBe(spec);
     expect(generated.error.name).toBe('NotFoundError');
     expect(generated.error.message).toBe(
-      `Unable to read url, NotFoundError: could not read ${mockApiOrigin}/component-notfound.yaml, 404 Not Found`,
+      `Unable to read url, no matching files found for ${mockApiOrigin}/component-notfound.yaml`,
     );
   });
 
-  it('uses search when there are globs', async () => {
+  it("uses reader' search method", async () => {
     const logger = mockServices.logger.mock();
 
     const reader = mockServices.urlReader.mock({
@@ -206,38 +204,6 @@ describe('UrlReaderProcessor', () => {
 
     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);
-  });
-
-  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,

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

@@ -18,7 +18,6 @@ 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,
@@ -29,7 +28,6 @@ import {
   processingResult,
 } from '@backstage/plugin-catalog-node';
 import { LoggerService, UrlReaderService } from '@backstage/backend-plugin-api';
-import { Config } from '@backstage/config';
 
 const CACHE_KEY = 'v1';
 
@@ -48,25 +46,14 @@ 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() {
@@ -149,35 +136,13 @@ export class UrlReaderProcessor implements CatalogProcessor {
     location: string,
     etag?: string,
   ): Promise<{ response: { data: Buffer; url: string }[]; etag?: string }> {
-    // New behavior: always use the search method
-    if (this.#useUrlReadersSearch) {
-      const response = await this.options.reader.search(location, { etag });
+    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 };
-    }
-
-    // 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,
-    };
+    return { response: await Promise.all(output), etag: response.etag };
   }
 }

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

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

yarn.lock

@@ -6135,7 +6135,6 @@ __metadata:
     "@backstage/plugin-permission-common": "workspace:^"
     "@backstage/plugin-permission-node": "workspace:^"
     "@backstage/repo-tools": "workspace:^"
-    "@backstage/test-utils": "workspace:^"
     "@backstage/types": "workspace:^"
     "@opentelemetry/api": "npm:^1.9.0"
     "@types/core-js": "npm:^2.5.4"