implement review suggestions

Author: hopehadfield

workspaces/extensions/plugins/catalog-backend-module-extensions/src/module.ts

@@ -73,10 +73,10 @@ export const catalogModuleExtensions = createBackendModule({
         const catalogApi = new CatalogClient({ discoveryApi: discovery });
 
         catalog.addEntityProvider(
-          new ExtensionsPackageProvider(taskRunner, config),
+          new ExtensionsPackageProvider(taskRunner, config, logger),
         );
         catalog.addEntityProvider(
-          new ExtensionsPluginProvider(delayedTaskRunner, config),
+          new ExtensionsPluginProvider(delayedTaskRunner, config, logger),
         );
         // Disabling the collection provider as collections/all.yaml is already commented in RHDH 1.5 image.
         // catalog.addEntityProvider(

workspaces/extensions/plugins/catalog-backend-module-extensions/src/providers/BaseEntityProvider.test.ts

@@ -15,7 +15,10 @@
  */
 
 import { Entity } from '@backstage/catalog-model';
-import { SchedulerServiceTaskRunner } from '@backstage/backend-plugin-api';
+import {
+  LoggerService,
+  SchedulerServiceTaskRunner,
+} from '@backstage/backend-plugin-api';
 import { BaseEntityProvider } from './BaseEntityProvider';
 import { JsonFileData } from '../types';
 
@@ -32,6 +35,13 @@ class TestEntityProvider extends BaseEntityProvider<Entity> {
 const taskRunner: SchedulerServiceTaskRunner = {
   run: jest.fn(async ({ fn }) => fn(new AbortController().signal)),
 };
+const logger: LoggerService = {
+  warn: jest.fn(),
+  error: jest.fn(),
+  info: jest.fn(),
+  debug: jest.fn(),
+  child: jest.fn(),
+};
 
 const createEntity = (overrides?: Partial<Entity>): Entity => ({
   apiVersion: 'extensions.backstage.io/v1alpha1',
@@ -57,15 +67,15 @@ const createFileData = (
 
 describe('BaseEntityProvider collision policy', () => {
   beforeEach(() => {
-    jest.spyOn(console, 'warn').mockImplementation(() => undefined);
+    jest.clearAllMocks();
   });
 
   afterEach(() => {
     jest.restoreAllMocks();
   });
 
   it('keeps first definition when duplicate entities are equivalent', () => {
-    const provider = new TestEntityProvider(taskRunner);
+    const provider = new TestEntityProvider(taskRunner, undefined, logger);
     const duplicate = createEntity();
 
     const entities = provider.getEntities([
@@ -74,34 +84,37 @@ describe('BaseEntityProvider collision policy', () => {
     ]);
 
     expect(entities).toHaveLength(1);
-    expect(console.warn).toHaveBeenCalledWith(
+    expect(logger.warn).toHaveBeenCalledWith(
       expect.stringContaining(
-        "Skipping duplicate Extensions entity 'plugin/default/duplicate-plugin'",
+        "Skipping duplicate Extensions entity 'plugin:default/duplicate-plugin'",
       ),
     );
   });
 
-  it('throws when duplicate entities have conflicting definitions', () => {
-    const provider = new TestEntityProvider(taskRunner);
+  it('warns and skips when duplicate entities have conflicting definitions', () => {
+    const provider = new TestEntityProvider(taskRunner, undefined, logger);
     const firstEntity = createEntity({
       spec: { owner: 'owner-a' },
     });
     const secondEntity = createEntity({
       spec: { owner: 'owner-b' },
     });
 
-    expect(() =>
-      provider.getEntities([
-        createFileData('/extensions/primary/plugin.yaml', firstEntity),
-        createFileData('/extensions/extra/community/plugin.yaml', secondEntity),
-      ]),
-    ).toThrow(
-      "Conflicting Extensions entities detected for 'plugin/default/duplicate-plugin'",
+    const entities = provider.getEntities([
+      createFileData('/extensions/primary/plugin.yaml', firstEntity),
+      createFileData('/extensions/extra/community/plugin.yaml', secondEntity),
+    ]);
+
+    expect(entities).toHaveLength(1);
+    expect(logger.warn).toHaveBeenCalledWith(
+      expect.stringContaining(
+        "Conflicting Extensions entities detected for 'plugin:default/duplicate-plugin'",
+      ),
     );
   });
 
   it('keeps entities with same name when namespaces differ', () => {
-    const provider = new TestEntityProvider(taskRunner);
+    const provider = new TestEntityProvider(taskRunner, undefined, logger);
     const defaultNamespaceEntity = createEntity({
       metadata: { name: 'shared-name' },
     });

workspaces/extensions/plugins/catalog-backend-module-extensions/src/providers/BaseEntityProvider.ts

@@ -13,11 +13,15 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { SchedulerServiceTaskRunner } from '@backstage/backend-plugin-api';
+import {
+  LoggerService,
+  SchedulerServiceTaskRunner,
+} from '@backstage/backend-plugin-api';
 import {
   ANNOTATION_LOCATION,
   ANNOTATION_ORIGIN_LOCATION,
   Entity,
+  stringifyEntityRef,
 } from '@backstage/catalog-model';
 import {
   EntityProvider,
@@ -39,27 +43,24 @@ export abstract class BaseEntityProvider<T extends Entity>
   private connection?: EntityProviderConnection;
   private taskRunner: SchedulerServiceTaskRunner;
   private config?: Config;
+  private logger?: LoggerService;
 
   private static readonly EXTENSIONS_DIRECTORY = '/extensions';
   private static readonly DEPRECATED_MARKETPLACE_DIRECTORY = '/marketplace';
 
-  constructor(taskRunner: SchedulerServiceTaskRunner, config?: Config) {
+  constructor(
+    taskRunner: SchedulerServiceTaskRunner,
+    config?: Config,
+    logger?: LoggerService,
+  ) {
     this.taskRunner = taskRunner;
     this.config = config;
+    this.logger = logger;
   }
 
   abstract getProviderName(): string;
   abstract getKind(): string;
 
-  private getEntityIdentity(entity: Entity): string {
-    const namespace = entity.metadata.namespace ?? 'default';
-    return [
-      entity.kind.toLocaleLowerCase('en-US'),
-      namespace.toLocaleLowerCase('en-US'),
-      entity.metadata.name.toLocaleLowerCase('en-US'),
-    ].join('/');
-  }
-
   private addProviderAnnotations(entity: T): T {
     return {
       ...entity,
@@ -79,7 +80,7 @@ export abstract class BaseEntityProvider<T extends Entity>
       return [];
     }
 
-    const entitiesByIdentity = new Map<
+    const entitiesByEntityRef = new Map<
       string,
       { entity: T; filePath: string }
     >();
@@ -89,29 +90,34 @@ export abstract class BaseEntityProvider<T extends Entity>
         continue;
       }
 
-      const identity = this.getEntityIdentity(fileData.content);
-      const existing = entitiesByIdentity.get(identity);
+      const identity = stringifyEntityRef({
+        kind: fileData.content.kind,
+        namespace: fileData.content.metadata.namespace ?? 'default',
+        name: fileData.content.metadata.name,
+      }).toLocaleLowerCase('en-US');
+      const existing = entitiesByEntityRef.get(identity);
       if (!existing) {
-        entitiesByIdentity.set(identity, {
+        entitiesByEntityRef.set(identity, {
           entity: fileData.content,
           filePath: fileData.filePath,
         });
         continue;
       }
 
       if (isDeepStrictEqual(existing.entity, fileData.content)) {
-        console.warn(
+        this.logger?.warn(
           `Skipping duplicate Extensions entity '${identity}' from '${fileData.filePath}'. Keeping first definition from '${existing.filePath}'.`,
         );
         continue;
       }
 
-      throw new Error(
-        `Conflicting Extensions entities detected for '${identity}' in '${existing.filePath}' and '${fileData.filePath}'.`,
+      this.logger?.warn(
+        `Conflicting Extensions entities detected for '${identity}' in '${existing.filePath}' and '${fileData.filePath}'. Skipping conflicting definition from '${fileData.filePath}'.`,
       );
+      continue;
     }
 
-    return Array.from(entitiesByIdentity.values()).map(({ entity }) =>
+    return Array.from(entitiesByEntityRef.values()).map(({ entity }) =>
       this.addProviderAnnotations(entity),
     );
   }