Move onConflict refresh logic into DefaultLocationStore

The store now owns the conflict resolution and refresh logic directly,
keeping DefaultLocationService as a thin pass-through layer.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Fredrik Adelöw <freben@spotify.com>

plugins/catalog-backend/src/providers/DefaultLocationStore.ts

@@ -29,13 +29,14 @@ import {
   EntityProviderConnection,
 } from '@backstage/plugin-catalog-node';
 import { locationSpecToLocationEntity } from '../util/conversion';
-import { LocationInput, LocationStore } from '../service/types';
+import { LocationInput, LocationStore, RefreshService } from '../service/types';
 import {
   ANNOTATION_ORIGIN_LOCATION,
   CompoundEntityRef,
   parseLocationRef,
   stringifyEntityRef,
 } from '@backstage/catalog-model';
+import { BackstageCredentials } from '@backstage/backend-plugin-api';
 import {
   CatalogScmEvent,
   CatalogScmEventsService,
@@ -53,6 +54,7 @@ export class DefaultLocationStore implements LocationStore, EntityProvider {
   private readonly db: Knex;
   private readonly scmEvents: CatalogScmEventsService;
   private readonly scmEventHandlingConfig: ScmEventHandlingConfig;
+  private refreshService?: RefreshService;
 
   constructor(
     db: Knex,
@@ -64,20 +66,36 @@ export class DefaultLocationStore implements LocationStore, EntityProvider {
     this.scmEventHandlingConfig = scmEventHandlingConfig;
   }
 
+  setRefreshService(refreshService: RefreshService): void {
+    this.refreshService = refreshService;
+  }
+
   getProviderName(): string {
     return 'DefaultLocationStore';
   }
 
-  async createLocation(input: LocationInput): Promise<Location> {
+  async createLocation(
+    input: LocationInput,
+    options?: {
+      onConflict?: 'refresh' | 'reject';
+      credentials?: BackstageCredentials;
+    },
+  ): Promise<Location> {
+    let existed = false;
+
     const location = await this.db.transaction(async tx => {
       // Attempt to find a previous location matching the input
       const previousLocations = await this.locations(tx);
       // TODO: when location id's are a compilation of input target we can remove this full
       // lookup of locations first and just grab the by that instead.
-      const previousLocation = previousLocations.some(
+      const previousLocation = previousLocations.find(
         l => input.type === l.type && input.target === l.target,
       );
       if (previousLocation) {
+        if (options?.onConflict === 'refresh') {
+          existed = true;
+          return previousLocation;
+        }
         throw new ConflictError(
           `Location ${input.type}:${input.target} already exists`,
         );
@@ -93,12 +111,33 @@ export class DefaultLocationStore implements LocationStore, EntityProvider {
 
       return inner;
     });
-    const entity = locationSpecToLocationEntity({ location });
-    await this.connection.applyMutation({
-      type: 'delta',
-      added: [{ entity, locationKey: getEntityLocationRef(entity) }],
-      removed: [],
-    });
+
+    if (existed) {
+      if (!this.refreshService) {
+        throw new InputError(
+          'onConflict refresh is not supported: no refresh service available',
+        );
+      }
+      if (!options?.credentials) {
+        throw new InputError(
+          'onConflict refresh requires credentials to be provided',
+        );
+      }
+      const entityRef = stringifyEntityRef(
+        locationSpecToLocationEntity({ location }),
+      );
+      await this.refreshService.refresh({
+        entityRef,
+        credentials: options.credentials,
+      });
+    } else {
+      const entity = locationSpecToLocationEntity({ location });
+      await this.connection.applyMutation({
+        type: 'delta',
+        added: [{ entity, locationKey: getEntityLocationRef(entity) }],
+        removed: [],
+      });
+    }
 
     return location;
   }

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

@@ -594,10 +594,10 @@ export class CatalogBuilder {
     const innerRefreshService = new DefaultRefreshService({
       database: catalogDatabase,
     });
+    locationStore.setRefreshService(innerRefreshService);
     const locationService = new AuthorizedLocationService(
       new DefaultLocationService(locationStore, orchestrator, {
         allowedLocationTypes: this.allowedLocationType,
-        refreshService: innerRefreshService,
       }),
       permissionsService,
     );

plugins/catalog-backend/src/service/DefaultLocationService.test.ts

@@ -16,8 +16,8 @@
 
 import { DefaultLocationService } from './DefaultLocationService';
 import { CatalogProcessingOrchestrator } from '../processing/types';
-import { LocationStore, RefreshService } from './types';
-import { ConflictError, InputError } from '@backstage/errors';
+import { LocationStore } from './types';
+import { InputError } from '@backstage/errors';
 
 describe('DefaultLocationServiceTest', () => {
   const orchestrator: jest.Mocked<CatalogProcessingOrchestrator> = {
@@ -257,10 +257,13 @@ describe('DefaultLocationServiceTest', () => {
           type: 'url',
         },
       });
-      expect(store.createLocation).toHaveBeenCalledWith({
-        target: 'https://backstage.io/catalog-info.yaml',
-        type: 'url',
-      });
+      expect(store.createLocation).toHaveBeenCalledWith(
+        {
+          target: 'https://backstage.io/catalog-info.yaml',
+          type: 'url',
+        },
+        { onConflict: undefined },
+      );
     });
 
     it('should create location with unknown type if configuration allows it', async () => {
@@ -291,10 +294,13 @@ describe('DefaultLocationServiceTest', () => {
           type: 'unknown',
         },
       });
-      expect(store.createLocation).toHaveBeenCalledWith({
-        target: 'https://backstage.io/catalog-info.yaml',
-        type: 'unknown',
-      });
+      expect(store.createLocation).toHaveBeenCalledWith(
+        {
+          target: 'https://backstage.io/catalog-info.yaml',
+          type: 'unknown',
+        },
+        { onConflict: undefined },
+      );
     });
 
     it('should not allow locations of unknown types by default', async () => {
@@ -309,86 +315,33 @@ describe('DefaultLocationServiceTest', () => {
       ).rejects.toThrow(InputError);
     });
 
-    it('should refresh existing location on conflict when onConflict is refresh', async () => {
+    it('should pass onConflict and credentials through to store', async () => {
       const locationSpec = {
         type: 'url',
         target: 'https://backstage.io/catalog-info.yaml',
       };
 
-      const refreshService: jest.Mocked<RefreshService> = {
-        refresh: jest.fn(),
-      };
-
-      const locationServiceWithRefresh = new DefaultLocationService(
-        store,
-        orchestrator,
-        { allowedLocationTypes: ['url'], refreshService },
-      );
-
-      store.createLocation.mockRejectedValueOnce(
-        new ConflictError(
-          `Location ${locationSpec.type}:${locationSpec.target} already exists`,
-        ),
-      );
-      store.getLocationByEntity.mockResolvedValueOnce({
+      store.createLocation.mockResolvedValueOnce({
         id: 'existing-id',
         ...locationSpec,
       });
 
       const credentials = {} as any;
-      const result = await locationServiceWithRefresh.createLocation(
-        locationSpec,
-        false,
-        { onConflict: 'refresh', credentials },
-      );
+      const result = await locationService.createLocation(locationSpec, false, {
+        onConflict: 'refresh',
+        credentials,
+      });
 
       expect(result).toEqual({
         location: { id: 'existing-id', ...locationSpec },
         entities: [],
       });
-      expect(refreshService.refresh).toHaveBeenCalledWith({
-        entityRef: expect.stringMatching(/^location:default\/generated-/),
+      expect(store.createLocation).toHaveBeenCalledWith(locationSpec, {
+        onConflict: 'refresh',
         credentials,
       });
     });
 
-    it('should still throw conflict error when onConflict is reject', async () => {
-      const locationSpec = {
-        type: 'url',
-        target: 'https://backstage.io/catalog-info.yaml',
-      };
-
-      store.createLocation.mockRejectedValueOnce(
-        new ConflictError(
-          `Location ${locationSpec.type}:${locationSpec.target} already exists`,
-        ),
-      );
-
-      await expect(
-        locationService.createLocation(locationSpec, false, {
-          onConflict: 'reject',
-          credentials: {} as any,
-        }),
-      ).rejects.toThrow(ConflictError);
-    });
-
-    it('should throw conflict error by default when onConflict is not set', async () => {
-      const locationSpec = {
-        type: 'url',
-        target: 'https://backstage.io/catalog-info.yaml',
-      };
-
-      store.createLocation.mockRejectedValueOnce(
-        new ConflictError(
-          `Location ${locationSpec.type}:${locationSpec.target} already exists`,
-        ),
-      );
-
-      await expect(
-        locationService.createLocation(locationSpec, false),
-      ).rejects.toThrow(ConflictError);
-    });
-
     it('should return default InputError for failed processed entities in dryRun mode', async () => {
       store.listLocations.mockResolvedValueOnce([]);
 

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

@@ -24,21 +24,15 @@ import {
 } from '@backstage/catalog-model';
 import { Location } from '@backstage/catalog-client';
 import { CatalogProcessingOrchestrator } from '../processing/types';
-import {
-  LocationInput,
-  LocationService,
-  LocationStore,
-  RefreshService,
-} from './types';
+import { LocationInput, LocationService, LocationStore } from './types';
 import { locationSpecToMetadataName } from '../util/conversion';
-import { ConflictError, InputError } from '@backstage/errors';
+import { InputError } from '@backstage/errors';
 import { DeferredEntity } from '@backstage/plugin-catalog-node';
 import { FilterPredicate } from '@backstage/filter-predicates';
 import { BackstageCredentials } from '@backstage/backend-plugin-api';
 
 export type DefaultLocationServiceOptions = {
   allowedLocationTypes: string[];
-  refreshService?: RefreshService;
 };
 
 export class DefaultLocationService implements LocationService {
@@ -76,48 +70,10 @@ export class DefaultLocationService implements LocationService {
     if (dryRun) {
       return this.dryRunCreateLocation(input);
     }
-    try {
-      const location = await this.store.createLocation(input);
-      return { location, entities: [] };
-    } catch (error) {
-      if (options?.onConflict === 'refresh' && error instanceof ConflictError) {
-        return this.refreshExistingLocation(input, options);
-      }
-      throw error;
-    }
-  }
-
-  private async refreshExistingLocation(
-    input: LocationInput,
-    options: { credentials?: BackstageCredentials },
-  ): Promise<{ location: Location; entities: Entity[] }> {
-    const { refreshService } = this.options;
-    if (!refreshService) {
-      throw new InputError(
-        'onConflict refresh is not supported: no refresh service available',
-      );
-    }
-    if (!options.credentials) {
-      throw new InputError(
-        'onConflict refresh requires credentials to be provided',
-      );
-    }
-
-    const location = await this.store.getLocationByEntity(
-      `location:default/${locationSpecToMetadataName(input)}`,
-    );
-
-    const entityRef = stringifyEntityRef({
-      kind: 'Location',
-      namespace: 'default',
-      name: locationSpecToMetadataName(input),
+    const location = await this.store.createLocation(input, {
+      onConflict: options?.onConflict,
+      credentials: options?.credentials,
     });
-
-    await refreshService.refresh({
-      entityRef,
-      credentials: options.credentials,
-    });
-
     return { location, entities: [] };
   }
 

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

@@ -85,7 +85,13 @@ export interface RefreshService {
  * Interacts with the database to manage locations.
  */
 export interface LocationStore {
-  createLocation(location: LocationInput): Promise<Location>;
+  createLocation(
+    location: LocationInput,
+    options?: {
+      onConflict?: 'refresh' | 'reject';
+      credentials?: BackstageCredentials;
+    },
+  ): Promise<Location>;
   listLocations(): Promise<Location[]>;
   queryLocations(options: {
     limit: number;