Merge pull request #33221 from backstage/freben/add-location-on-conflict-refresh

Add onConflict option to location registration endpoint

.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.

packages/catalog-client/report.api.md

@@ -15,6 +15,7 @@ export type AddLocationRequest = {
   type?: string;
   target: string;
   dryRun?: boolean;
+  onConflict?: 'refresh' | 'reject';
 };
 
 // @public

packages/catalog-client/src/CatalogClient.ts

@@ -567,12 +567,15 @@ export class CatalogClient implements CatalogApi {
     request: AddLocationRequest,
     options?: CatalogRequestOptions,
   ): Promise<AddLocationResponse> {
-    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,
     );

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<TypedResponse<CreateLocation201Response>> {
     const baseUrl = await this.discoveryApi.getBaseUrl(pluginId);
 
-    const uriTemplate = `/locations{?dryRun}`;
+    const uriTemplate = `/locations{?dryRun,onConflict}`;
 
     const uri = parser.parse(uriTemplate).expand({
       ...request.query,

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';
 };
 
 /**

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

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<DbRefreshStateRow>('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<DbRefreshStateRow>('refresh_state').where({
+          entity_ref: entityRef,
+        });
+        expect(row.result_hash).toBe('');
+        expect(new Date(row.next_update_at).getTime()).toBeGreaterThan(
+          oldDate.getTime(),
+        );
+      },
+    );
   });
 
   describe('deleteLocation', () => {

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

@@ -68,16 +68,27 @@ export class DefaultLocationStore implements LocationStore, EntityProvider {
     return 'DefaultLocationStore';
   }
 
-  async createLocation(input: LocationInput): Promise<Location> {
+  async createLocation(
+    input: LocationInput,
+    options?: {
+      onConflict?: 'refresh' | 'reject';
+    },
+  ): 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,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<DbRefreshStateRow>('refresh_state')
+        .where({ entity_ref: entityRef })
+        .update({
+          next_update_at: this.db.fn.now(),
+          result_hash: '',
+        });
+    }
+
     return location;
   }
 

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:

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;
 };

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,

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<{

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,
     );

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([]);
 

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: [] };
   }
 

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({

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),
             },
           );

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<Location>;
+  createLocation(
+    location: LocationInput,
+    options?: {
+      onConflict: 'refresh' | 'reject';
+    },
+  ): Promise<Location>;
   listLocations(): Promise<Location[]>;
   queryLocations(options: {
     limit: number;