diff --git a/.changeset/thick-kings-destroy.md b/.changeset/thick-kings-destroy.md new file mode 100644 index 0000000000..77c6be0d3c --- /dev/null +++ b/.changeset/thick-kings-destroy.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Make sure to stitch entities correctly after deletion, to ensure that their relations are updated. diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index 25f9fde313..d47ace1454 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -409,7 +409,11 @@ export class CatalogBuilder { parser, policy, }); - const unauthorizedEntitiesCatalog = new DefaultEntitiesCatalog(dbClient); + const stitcher = new Stitcher(dbClient, logger); + const unauthorizedEntitiesCatalog = new DefaultEntitiesCatalog( + dbClient, + stitcher, + ); let permissionEvaluator: PermissionEvaluator; if ('authorizeConditional' in permissions) { @@ -453,7 +457,6 @@ export class CatalogBuilder { permissions: catalogPermissions, rules: this.permissionRules, }); - const stitcher = new Stitcher(dbClient, logger); const locationStore = new DefaultLocationStore(dbClient); const configLocationProvider = new ConfigLocationEntityProvider(config); diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts index 43e208197f..49d4fe9e19 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts @@ -25,12 +25,15 @@ import { DbRefreshStateRow, DbSearchRow, } from '../database/tables'; +import { Stitcher } from '../stitching/Stitcher'; import { DefaultEntitiesCatalog } from './DefaultEntitiesCatalog'; describe('DefaultEntitiesCatalog', () => { const databases = TestDatabases.create({ ids: ['POSTGRES_13', 'POSTGRES_9', 'SQLITE_3'], }); + const stitch = jest.fn(); + const stitcher: Stitcher = { stitch } as any; async function createDatabase(databaseId: TestDatabaseId) { const knex = await databases.init(databaseId); @@ -122,6 +125,10 @@ describe('DefaultEntitiesCatalog', () => { ); } + afterEach(() => { + jest.resetAllMocks(); + }); + describe('entityAncestry', () => { it.each(databases.eachSupportedId())( 'should return the ancestry with one parent, %p', @@ -151,7 +158,7 @@ describe('DefaultEntitiesCatalog', () => { await addEntity(knex, parent, [{ entity: grandparent }]); await addEntity(knex, root, [{ entity: parent }]); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); const result = await catalog.entityAncestry('k:default/root'); expect(result.rootEntityRef).toEqual('k:default/root'); @@ -181,7 +188,7 @@ describe('DefaultEntitiesCatalog', () => { 'should throw error if the entity does not exist, %p', async databaseId => { const { knex } = await createDatabase(databaseId); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); await expect(() => catalog.entityAncestry('k:default/root'), ).rejects.toThrow('No such entity k:default/root'); @@ -224,7 +231,7 @@ describe('DefaultEntitiesCatalog', () => { await addEntity(knex, parent2, [{ entity: grandparent }]); await addEntity(knex, root, [{ entity: parent1 }, { entity: parent2 }]); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); const result = await catalog.entityAncestry('k:default/root'); expect(result.rootEntityRef).toEqual('k:default/root'); @@ -280,7 +287,7 @@ describe('DefaultEntitiesCatalog', () => { }; await addEntityToSearch(knex, entity1); await addEntityToSearch(knex, entity2); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); const testFilter = { key: 'spec.test', @@ -313,7 +320,7 @@ describe('DefaultEntitiesCatalog', () => { }; await addEntityToSearch(knex, entity1); await addEntityToSearch(knex, entity2); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); const testFilter = { not: { @@ -360,7 +367,7 @@ describe('DefaultEntitiesCatalog', () => { await addEntityToSearch(knex, entity2); await addEntityToSearch(knex, entity3); await addEntityToSearch(knex, entity4); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); const testFilter1 = { key: 'metadata.org', @@ -415,7 +422,7 @@ describe('DefaultEntitiesCatalog', () => { }; await addEntityToSearch(knex, entity1); await addEntityToSearch(knex, entity2); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); const testFilter1 = { key: 'metadata.org', @@ -457,7 +464,7 @@ describe('DefaultEntitiesCatalog', () => { }; await addEntityToSearch(knex, entity1); await addEntityToSearch(knex, entity2); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); const testFilter = { key: 'kind', @@ -501,7 +508,7 @@ describe('DefaultEntitiesCatalog', () => { }, [], ); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); const { entities } = await catalog.entities(); @@ -557,10 +564,16 @@ describe('DefaultEntitiesCatalog', () => { metadata: { name: 'root' }, spec: {}, }; - const unrelated: Entity = { + const unrelated1: Entity = { apiVersion: 'a', kind: 'k', - metadata: { name: 'unrelated' }, + metadata: { name: 'unrelated1' }, + spec: {}, + }; + const unrelated2: Entity = { + apiVersion: 'a', + kind: 'k', + metadata: { name: 'unrelated2' }, spec: {}, }; @@ -571,10 +584,23 @@ describe('DefaultEntitiesCatalog', () => { { entity: parent1 }, { entity: parent2 }, ]); - await addEntity(knex, unrelated, []); + await addEntity(knex, unrelated1, []); + await addEntity(knex, unrelated2, []); await knex('refresh_state').update({ result_hash: 'not-changed' }); + await knex('relations').insert({ + originating_entity_id: uid, + type: 't', + source_entity_ref: 'k:default/root', + target_entity_ref: 'k:default/unrelated1', + }); + await knex('relations').insert({ + originating_entity_id: uid, + type: 't', + source_entity_ref: 'k:default/unrelated2', + target_entity_ref: 'k:default/root', + }); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); await catalog.removeEntityByUid(uid); await expect( @@ -586,8 +612,12 @@ describe('DefaultEntitiesCatalog', () => { { entity_ref: 'k:default/grandparent', result_hash: 'not-changed' }, { entity_ref: 'k:default/parent1', result_hash: 'child-was-deleted' }, { entity_ref: 'k:default/parent2', result_hash: 'child-was-deleted' }, - { entity_ref: 'k:default/unrelated', result_hash: 'not-changed' }, + { entity_ref: 'k:default/unrelated1', result_hash: 'not-changed' }, + { entity_ref: 'k:default/unrelated2', result_hash: 'not-changed' }, ]); + expect(stitch).toHaveBeenCalledWith( + new Set(['k:default/unrelated1', 'k:default/unrelated2']), + ); }, ); }); @@ -616,7 +646,7 @@ describe('DefaultEntitiesCatalog', () => { metadata: { name: 'two' }, spec: {}, }); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); await expect(catalog.facets({ facets: ['kind'] })).resolves.toEqual({ facets: { @@ -654,7 +684,7 @@ describe('DefaultEntitiesCatalog', () => { }, spec: {}, }); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); await expect( catalog.facets({ @@ -698,7 +728,7 @@ describe('DefaultEntitiesCatalog', () => { }, spec: {}, }); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog(knex, stitcher); await expect( catalog.facets({ diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts index fa11903447..dc84b12484 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts @@ -38,8 +38,10 @@ import { DbPageInfo, DbRefreshStateReferencesRow, DbRefreshStateRow, + DbRelationsRow, DbSearchRow, } from '../database/tables'; +import { Stitcher } from '../stitching/Stitcher'; function parsePagination(input?: EntityPagination): { limit?: number; @@ -158,7 +160,10 @@ function parseFilter( } export class DefaultEntitiesCatalog implements EntitiesCatalog { - constructor(private readonly database: Knex) {} + constructor( + private readonly database: Knex, + private readonly stitcher: Stitcher, + ) {} async entities(request?: EntitiesRequest): Promise { const db = this.database; @@ -244,6 +249,7 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { await this.database('refresh_state') .update({ result_hash: 'child-was-deleted', + next_update_at: this.database.fn.now(), }) .whereIn('entity_ref', function parents(builder) { return builder @@ -256,9 +262,35 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { .select('refresh_state_references.source_entity_ref'); }); + // Stitch the entities that the deleted one had relations to. If we do not + // do this, the entities in the other end of the relations will still look + // like they have a relation to the entity that was deleted, despite not + // having any corresponding rows in the relations table. + const relationPeers = await this.database + .from('relations') + .innerJoin('refresh_state', { + 'refresh_state.entity_ref': 'relations.target_entity_ref', + }) + .where('relations.originating_entity_id', '=', uid) + .andWhere('refresh_state.entity_id', '!=', uid) + .select({ ref: 'relations.target_entity_ref' }) + .union(other => + other + .from('relations') + .innerJoin('refresh_state', { + 'refresh_state.entity_ref': 'relations.source_entity_ref', + }) + .where('relations.originating_entity_id', '=', uid) + .andWhere('refresh_state.entity_id', '!=', uid) + .select({ ref: 'relations.source_entity_ref' }), + ); + + // Perform the actual deletion await this.database('refresh_state') .where('entity_id', uid) .delete(); + + await this.stitcher.stitch(new Set(relationPeers.map(p => p.ref))); } async entityAncestry(rootRef: string): Promise {