diff --git a/.changeset/eight-poems-take.md b/.changeset/eight-poems-take.md new file mode 100644 index 0000000000..00f1d09466 --- /dev/null +++ b/.changeset/eight-poems-take.md @@ -0,0 +1,6 @@ +--- +'@backstage/catalog-client': minor +'@backstage/plugin-catalog-backend': minor +--- + +Add an `onConflict` option to location creation that can refresh an existing location instead of throwing a conflict error. diff --git a/packages/catalog-client/report.api.md b/packages/catalog-client/report.api.md index c155084ccd..73eb280d87 100644 --- a/packages/catalog-client/report.api.md +++ b/packages/catalog-client/report.api.md @@ -15,6 +15,7 @@ export type AddLocationRequest = { type?: string; target: string; dryRun?: boolean; + onConflict?: 'refresh' | 'reject'; }; // @public diff --git a/packages/catalog-client/src/CatalogClient.ts b/packages/catalog-client/src/CatalogClient.ts index a671a155d3..cf363e1823 100644 --- a/packages/catalog-client/src/CatalogClient.ts +++ b/packages/catalog-client/src/CatalogClient.ts @@ -567,12 +567,15 @@ export class CatalogClient implements CatalogApi { request: AddLocationRequest, options?: CatalogRequestOptions, ): Promise { - const { type = 'url', target, dryRun } = request; + const { type = 'url', target, dryRun, onConflict } = request; const response = await this.apiClient.createLocation( { body: { type, target }, - query: { dryRun: dryRun ? 'true' : undefined }, + query: { + dryRun: dryRun ? 'true' : undefined, + onConflict, + }, }, options, ); diff --git a/packages/catalog-client/src/schema/openapi/generated/apis/Api.client.ts b/packages/catalog-client/src/schema/openapi/generated/apis/Api.client.ts index 63d55aea55..4b3e088fe2 100644 --- a/packages/catalog-client/src/schema/openapi/generated/apis/Api.client.ts +++ b/packages/catalog-client/src/schema/openapi/generated/apis/Api.client.ts @@ -178,6 +178,7 @@ export type CreateLocation = { body: CreateLocationRequest; query: { dryRun?: string; + onConflict?: 'refresh' | 'reject'; }; }; /** @@ -592,6 +593,7 @@ export class DefaultApiClient { * Create a location for a given target. * @param createLocationRequest - * @param dryRun - + * @param onConflict - Behavior when the location already exists. \'reject\' (default) returns a 409 error, \'refresh\' triggers a refresh of the existing location entity and returns 201. */ public async createLocation( // @ts-ignore @@ -600,7 +602,7 @@ export class DefaultApiClient { ): Promise> { const baseUrl = await this.discoveryApi.getBaseUrl(pluginId); - const uriTemplate = `/locations{?dryRun}`; + const uriTemplate = `/locations{?dryRun,onConflict}`; const uri = parser.parse(uriTemplate).expand({ ...request.query, diff --git a/packages/catalog-client/src/types/api.ts b/packages/catalog-client/src/types/api.ts index fac4ed5928..d6d5392fcf 100644 --- a/packages/catalog-client/src/types/api.ts +++ b/packages/catalog-client/src/types/api.ts @@ -396,6 +396,12 @@ export type AddLocationRequest = { * contain the entities that match the given location. */ dryRun?: boolean; + /** + * Behavior when the location already exists. If set to `'reject'` (the + * default), a conflict error is returned. If set to `'refresh'`, the + * existing location entity is marked for refresh and a 201 is returned. + */ + onConflict?: 'refresh' | 'reject'; }; /** diff --git a/plugins/catalog-backend/config.d.ts b/plugins/catalog-backend/config.d.ts index 48dae2d747..433d8c494c 100644 --- a/plugins/catalog-backend/config.d.ts +++ b/plugins/catalog-backend/config.d.ts @@ -191,6 +191,15 @@ export interface Config { stitchTimeout?: HumanDuration | string; }; + /** + * The strategy to use when there is a conflict with a location being registered. + * + * The default value is "reject". + * + * The "refresh" strategy will refresh the existing location instead of throwing a conflict error. + */ + defaultLocationConflictStrategy?: 'refresh' | 'reject'; + /** * The interval at which the catalog should process its entities. * @remarks diff --git a/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts b/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts index e1bd8ec386..d1cd6e7c5e 100644 --- a/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts +++ b/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts @@ -15,7 +15,10 @@ */ import { TestDatabaseId, TestDatabases } from '@backstage/backend-test-utils'; -import { ANNOTATION_ORIGIN_LOCATION } from '@backstage/catalog-model'; +import { + ANNOTATION_ORIGIN_LOCATION, + stringifyEntityRef, +} from '@backstage/catalog-model'; import { v4 as uuid } from 'uuid'; import { applyDatabaseMigrations } from '../database/migrations'; import { @@ -25,6 +28,7 @@ import { DbSearchRow, } from '../database/tables'; import { DefaultLocationStore } from './DefaultLocationStore'; +import { locationSpecToLocationEntity } from '../util/conversion'; import { CatalogScmEventsServiceSubscriber } from '@backstage/plugin-catalog-node/alpha'; import waitFor from 'wait-for-expect'; @@ -154,6 +158,48 @@ describe('DefaultLocationStore', () => { }); }, ); + + it.each(databases.eachSupportedId())( + 'updates refresh_state when onConflict is refresh, %p', + async databaseId => { + const { store, knex } = await createLocationStore(databaseId); + const spec = { + type: 'url', + target: + 'https://github.com/backstage/demo/blob/master/catalog-info.yml', + }; + + // Create the location initially + await store.createLocation(spec); + + // Seed a refresh_state row for the corresponding Location entity + const entity = locationSpecToLocationEntity({ location: spec }); + const entityRef = stringifyEntityRef(entity); + const entityId = uuid(); + const oldDate = new Date('2020-01-01T00:00:00Z'); + await knex('refresh_state').insert({ + entity_id: entityId, + entity_ref: entityRef, + unprocessed_entity: '{}', + errors: '[]', + next_update_at: oldDate, + last_discovery_at: oldDate, + result_hash: 'old-hash', + }); + + // Re-register the same location with onConflict: 'refresh' + await store.createLocation(spec, { onConflict: 'refresh' }); + + // Verify that the refresh_state row was updated + const [row] = await knex('refresh_state').where({ + entity_ref: entityRef, + }); + expect(row.result_hash).toBe(''); + expect(new Date(row.next_update_at).getTime()).toBeGreaterThan( + oldDate.getTime(), + ); + }, + ); }); describe('deleteLocation', () => { diff --git a/plugins/catalog-backend/src/providers/DefaultLocationStore.ts b/plugins/catalog-backend/src/providers/DefaultLocationStore.ts index bfd4270696..8619d13459 100644 --- a/plugins/catalog-backend/src/providers/DefaultLocationStore.ts +++ b/plugins/catalog-backend/src/providers/DefaultLocationStore.ts @@ -68,16 +68,27 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { return 'DefaultLocationStore'; } - async createLocation(input: LocationInput): Promise { + async createLocation( + input: LocationInput, + options?: { + onConflict?: 'refresh' | 'reject'; + }, + ): 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,6 +104,9 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { return inner; }); + + // Always upsert the entity, even if the location already existed, to + // recover from cases where the entity was inadvertently deleted. const entity = locationSpecToLocationEntity({ location }); await this.connection.applyMutation({ type: 'delta', @@ -100,6 +114,18 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { removed: [], }); + if (existed) { + // This is the "onConflict refresh" case, where a re-registration safely + // tries to recover from a bad state. + const entityRef = stringifyEntityRef(entity); + await this.db('refresh_state') + .where({ entity_ref: entityRef }) + .update({ + next_update_at: this.db.fn.now(), + result_hash: '', + }); + } + return location; } diff --git a/plugins/catalog-backend/src/schema/openapi.yaml b/plugins/catalog-backend/src/schema/openapi.yaml index 57d950b9f8..d0266e4d46 100644 --- a/plugins/catalog-backend/src/schema/openapi.yaml +++ b/plugins/catalog-backend/src/schema/openapi.yaml @@ -25,6 +25,9 @@ info: name: Apache-2.0 url: http://www.apache.org/licenses/LICENSE-2.0.html contact: {} +tags: + - name: Entity + - name: Locations servers: - url: / components: @@ -1267,6 +1270,19 @@ paths: allowReserved: true schema: type: string + - in: query + name: onConflict + required: false + allowReserved: true + schema: + type: string + enum: + - refresh + - reject + description: >- + Behavior when the location already exists. 'reject' (default) + returns a 409 error, 'refresh' triggers a refresh of the + existing location entity and returns 201. requestBody: required: true content: diff --git a/plugins/catalog-backend/src/schema/openapi/generated/apis/Api.server.ts b/plugins/catalog-backend/src/schema/openapi/generated/apis/Api.server.ts index cbde6365ea..81a952a166 100644 --- a/plugins/catalog-backend/src/schema/openapi/generated/apis/Api.server.ts +++ b/plugins/catalog-backend/src/schema/openapi/generated/apis/Api.server.ts @@ -172,6 +172,7 @@ export type CreateLocation = { body: CreateLocationRequest; query: { dryRun?: string; + onConflict?: 'refresh' | 'reject'; }; response: CreateLocation201Response | Error | Error; }; diff --git a/plugins/catalog-backend/src/schema/openapi/generated/router.ts b/plugins/catalog-backend/src/schema/openapi/generated/router.ts index 56ee4cc036..6b92c7ae63 100644 --- a/plugins/catalog-backend/src/schema/openapi/generated/router.ts +++ b/plugins/catalog-backend/src/schema/openapi/generated/router.ts @@ -33,6 +33,14 @@ export const spec = { }, contact: {}, }, + tags: [ + { + name: 'Entity', + }, + { + name: 'Locations', + }, + ], servers: [ { url: '/', @@ -1490,6 +1498,18 @@ export const spec = { type: 'string', }, }, + { + in: 'query', + name: 'onConflict', + required: false, + allowReserved: true, + schema: { + type: 'string', + enum: ['refresh', 'reject'], + }, + description: + "Behavior when the location already exists. 'reject' (default) returns a 409 error, 'refresh' triggers a refresh of the existing location entity and returns 201.", + }, ], requestBody: { required: true, diff --git a/plugins/catalog-backend/src/service/AuthorizedLocationService.ts b/plugins/catalog-backend/src/service/AuthorizedLocationService.ts index 83f5118153..1b18d50324 100644 --- a/plugins/catalog-backend/src/service/AuthorizedLocationService.ts +++ b/plugins/catalog-backend/src/service/AuthorizedLocationService.ts @@ -46,6 +46,7 @@ export class AuthorizedLocationService implements LocationService { spec: LocationInput, dryRun: boolean, options: { + onConflict?: 'refresh' | 'reject'; credentials: BackstageCredentials; }, ): Promise<{ diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index d0f2442f0c..d3bf47874d 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -594,6 +594,10 @@ export class CatalogBuilder { const locationService = new AuthorizedLocationService( new DefaultLocationService(locationStore, orchestrator, { allowedLocationTypes: this.allowedLocationType, + defaultLocationConflictStrategy: + (config.getOptionalString( + 'catalog.defaultLocationConflictStrategy', + ) as 'refresh' | 'reject') || 'reject', }), permissionsService, ); diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts index 822706ada4..3cce0efc3f 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts @@ -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', + }, + expect.anything(), + ); }); it('should create location with unknown type if configuration allows it', async () => { @@ -279,6 +282,7 @@ describe('DefaultLocationServiceTest', () => { orchestrator, { allowedLocationTypes: ['url', 'unknown'], + defaultLocationConflictStrategy: 'reject', }, ); await expect( @@ -291,10 +295,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', + }, + expect.anything(), + ); }); it('should not allow locations of unknown types by default', async () => { @@ -309,6 +316,31 @@ describe('DefaultLocationServiceTest', () => { ).rejects.toThrow(InputError); }); + it('should pass onConflict through to store', async () => { + const locationSpec = { + type: 'url', + target: 'https://backstage.io/catalog-info.yaml', + }; + + store.createLocation.mockResolvedValueOnce({ + id: 'existing-id', + ...locationSpec, + }); + + const result = await locationService.createLocation(locationSpec, false, { + onConflict: 'refresh', + credentials: {} as any, + }); + + expect(result).toEqual({ + location: { id: 'existing-id', ...locationSpec }, + entities: [], + }); + expect(store.createLocation).toHaveBeenCalledWith(locationSpec, { + onConflict: 'refresh', + }); + }); + 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 ad3044ffd9..7effaa2e88 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.ts @@ -33,6 +33,7 @@ import { BackstageCredentials } from '@backstage/backend-plugin-api'; export type DefaultLocationServiceOptions = { allowedLocationTypes: string[]; + defaultLocationConflictStrategy: 'refresh' | 'reject'; }; export class DefaultLocationService implements LocationService { @@ -45,6 +46,7 @@ export class DefaultLocationService implements LocationService { orchestrator: CatalogProcessingOrchestrator, options: DefaultLocationServiceOptions = { allowedLocationTypes: ['url'], + defaultLocationConflictStrategy: 'reject', }, ) { this.store = store; @@ -55,6 +57,10 @@ export class DefaultLocationService implements LocationService { async createLocation( input: LocationInput, dryRun: boolean, + options?: { + onConflict?: 'refresh' | 'reject'; + credentials?: BackstageCredentials; + }, ): Promise<{ location: Location; entities: Entity[]; exists?: boolean }> { if (!this.options.allowedLocationTypes.includes(input.type)) { throw new InputError( @@ -66,7 +72,10 @@ export class DefaultLocationService implements LocationService { if (dryRun) { return this.dryRunCreateLocation(input); } - const location = await this.store.createLocation(input); + const location = await this.store.createLocation(input, { + onConflict: + options?.onConflict ?? this.options.defaultLocationConflictStrategy, + }); return { location, entities: [] }; } diff --git a/plugins/catalog-backend/src/service/createRouter.test.ts b/plugins/catalog-backend/src/service/createRouter.test.ts index 1984f2f04f..88cd5abde3 100644 --- a/plugins/catalog-backend/src/service/createRouter.test.ts +++ b/plugins/catalog-backend/src/service/createRouter.test.ts @@ -1685,7 +1685,10 @@ describe('POST /locations/by-query works end to end', () => { const locationService = new DefaultLocationService( store, { process: jest.fn() }, - { allowedLocationTypes: ['url'] }, + { + allowedLocationTypes: ['url'], + defaultLocationConflictStrategy: 'reject', + }, ); const router = await createRouter({ diff --git a/plugins/catalog-backend/src/service/createRouter.ts b/plugins/catalog-backend/src/service/createRouter.ts index 1a7f1bf6fc..89377c9e12 100644 --- a/plugins/catalog-backend/src/service/createRouter.ts +++ b/plugins/catalog-backend/src/service/createRouter.ts @@ -606,6 +606,7 @@ export async function createRouter( .post('/locations', async (req, res) => { const location = await validateRequestBody(req, locationInput); const dryRun = yn(req.query.dryRun, { default: false }); + const onConflict = req.query.onConflict; const auditorEvent = await auditor.createEvent({ eventId: 'location-mutate', @@ -629,6 +630,7 @@ export async function createRouter( location, dryRun, { + onConflict, credentials: await httpAuth.credentials(req), }, ); diff --git a/plugins/catalog-backend/src/service/types.ts b/plugins/catalog-backend/src/service/types.ts index f452484b72..c949da1eda 100644 --- a/plugins/catalog-backend/src/service/types.ts +++ b/plugins/catalog-backend/src/service/types.ts @@ -35,6 +35,7 @@ export interface LocationService { location: LocationInput, dryRun: boolean, options: { + onConflict?: 'refresh' | 'reject'; credentials: BackstageCredentials; }, ): Promise<{ location: Location; entities: Entity[]; exists?: boolean }>; @@ -84,7 +85,12 @@ export interface RefreshService { * Interacts with the database to manage locations. */ export interface LocationStore { - createLocation(location: LocationInput): Promise; + createLocation( + location: LocationInput, + options?: { + onConflict: 'refresh' | 'reject'; + }, + ): Promise; listLocations(): Promise; queryLocations(options: { limit: number;