Use stable location_entity_ref as locationKey; validate allowedLocationTypes
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 <freben@spotify.com> Made-with: Cursor
This commit is contained in:
@@ -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: [],
|
||||
|
||||
@@ -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: [],
|
||||
});
|
||||
|
||||
@@ -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<CatalogProcessingOrchestrator> = {
|
||||
@@ -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({
|
||||
|
||||
@@ -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<Location> {
|
||||
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);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user