diff --git a/.changeset/itchy-rings-rule.md b/.changeset/itchy-rings-rule.md new file mode 100644 index 0000000000..3e8b9e8b2b --- /dev/null +++ b/.changeset/itchy-rings-rule.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Fixed an issue where entities sometimes were not properly deleted during a full mutation. diff --git a/plugins/catalog-backend/src/database/DefaultCatalogDatabase.test.ts b/plugins/catalog-backend/src/database/DefaultCatalogDatabase.test.ts index b873f6a985..827ab58ebe 100644 --- a/plugins/catalog-backend/src/database/DefaultCatalogDatabase.test.ts +++ b/plugins/catalog-backend/src/database/DefaultCatalogDatabase.test.ts @@ -105,6 +105,7 @@ describe('DefaultCatalogDatabase', () => { 'location:default/root-2', ]); }, + 60_000, ); }); }); diff --git a/plugins/catalog-backend/src/database/DefaultProviderDatabase.test.ts b/plugins/catalog-backend/src/database/DefaultProviderDatabase.test.ts index 140925b6fd..09bec187fa 100644 --- a/plugins/catalog-backend/src/database/DefaultProviderDatabase.test.ts +++ b/plugins/catalog-backend/src/database/DefaultProviderDatabase.test.ts @@ -745,5 +745,39 @@ describe('DefaultProviderDatabase', () => { }, 60_000, ); + + it.each(databases.eachSupportedId())( + 'should gracefully handle accidental duplicate refresh state references when deletion happens during a full sync, %p', + async databaseId => { + const fakeLogger = { debug: jest.fn() }; + const { knex, db } = await createDatabase( + databaseId, + fakeLogger as any, + ); + + await createLocations(knex, ['component:default/a']); + + await insertRefRow(knex, { + source_key: 'a', + target_entity_ref: 'component:default/a', + }); + await insertRefRow(knex, { + source_key: 'a', + target_entity_ref: 'component:default/a', + }); + + await db.transaction(async tx => { + await db.replaceUnprocessedEntities(tx, { + type: 'full', + sourceKey: 'a', + items: [], + }); + }); + + const state = await knex('refresh_state').select(); + expect(state).toEqual([]); + }, + 60_000, + ); }); }); diff --git a/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.test.ts b/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.test.ts new file mode 100644 index 0000000000..8ab94df64a --- /dev/null +++ b/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.test.ts @@ -0,0 +1,278 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +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 { deleteWithEagerPruningOfChildren } from './deleteWithEagerPruningOfChildren'; + +describe('deleteWithEagerPruningOfChildren', () => { + const databases = TestDatabases.create({ + ids: ['MYSQL_8', 'POSTGRES_13', 'POSTGRES_9', 'SQLITE_3'], + }); + + async function createDatabase(databaseId: TestDatabaseId) { + const knex = await databases.init(databaseId); + await applyDatabaseMigrations(knex); + return knex; + } + + async function run( + knex: Knex, + options: { entityRefs: string[]; sourceKey: string }, + ): Promise { + let result: number; + await knex.transaction( + async tx => { + // We can't return here, as knex swallows the return type in case the + // transaction is rolled back: + // https://github.com/knex/knex/blob/e37aeaa31c8ef9c1b07d2e4d3ec6607e557d800d/lib/transaction.js#L136 + result = await deleteWithEagerPruningOfChildren({ tx, ...options }); + }, + { + // If we explicitly trigger a rollback, don't fail. + doNotRejectOnRollback: true, + }, + ); + return result!; + } + + async function insertReference( + knex: Knex, + ...refs: DbRefreshStateReferencesRow[] + ) { + return knex('refresh_state_references').insert( + refs, + ); + } + + async function insertEntity(knex: Knex, ...entityRefs: string[]) { + for (const ref of entityRefs) { + await knex('refresh_state').insert({ + entity_id: uuid.v4(), + entity_ref: ref, + unprocessed_entity: '{}', + processed_entity: '{}', + errors: '[]', + next_update_at: '2021-04-01 13:37:00', + last_discovery_at: '2021-04-01 13:37:00', + }); + } + } + + async function remainingEntities(knex: Knex) { + const rows = await knex('refresh_state') + .orderBy('entity_ref') + .select('entity_ref'); + return rows.map(r => r.entity_ref); + } + + it.each(databases.eachSupportedId())( + 'works for the simple path, %p', + async databaseId => { + /* + P1 - E1 - E2 + + P1 - E3 + + P1 - E4 + + P2 - E5 + + Scenario: P1 issues delete for E1 and E3 + + Result: E1, E2, and E3 deleted; E4 and E5 remain + */ + const knex = await createDatabase(databaseId); + await insertEntity(knex, 'E1', 'E2', 'E3', 'E4', 'E5'); + await insertReference( + knex, + { source_key: 'P1', target_entity_ref: 'E1' }, + { source_entity_ref: 'E1', target_entity_ref: 'E2' }, + { source_key: 'P1', target_entity_ref: 'E3' }, + { source_key: 'P1', target_entity_ref: 'E4' }, + { source_key: 'P2', target_entity_ref: 'E5' }, + ); + await run(knex, { sourceKey: 'P1', entityRefs: ['E1', 'E3'] }); + await expect(remainingEntities(knex)).resolves.toEqual(['E4', 'E5']); + }, + 60_000, + ); + + it.each(databases.eachSupportedId())( + 'works when there are multiple identical references, %p', + async databaseId => { + /* + P1 + \ + E1 + / + P1 + + P1 - E2 + + Scenario: P1 issues delete for E1 + + Result: E1 deleted; E2 remains + */ + const knex = await createDatabase(databaseId); + await insertEntity(knex, 'E1', 'E2'); + await insertReference( + knex, + { source_key: 'P1', target_entity_ref: 'E1' }, + { source_key: 'P1', target_entity_ref: 'E1' }, + { source_key: 'P1', target_entity_ref: 'E2' }, + ); + await run(knex, { sourceKey: 'P1', entityRefs: ['E1'] }); + await expect(remainingEntities(knex)).resolves.toEqual(['E2']); + }, + 60_000, + ); + + it.each(databases.eachSupportedId())( + 'leaves out things that have roots in other source keys, %p', + async databaseId => { + /* + P1 - E1 + \ + E2 + / + P2 - E3 + + Scenario: P1 issues delete for E1 + + Result: E1 deleted; E2 and E3 remain + */ + const knex = await createDatabase(databaseId); + await insertEntity(knex, 'E1', 'E2', 'E3'); + await insertReference( + knex, + { source_key: 'P1', target_entity_ref: 'E1' }, + { source_entity_ref: 'E1', target_entity_ref: 'E2' }, + { source_key: 'P2', target_entity_ref: 'E3' }, + { source_entity_ref: 'E3', target_entity_ref: 'E2' }, + ); + await run(knex, { sourceKey: 'P1', entityRefs: ['E1'] }); + await expect(remainingEntities(knex)).resolves.toEqual(['E2', 'E3']); + }, + 60_000, + ); + + it.each(databases.eachSupportedId())( + 'leaves out things that have several different roots for the same source key, %p', + async databaseId => { + /* + P1 - E1 + \ + E2 + / + P1 - E3 + + Scenario: P1 issues delete for E1 + + Result: E1 deleted; E2 and E3 remain + */ + const knex = await createDatabase(databaseId); + await insertEntity(knex, 'E1', 'E2', 'E3'); + await insertReference( + knex, + { source_key: 'P1', target_entity_ref: 'E1' }, + { source_entity_ref: 'E1', target_entity_ref: 'E2' }, + { source_key: 'P1', target_entity_ref: 'E3' }, + { source_entity_ref: 'E3', target_entity_ref: 'E2' }, + ); + await run(knex, { sourceKey: 'P1', entityRefs: ['E1'] }); + await expect(remainingEntities(knex)).resolves.toEqual(['E2', 'E3']); + }, + 60_000, + ); + + it.each(databases.eachSupportedId())( + 'handles cycles and diamonds gracefully, %p', + async databaseId => { + /* + P1 - E1 <-> E2 + \ + E4 E6 + / \ / + P1 -- E3 -- E5 + + Scenario: P1 issues delete for E1, then for E3 + + Result: Everything deleted, but in two steps + */ + const knex = await createDatabase(databaseId); + await insertEntity(knex, 'E1', 'E2', 'E3', 'E4', 'E5', 'E6'); + await insertReference( + knex, + { source_key: 'P1', target_entity_ref: 'E1' }, + { source_entity_ref: 'E1', target_entity_ref: 'E2' }, + { source_entity_ref: 'E2', target_entity_ref: 'E1' }, + { source_entity_ref: 'E2', target_entity_ref: 'E6' }, + { source_key: 'P1', target_entity_ref: 'E3' }, + { source_entity_ref: 'E3', target_entity_ref: 'E4' }, + { source_entity_ref: 'E4', target_entity_ref: 'E5' }, + { source_entity_ref: 'E3', target_entity_ref: 'E5' }, + { source_entity_ref: 'E5', target_entity_ref: 'E6' }, + ); + await run(knex, { sourceKey: 'P1', entityRefs: ['E1'] }); + await expect(remainingEntities(knex)).resolves.toEqual([ + 'E3', + 'E4', + 'E5', + 'E6', + ]); + await run(knex, { sourceKey: 'P1', entityRefs: ['E3'] }); + await expect(remainingEntities(knex)).resolves.toEqual([]); + }, + 60_000, + ); + + it.each(databases.eachSupportedId())( + 'silently ignores attempts to delete things that are not your own and/or are not roots, %p', + async databaseId => { + /* + P1 - E1 - E2 + + P1 - E3 + + P2 - E4 + + Scenario: P1 issues delete for E2, E3, and E4 + + Result: E3 is deleted; E1, E2 and E4 remain + */ + const knex = await createDatabase(databaseId); + await insertEntity(knex, 'E1', 'E2', 'E3', 'E4'); + await insertReference( + knex, + { source_key: 'P1', target_entity_ref: 'E1' }, + { source_entity_ref: 'E1', target_entity_ref: 'E2' }, + { source_key: 'P1', target_entity_ref: 'E3' }, + { source_key: 'P2', target_entity_ref: 'E4' }, + ); + await run(knex, { sourceKey: 'P1', entityRefs: ['E2', 'E3', 'E4'] }); + await expect(remainingEntities(knex)).resolves.toEqual([ + 'E1', + 'E2', + 'E4', + ]); + }, + 60_000, + ); +}); diff --git a/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.ts b/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.ts index 3d642cbf3a..33da37bc55 100644 --- a/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.ts +++ b/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.ts @@ -16,7 +16,7 @@ import { Knex } from 'knex'; import lodash from 'lodash'; -import { DbRefreshStateReferencesRow, DbRefreshStateRow } from '../../tables'; +import { DbRefreshStateReferencesRow } from '../../tables'; /** * Given a number of entity refs originally created by a given entity provider @@ -31,116 +31,157 @@ export async function deleteWithEagerPruningOfChildren(options: { sourceKey: string; }): Promise { const { tx, entityRefs, sourceKey } = options; - let removedCount = 0; - - const rootId = () => - tx.raw( - tx.client.config.client.includes('mysql') - ? 'CAST(NULL as UNSIGNED INT)' - : 'CAST(NULL as INT)', - [], - ); // Split up the operation by (large) chunks, so that we do not hit database // limits for the number of permitted bindings on a precompiled statement + let removedCount = 0; for (const refs of lodash.chunk(entityRefs, 1000)) { - /* - WITH RECURSIVE - -- All the nodes that can be reached downwards from our root - descendants(root_id, entity_ref) AS ( - SELECT id, target_entity_ref - FROM refresh_state_references - WHERE source_key = "R1" AND target_entity_ref = "A" - UNION - SELECT descendants.root_id, target_entity_ref - FROM descendants - JOIN refresh_state_references ON source_entity_ref = descendants.entity_ref - ), - -- All the nodes that can be reached upwards from the descendants - ancestors(root_id, via_entity_ref, to_entity_ref) AS ( - SELECT CAST(NULL as INT), entity_ref, entity_ref - FROM descendants - UNION - SELECT - CASE WHEN source_key IS NOT NULL THEN id ELSE NULL END, - source_entity_ref, - ancestors.to_entity_ref - FROM ancestors - JOIN refresh_state_references ON target_entity_ref = ancestors.via_entity_ref - ) - -- Start out with all of the descendants - SELECT descendants.entity_ref - FROM descendants - -- Expand with all ancestors that point to those, but aren't the current root - LEFT OUTER JOIN ancestors - ON ancestors.to_entity_ref = descendants.entity_ref - AND ancestors.root_id IS NOT NULL - AND ancestors.root_id != descendants.root_id - -- Exclude all lines that had such a foreign ancestor - WHERE ancestors.root_id IS NULL; - */ - removedCount += await tx('refresh_state') - .whereIn('entity_ref', function orphanedEntityRefs(orphans) { - return ( - orphans - // All the nodes that can be reached downwards from our root - .withRecursive('descendants', function descendants(outer) { - return outer - .select({ root_id: 'id', entity_ref: 'target_entity_ref' }) - .from('refresh_state_references') - .where('source_key', sourceKey) - .whereIn('target_entity_ref', refs) - .union(function recursive(inner) { - return inner - .select({ - root_id: 'descendants.root_id', - entity_ref: 'refresh_state_references.target_entity_ref', - }) - .from('descendants') - .join('refresh_state_references', { - 'descendants.entity_ref': - 'refresh_state_references.source_entity_ref', - }); - }); - }) - // All the nodes that can be reached upwards from the descendants - .withRecursive('ancestors', function ancestors(outer) { - return outer - .select({ - root_id: rootId(), - via_entity_ref: 'entity_ref', - to_entity_ref: 'entity_ref', - }) + removedCount += await tx + .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') - .union(function recursive(inner) { - return inner - .select({ - root_id: tx.raw( - 'CASE WHEN source_key IS NOT NULL THEN id ELSE NULL END', - [], - ), - via_entity_ref: 'source_entity_ref', - to_entity_ref: 'ancestors.to_entity_ref', - }) + .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', { - target_entity_ref: 'ancestors.via_entity_ref', - }); - }); - }) - // Start out with all of the descendants - .select('descendants.entity_ref') - .from('descendants') - // Expand with all ancestors that point to those, but aren't the current root - .leftOuterJoin('ancestors', function keepaliveRoots() { - this.on('ancestors.to_entity_ref', '=', 'descendants.entity_ref'); - this.andOnNotNull('ancestors.root_id'); - this.andOn('ancestors.root_id', '!=', 'descendants.root_id'); - }) - .whereNull('ancestors.root_id') - ); - }) - .delete(); + .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'), + ); // 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