diff --git a/plugins/catalog-backend/src/providers/DefaultLocationStore.ts b/plugins/catalog-backend/src/providers/DefaultLocationStore.ts index bfd4270696..233f5f5273 100644 --- a/plugins/catalog-backend/src/providers/DefaultLocationStore.ts +++ b/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 { + async createLocation( + input: LocationInput, + options?: { + onConflict?: 'refresh' | 'reject'; + credentials?: BackstageCredentials; + }, + ): Promise { + 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; } diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index e2fdaf35c2..5cc06b5e75 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/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, ); diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts index 222689e109..2071836c60 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts +++ b/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 = { @@ -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 = { - 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([]); diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.ts b/plugins/catalog-backend/src/service/DefaultLocationService.ts index 4057068ff7..99c2ffa16d 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.ts +++ b/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: [] }; } diff --git a/plugins/catalog-backend/src/service/types.ts b/plugins/catalog-backend/src/service/types.ts index 787eecb497..33b37c8f3f 100644 --- a/plugins/catalog-backend/src/service/types.ts +++ b/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; + createLocation( + location: LocationInput, + options?: { + onConflict?: 'refresh' | 'reject'; + credentials?: BackstageCredentials; + }, + ): Promise; listLocations(): Promise; queryLocations(options: { limit: number;