From 068f4739acfa3708ce3ca93e34d0724aa39af5f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Fri, 10 Apr 2026 13:44:14 +0200 Subject: [PATCH] Use stable location_entity_ref as locationKey; validate allowedLocationTypes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use location_entity_ref as the locationKey in all applyMutation calls instead of type:target. This prevents the catalog from treating a type/target update as a remove+re-add cycle (which would temporarily prune the entity and its descendants). The location_entity_ref never changes after creation, so the catalog can safely upsert the entity in-place on updateLocation. Also add allowedLocationTypes validation to updateLocation in DefaultLocationService, matching the existing check in createLocation. Signed-off-by: Fredrik Adelöw Made-with: Cursor --- .../providers/DefaultLocationStore.test.ts | 42 ++++++++++++------- .../src/providers/DefaultLocationStore.ts | 11 +++-- .../service/DefaultLocationService.test.ts | 36 ++++++++++++++++ .../src/service/DefaultLocationService.ts | 9 +++- 4 files changed, 76 insertions(+), 22 deletions(-) diff --git a/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts b/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts index b11687e33f..2f68723f37 100644 --- a/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts +++ b/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts @@ -154,8 +154,10 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: - 'url:https://github.com/backstage/demo/blob/master/catalog-info.yml', + locationKey: computeLocationEntityRef( + 'url', + 'https://github.com/backstage/demo/blob/master/catalog-info.yml', + ), }, ]), }); @@ -263,8 +265,10 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: - 'url:https://github.com/backstage/demo/blob/master/catalog-info.yml', + locationKey: computeLocationEntityRef( + 'url', + 'https://github.com/backstage/demo/blob/master/catalog-info.yml', + ), }, ], }); @@ -341,7 +345,9 @@ describe('DefaultLocationStore', () => { entity: expect.objectContaining({ spec: { type: 'url', target: 'https://example.com/new' }, }), - locationKey: 'url:https://example.com/new', + // locationKey is the stable entityRef from when the location was + // originally created, not derived from the new type:target + locationKey: created.entityRef, }, ], }); @@ -467,7 +473,7 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: `url:${matchTarget}`, + locationKey: computeLocationEntityRef('url', matchTarget), }, ], removed: [], @@ -482,7 +488,7 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: `url:${otherTarget}`, + locationKey: computeLocationEntityRef('url', otherTarget), }, ], removed: [], @@ -584,7 +590,7 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: `url:${matchTarget}`, + locationKey: computeLocationEntityRef('url', matchTarget), }, ], removed: [], @@ -599,7 +605,7 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: `url:${otherTarget}`, + locationKey: computeLocationEntityRef('url', otherTarget), }, ], removed: [], @@ -651,7 +657,10 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: `url:https://github.com/backstage/freben/blob/master/catalog-info.yaml`, + locationKey: computeLocationEntityRef( + 'url', + 'https://github.com/backstage/freben/blob/master/catalog-info.yaml', + ), }, ], removed: [], @@ -723,7 +732,7 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: `url:${matchTarget}`, + locationKey: computeLocationEntityRef('url', matchTarget), }, ], removed: [], @@ -738,7 +747,7 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: `url:${otherTarget}`, + locationKey: computeLocationEntityRef('url', otherTarget), }, ], removed: [], @@ -840,7 +849,7 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: `url:${matchTarget}`, + locationKey: computeLocationEntityRef('url', matchTarget), }, ], removed: [], @@ -855,7 +864,7 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: `url:${otherTarget}`, + locationKey: computeLocationEntityRef('url', otherTarget), }, ], removed: [], @@ -906,7 +915,10 @@ describe('DefaultLocationStore', () => { type: 'url', }, }), - locationKey: `url:https://github.com/freben/demo-renamed/blob/master/folder/catalog-info.yaml`, + locationKey: computeLocationEntityRef( + 'url', + 'https://github.com/freben/demo-renamed/blob/master/folder/catalog-info.yaml', + ), }, ], removed: [], diff --git a/plugins/catalog-backend/src/providers/DefaultLocationStore.ts b/plugins/catalog-backend/src/providers/DefaultLocationStore.ts index 519835439f..9e8d0341d1 100644 --- a/plugins/catalog-backend/src/providers/DefaultLocationStore.ts +++ b/plugins/catalog-backend/src/providers/DefaultLocationStore.ts @@ -23,7 +23,6 @@ import { DbRefreshStateRow, DbSearchRow, } from '../database/tables'; -import { getEntityLocationRef } from '../processing/util'; import { EntityProvider, EntityProviderConnection, @@ -117,7 +116,7 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { }); await this.connection.applyMutation({ type: 'delta', - added: [{ entity, locationKey: getEntityLocationRef(entity) }], + added: [{ entity, locationKey: location.location_entity_ref }], removed: [], }); @@ -267,7 +266,7 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { await this.connection.applyMutation({ type: 'delta', - added: [{ entity, locationKey: getEntityLocationRef(entity) }], + added: [{ entity, locationKey: row.location_entity_ref }], removed: [], }); @@ -303,7 +302,7 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { await this.connection.applyMutation({ type: 'delta', added: [], - removed: [{ entity, locationKey: getEntityLocationRef(entity) }], + removed: [{ entity, locationKey: deleted.location_entity_ref }], }); } @@ -369,7 +368,7 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { location, locationEntityRef: location.location_entity_ref, }); - return { entity, locationKey: getEntityLocationRef(entity) }; + return { entity, locationKey: location.location_entity_ref }; }); await this.connection.applyMutation({ @@ -500,7 +499,7 @@ export class DefaultLocationStore implements LocationStore, EntityProvider { location, locationEntityRef: location.location_entity_ref, }); - return { entity, locationKey: getEntityLocationRef(entity) }; + return { entity, locationKey: location.location_entity_ref }; }), removed: [], }); diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts index 412fe77341..3df46ceae5 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts @@ -18,6 +18,7 @@ import { DefaultLocationService } from './DefaultLocationService'; import { CatalogProcessingOrchestrator } from '../processing/types'; import { LocationStore } from './types'; import { InputError } from '@backstage/errors'; +import { mockCredentials } from '@backstage/backend-test-utils'; describe('DefaultLocationServiceTest', () => { const orchestrator: jest.Mocked = { @@ -417,6 +418,41 @@ describe('DefaultLocationServiceTest', () => { }); }); + describe('updateLocation', () => { + it('should not allow locations of disallowed types', async () => { + await expect( + locationService.updateLocation( + 'some-id', + { type: 'unknown', target: 'https://backstage.io/catalog-info.yaml' }, + { credentials: mockCredentials.none() }, + ), + ).rejects.toThrow(InputError); + }); + + it('should delegate to store for allowed types', async () => { + const updated = { + id: 'some-id', + type: 'url', + target: 'https://backstage.io/catalog-info.yaml', + entityRef: 'location:default/generated-abc', + }; + store.updateLocation.mockResolvedValue(updated); + + await expect( + locationService.updateLocation( + 'some-id', + { type: 'url', target: 'https://backstage.io/catalog-info.yaml' }, + { credentials: mockCredentials.none() }, + ), + ).resolves.toEqual(updated); + + expect(store.updateLocation).toHaveBeenCalledWith('some-id', { + type: 'url', + target: 'https://backstage.io/catalog-info.yaml', + }); + }); + }); + describe('getLocationByEntity', () => { it('should call locationStore.getLocationByEntity', async () => { await locationService.getLocationByEntity({ diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.ts b/plugins/catalog-backend/src/service/DefaultLocationService.ts index 7dc98dabc4..103b5ff5a0 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.ts @@ -103,11 +103,18 @@ export class DefaultLocationService implements LocationService { return this.store.getLocation(id); } - updateLocation( + async updateLocation( id: string, location: LocationInput, _options: { credentials: BackstageCredentials }, ): Promise { + if (!this.options.allowedLocationTypes.includes(location.type)) { + throw new InputError( + `Registered locations must be of an allowed type ${JSON.stringify( + this.options.allowedLocationTypes, + )}`, + ); + } return this.store.updateLocation(id, location); }