From 348e8c1cdbb9ed17f523d6f33bdc4207115fead5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 21 Sep 2023 15:36:33 +0200 Subject: [PATCH] ensure stitching happens after eager deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/lovely-coins-occur.md | 5 + .../deleteWithEagerPruningOfChildren.test.ts | 53 ++- .../deleteWithEagerPruningOfChildren.ts | 365 +++++++++++------- 3 files changed, 271 insertions(+), 152 deletions(-) create mode 100644 .changeset/lovely-coins-occur.md diff --git a/.changeset/lovely-coins-occur.md b/.changeset/lovely-coins-occur.md new file mode 100644 index 0000000000..34a6774388 --- /dev/null +++ b/.changeset/lovely-coins-occur.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Fixes a bug where eagerly deleted entities did not properly trigger re-stitching of entities that they had relations to. diff --git a/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.test.ts b/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.test.ts index dcd87613de..830cc20480 100644 --- a/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.test.ts +++ b/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.test.ts @@ -18,7 +18,11 @@ import { TestDatabaseId, TestDatabases } from '@backstage/backend-test-utils'; import { Knex } from 'knex'; import * as uuid from 'uuid'; import { applyDatabaseMigrations } from '../../migrations'; -import { DbRefreshStateReferencesRow, DbRefreshStateRow } from '../../tables'; +import { + DbRefreshStateReferencesRow, + DbRefreshStateRow, + DbRelationsRow, +} from '../../tables'; import { deleteWithEagerPruningOfChildren } from './deleteWithEagerPruningOfChildren'; jest.setTimeout(60_000); @@ -43,6 +47,22 @@ describe('deleteWithEagerPruningOfChildren', () => { ); } + async function insertRelation( + knex: Knex, + ...relations: { from: string; to: string }[] + ) { + for (const rel of relations) { + await knex('relations').insert({ + originating_entity_id: await knex('refresh_state') + .select('entity_id') + .then(rows => rows[0].entity_id), // doesn't matter which one, this is consumed pre-deletion + source_entity_ref: rel.from, + target_entity_ref: rel.to, + type: 'fake', + }); + } + } + async function insertEntity(knex: Knex, ...entityRefs: string[]) { for (const ref of entityRefs) { await knex('refresh_state').insert({ @@ -64,6 +84,14 @@ describe('deleteWithEagerPruningOfChildren', () => { return rows.map(r => r.entity_ref); } + async function entitiesMarkedForStitching(knex: Knex) { + const rows = await knex('refresh_state') + .orderBy('entity_ref') + .select('entity_ref') + .where('result_hash', '=', 'force-stitching'); + return rows.map(r => r.entity_ref); + } + it.each(databases.eachSupportedId())( 'works for the simple path, %p', async databaseId => { @@ -78,7 +106,7 @@ describe('deleteWithEagerPruningOfChildren', () => { Scenario: P1 issues delete for E1 and E3 - Result: E1, E2, and E3 deleted; E4 and E5 remain + Result: E1, E2, and E3 deleted; E4 and E5 remain; E4 marked for stitching because it had a relation to a deleted entity */ const knex = await createDatabase(databaseId); await insertEntity(knex, 'E1', 'E2', 'E3', 'E4', 'E5'); @@ -90,12 +118,14 @@ describe('deleteWithEagerPruningOfChildren', () => { { source_key: 'P1', target_entity_ref: 'E4' }, { source_key: 'P2', target_entity_ref: 'E5' }, ); + await insertRelation(knex, { from: 'E4', to: 'E2' }); await deleteWithEagerPruningOfChildren({ knex, sourceKey: 'P1', entityRefs: ['E1', 'E3'], }); await expect(remainingEntities(knex)).resolves.toEqual(['E4', 'E5']); + await expect(entitiesMarkedForStitching(knex)).resolves.toEqual(['E4']); }, ); @@ -113,7 +143,7 @@ describe('deleteWithEagerPruningOfChildren', () => { Scenario: P1 issues delete for E1 - Result: E1 deleted; E2 remains + Result: E1 deleted; E2 remains; E2 marked for stitching because it had a relation to a deleted entity */ const knex = await createDatabase(databaseId); await insertEntity(knex, 'E1', 'E2'); @@ -123,12 +153,14 @@ describe('deleteWithEagerPruningOfChildren', () => { { source_key: 'P1', target_entity_ref: 'E1' }, { source_key: 'P1', target_entity_ref: 'E2' }, ); + await insertRelation(knex, { from: 'E2', to: 'E1' }); await deleteWithEagerPruningOfChildren({ knex, sourceKey: 'P1', entityRefs: ['E1'], }); await expect(remainingEntities(knex)).resolves.toEqual(['E2']); + await expect(entitiesMarkedForStitching(knex)).resolves.toEqual(['E2']); }, ); @@ -144,7 +176,7 @@ describe('deleteWithEagerPruningOfChildren', () => { Scenario: P1 issues delete for E1 - Result: E1 deleted; E2 and E3 remain + Result: E1 deleted; E2 and E3 remain; E2 marked for stitching because it had a relation to a deleted entity */ const knex = await createDatabase(databaseId); await insertEntity(knex, 'E1', 'E2', 'E3'); @@ -155,12 +187,18 @@ describe('deleteWithEagerPruningOfChildren', () => { { source_key: 'P2', target_entity_ref: 'E3' }, { source_entity_ref: 'E3', target_entity_ref: 'E2' }, ); + await insertRelation( + knex, + { from: 'E2', to: 'E1' }, + { from: 'E3', to: 'E2' }, + ); await deleteWithEagerPruningOfChildren({ knex, sourceKey: 'P1', entityRefs: ['E1'], }); await expect(remainingEntities(knex)).resolves.toEqual(['E2', 'E3']); + await expect(entitiesMarkedForStitching(knex)).resolves.toEqual(['E2']); }, ); @@ -193,6 +231,7 @@ describe('deleteWithEagerPruningOfChildren', () => { entityRefs: ['E1'], }); await expect(remainingEntities(knex)).resolves.toEqual(['E2', 'E3']); + await expect(entitiesMarkedForStitching(knex)).resolves.toEqual([]); }, ); @@ -208,7 +247,7 @@ describe('deleteWithEagerPruningOfChildren', () => { Scenario: P1 issues delete for E1, then for E3 - Result: Everything deleted, but in two steps + Result: Everything deleted, but in two steps; E4 marked for stitching in the first step because it had a relation to a deleted entity */ const knex = await createDatabase(databaseId); await insertEntity(knex, 'E1', 'E2', 'E3', 'E4', 'E5', 'E6'); @@ -224,6 +263,7 @@ describe('deleteWithEagerPruningOfChildren', () => { { source_entity_ref: 'E3', target_entity_ref: 'E5' }, { source_entity_ref: 'E5', target_entity_ref: 'E6' }, ); + await insertRelation(knex, { from: 'E4', to: 'E2' }); await deleteWithEagerPruningOfChildren({ knex, sourceKey: 'P1', @@ -235,12 +275,14 @@ describe('deleteWithEagerPruningOfChildren', () => { 'E5', 'E6', ]); + await expect(entitiesMarkedForStitching(knex)).resolves.toEqual(['E4']); await deleteWithEagerPruningOfChildren({ knex, sourceKey: 'P1', entityRefs: ['E3'], }); await expect(remainingEntities(knex)).resolves.toEqual([]); + await expect(entitiesMarkedForStitching(knex)).resolves.toEqual([]); }, ); @@ -277,6 +319,7 @@ describe('deleteWithEagerPruningOfChildren', () => { 'E2', 'E4', ]); + await expect(entitiesMarkedForStitching(knex)).resolves.toEqual([]); }, ); }); diff --git a/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.ts b/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.ts index 027860fcc6..70f7d08ed1 100644 --- a/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.ts +++ b/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.ts @@ -16,7 +16,11 @@ import { Knex } from 'knex'; import lodash from 'lodash'; -import { DbRefreshStateReferencesRow } from '../../tables'; +import { + DbFinalEntitiesRow, + DbRefreshStateReferencesRow, + DbRefreshStateRow, +} from '../../tables'; /** * Given a number of entity refs originally created by a given entity provider @@ -36,152 +40,24 @@ export async function deleteWithEagerPruningOfChildren(options: { // limits for the number of permitted bindings on a precompiled statement let removedCount = 0; for (const refs of lodash.chunk(entityRefs, 1000)) { - removedCount += await knex - .delete() - .from('refresh_state') - .whereIn('entity_ref', orphans => - orphans - // First find all nodes that can be reached downwards from the roots - // (deletion targets), including the roots themselves, by traversing - // down the refresh_state_references table. Note that this query - // starts with a condition that source_key = our source key, and - // target_entity_ref is one of the deletion targets. This has two - // effects: it won't match attempts at deleting something that didn't - // originate from us in the first place, and also won't match non-root - // entities (source_key would be null for those). - // - // KeyA - R1 - R2 Legend: - // \ ----------------------------------------- - // R3 Key* Source key - // / R* Entity ref - // KeyA - R4 - R5 lines Individual references; sources to - // / the left and targets to the right - // KeyB --- R6 - // - // The scenario is that KeyA wants to delete R1. - // - // The query starts with the KeyA-R1 reference, and then traverses - // down to also find R2 and R3. It uses union instead of union all, - // because it wants to find the set of unique descendants even if - // the tree has unexpected loops etc. - .withRecursive('descendants', ['entity_ref'], initial => - initial - .select('target_entity_ref') - .from('refresh_state_references') - .where('source_key', '=', sourceKey) - .whereIn('target_entity_ref', refs) - .union(recursive => - recursive - .select('refresh_state_references.target_entity_ref') - .from('descendants') - .join( - 'refresh_state_references', - 'descendants.entity_ref', - 'refresh_state_references.source_entity_ref', - ), - ), - ) - // Then for each descendant, traverse all the way back upwards through - // the refresh_state_references table to get an exhaustive list of all - // references that are part of keeping that particular descendant - // alive. - // - // Continuing the scenario from above, starting from R3, it goes - // upwards to find every pair along every relation line. - // - // Top branch: R2-R3, R1-R2, KeyA-R1 - // Middle branch: R5-R3, R4-R5, KeyA-R4 - // Bottom branch: R6-R5, KeyB-R6 - // - // Note that this all applied to the subject R3. The exact same thing - // will be done starting from each other descendant (R2 and R1). They - // only have one and two references to find, respectively. - // - // This query also uses union instead of union all, to get the set of - // distinct relations even if the tree has unexpected loops etc. - .withRecursive( - 'ancestors', - ['source_key', 'source_entity_ref', 'target_entity_ref', 'subject'], - initial => - initial - .select( - 'refresh_state_references.source_key', - 'refresh_state_references.source_entity_ref', - 'refresh_state_references.target_entity_ref', - 'descendants.entity_ref', - ) - .from('descendants') - .join( - 'refresh_state_references', - 'refresh_state_references.target_entity_ref', - 'descendants.entity_ref', - ) - .union(recursive => - recursive - .select( - 'refresh_state_references.source_key', - 'refresh_state_references.source_entity_ref', - 'refresh_state_references.target_entity_ref', - 'ancestors.subject', - ) - .from('ancestors') - .join( - 'refresh_state_references', - 'refresh_state_references.target_entity_ref', - 'ancestors.source_entity_ref', - ), - ), - ) - // Finally, from that list of ancestor relations per descendant, pick - // out the ones that are roots (have a source_key). Specifically, find - // ones that seem to be be either (1) from another source, or (2) - // aren't part of the deletion targets. Those are markers that tell us - // that the corresponding descendant should be kept alive and NOT - // subject to eager deletion, because there's "something else" (not - // targeted for deletion) that has references down through the tree to - // it. - // - // Continuing the scenario from above, for R3 we have - // - // KeyA-R1, KeyA-R4, KeyB-R6 - // - // This tells us that R3 should be kept alive for two reasons: it's - // referenced by a node that isn't being deleted (R4), and also by - // another source (KeyB). What about R1 and R2? They both have - // - // KeyA-R1 - // - // So those should be deleted, since they are definitely only being - // kept alive by something that's about to be deleted. - // - // Final shape of the tree: - // - // R3 - // / - // KeyA - R4 - R5 - // / - // KeyB --- R6 - .with('retained', ['entity_ref'], notPartOfDeletion => - notPartOfDeletion - .select('subject') - .from('ancestors') - .whereNotNull('ancestors.source_key') - .where(foreignKeyOrRef => - foreignKeyOrRef - .where('ancestors.source_key', '!=', sourceKey) - .orWhereNotIn('ancestors.target_entity_ref', refs), - ), - ) - // Return all descendants minus the retained ones - .select('descendants.entity_ref') - .from('descendants') - .leftOuterJoin( - 'retained', - 'retained.entity_ref', - 'descendants.entity_ref', - ) - .whereNull('retained.entity_ref'), - ); + const { orphanEntityRefs } = + await findDescendantsThatWouldHaveBeenOrphanedByDeletion({ + knex: options.knex, + refs, + sourceKey, + }); + + // Chunk again - these can be many more than the outer chunk size + for (const refsToDelete of lodash.chunk(orphanEntityRefs, 1000)) { + await markEntitiesAffectedByDeletionForStitching({ + knex: options.knex, + entityRefs: refsToDelete, + }); + await knex + .delete() + .from('refresh_state') + .whereIn('entity_ref', refsToDelete); + } // Delete the references that originate only from this entity provider. Note // that there may be more than one entity provider making a "claim" for a @@ -190,7 +66,202 @@ export async function deleteWithEagerPruningOfChildren(options: { .where('source_key', '=', sourceKey) .whereIn('target_entity_ref', refs) .delete(); + + removedCount += orphanEntityRefs.length; } return removedCount; } + +async function findDescendantsThatWouldHaveBeenOrphanedByDeletion(options: { + knex: Knex | Knex.Transaction; + refs: string[]; + sourceKey: string; +}): Promise<{ orphanEntityRefs: string[] }> { + const { knex, refs, sourceKey } = options; + + const orphans: string[] = + // First find all nodes that can be reached downwards from the roots + // (deletion targets), including the roots themselves, by traversing + // down the refresh_state_references table. Note that this query + // starts with a condition that source_key = our source key, and + // target_entity_ref is one of the deletion targets. This has two + // effects: it won't match attempts at deleting something that didn't + // originate from us in the first place, and also won't match non-root + // entities (source_key would be null for those). + // + // KeyA - R1 - R2 Legend: + // \ ----------------------------------------- + // R3 Key* Source key + // / R* Entity ref + // KeyA - R4 - R5 lines Individual references; sources to + // / the left and targets to the right + // KeyB --- R6 + // + // The scenario is that KeyA wants to delete R1. + // + // The query starts with the KeyA-R1 reference, and then traverses + // down to also find R2 and R3. It uses union instead of union all, + // because it wants to find the set of unique descendants even if + // the tree has unexpected loops etc. + await knex + .withRecursive('descendants', ['entity_ref'], initial => + initial + .select('target_entity_ref') + .from('refresh_state_references') + .where('source_key', '=', sourceKey) + .whereIn('target_entity_ref', refs) + .union(recursive => + recursive + .select('refresh_state_references.target_entity_ref') + .from('descendants') + .join( + 'refresh_state_references', + 'descendants.entity_ref', + 'refresh_state_references.source_entity_ref', + ), + ), + ) + // Then for each descendant, traverse all the way back upwards through + // the refresh_state_references table to get an exhaustive list of all + // references that are part of keeping that particular descendant + // alive. + // + // Continuing the scenario from above, starting from R3, it goes + // upwards to find every pair along every relation line. + // + // Top branch: R2-R3, R1-R2, KeyA-R1 + // Middle branch: R5-R3, R4-R5, KeyA-R4 + // Bottom branch: R6-R5, KeyB-R6 + // + // Note that this all applied to the subject R3. The exact same thing + // will be done starting from each other descendant (R2 and R1). They + // only have one and two references to find, respectively. + // + // This query also uses union instead of union all, to get the set of + // distinct relations even if the tree has unexpected loops etc. + .withRecursive( + 'ancestors', + ['source_key', 'source_entity_ref', 'target_entity_ref', 'subject'], + initial => + initial + .select( + 'refresh_state_references.source_key', + 'refresh_state_references.source_entity_ref', + 'refresh_state_references.target_entity_ref', + 'descendants.entity_ref', + ) + .from('descendants') + .join( + 'refresh_state_references', + 'refresh_state_references.target_entity_ref', + 'descendants.entity_ref', + ) + .union(recursive => + recursive + .select( + 'refresh_state_references.source_key', + 'refresh_state_references.source_entity_ref', + 'refresh_state_references.target_entity_ref', + 'ancestors.subject', + ) + .from('ancestors') + .join( + 'refresh_state_references', + 'refresh_state_references.target_entity_ref', + 'ancestors.source_entity_ref', + ), + ), + ) + // Finally, from that list of ancestor relations per descendant, pick + // out the ones that are roots (have a source_key). Specifically, find + // ones that seem to be be either (1) from another source, or (2) + // aren't part of the deletion targets. Those are markers that tell us + // that the corresponding descendant should be kept alive and NOT + // subject to eager deletion, because there's "something else" (not + // targeted for deletion) that has references down through the tree to + // it. + // + // Continuing the scenario from above, for R3 we have + // + // KeyA-R1, KeyA-R4, KeyB-R6 + // + // This tells us that R3 should be kept alive for two reasons: it's + // referenced by a node that isn't being deleted (R4), and also by + // another source (KeyB). What about R1 and R2? They both have + // + // KeyA-R1 + // + // So those should be deleted, since they are definitely only being + // kept alive by something that's about to be deleted. + // + // Final shape of the tree: + // + // R3 + // / + // KeyA - R4 - R5 + // / + // KeyB --- R6 + .with('retained', ['entity_ref'], notPartOfDeletion => + notPartOfDeletion + .select('subject') + .from('ancestors') + .whereNotNull('ancestors.source_key') + .where(foreignKeyOrRef => + foreignKeyOrRef + .where('ancestors.source_key', '!=', sourceKey) + .orWhereNotIn('ancestors.target_entity_ref', refs), + ), + ) + // Return all descendants minus the retained ones + .select('descendants.entity_ref AS entity_ref') + .from('descendants') + .leftOuterJoin( + 'retained', + 'retained.entity_ref', + 'descendants.entity_ref', + ) + .whereNull('retained.entity_ref') + .then(rows => rows.map(row => row.entity_ref)); + + return { orphanEntityRefs: orphans }; +} + +async function markEntitiesAffectedByDeletionForStitching(options: { + knex: Knex | Knex.Transaction; + entityRefs: string[]; +}) { + const { knex, entityRefs } = options; + + // We want to re-stitch anything that has a relation pointing to the + // soon-to-be-deleted entity. In many circumstances we also re-stitch children + // in the refresh_state_references graph because their orphan state might + // change, but not here - this code by its very definition is meant to not + // leave any orphans behind, so we can simplify away that. + const affectedIds = await knex + .select('refresh_state.entity_id AS entity_id') + .from('relations') + .join( + 'refresh_state', + 'relations.source_entity_ref', + 'refresh_state.entity_ref', + ) + .whereIn('relations.target_entity_ref', entityRefs) + .then(rows => rows.map(row => row.entity_id)); + + for (const ids of lodash.chunk(affectedIds, 1000)) { + await knex + .table('final_entities') + .update({ + hash: 'force-stitching', + }) + .whereIn('entity_id', ids); + await knex + .table('refresh_state') + .update({ + result_hash: 'force-stitching', + next_update_at: knex.fn.now(), + }) + .whereIn('entity_id', ids); + } +}