From 1aec041c3451f0e7ac33b01035c0bc7d86007bc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Fri, 10 Feb 2023 19:31:53 +0100 Subject: [PATCH] Handle eager deletion even in the face of duplicate references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/itchy-rings-rule.md | 5 + .../database/DefaultCatalogDatabase.test.ts | 1 + .../database/DefaultProviderDatabase.test.ts | 34 +++ .../deleteWithEagerPruningOfChildren.test.ts | 278 ++++++++++++++++++ .../deleteWithEagerPruningOfChildren.ts | 178 ++++++----- 5 files changed, 419 insertions(+), 77 deletions(-) create mode 100644 .changeset/itchy-rings-rule.md create mode 100644 plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.test.ts 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..c600825d6f 100644 --- a/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.ts +++ b/plugins/catalog-backend/src/database/operations/provider/deleteWithEagerPruningOfChildren.ts @@ -31,113 +31,137 @@ 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 + descendants(entity_ref) AS ( + SELECT target_entity_ref FROM refresh_state_references - WHERE source_key = "R1" AND target_entity_ref = "A" + WHERE source_key = "R1" AND target_entity_ref IN [...refs] UNION - SELECT descendants.root_id, target_entity_ref + SELECT 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 + -- All the individual relations that can be reached upwards from each descendant + ancestors(source_key, source_entity_ref, target_entity_ref, subject) AS ( + SELECT source_key, source_entity_ref, target_entity_ref, descendants.entity_ref FROM descendants + JOIN refresh_state_references ON refresh_state_references.target_entity_ref = descendants.entity_ref UNION SELECT - CASE WHEN source_key IS NOT NULL THEN id ELSE NULL END, - source_entity_ref, - ancestors.to_entity_ref + 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 ON target_entity_ref = ancestors.via_entity_ref + JOIN refresh_state_references ON refresh_state_references.target_entity_ref = ancestors.source_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; + -- Exclude those who seem to have a root relation somewhere upwards that's not part of our own deletion + WHERE NOT EXISTS ( + SELECT * FROM ancestors + WHERE ancestors.subject = descendants.entity_ref + AND ancestors.source_key IS NOT NULL + AND (ancestors.source_key != "R1" OR ancestors.target_entity_ref NOT IN [...refs]) + ) */ 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', - }) - .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', - }) - .from('ancestors') - .join('refresh_state_references', { - target_entity_ref: 'ancestors.via_entity_ref', - }); - }); - }) + .withRecursive( + 'descendants', + ['entity_ref'], + function descendants(outer) { + return outer + .select({ 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({ + 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 relations that can be reached upwards from each descendant + .withRecursive( + 'ancestors', + [ + 'source_key', + 'source_entity_ref', + 'target_entity_ref', + 'subject', + ], + function ancestors(outer) { + return outer + .select({ + source_key: 'refresh_state_references.source_key', + source_entity_ref: + 'refresh_state_references.source_entity_ref', + target_entity_ref: + 'refresh_state_references.target_entity_ref', + subject: 'descendants.entity_ref', + }) + .from('descendants') + .join('refresh_state_references', { + 'refresh_state_references.target_entity_ref': + 'descendants.entity_ref', + }) + .union(function recursive(inner) { + return inner + .select({ + source_key: 'refresh_state_references.source_key', + source_entity_ref: + 'refresh_state_references.source_entity_ref', + target_entity_ref: + 'refresh_state_references.target_entity_ref', + subject: 'ancestors.subject', + }) + .from('ancestors') + .join('refresh_state_references', { + 'refresh_state_references.target_entity_ref': + 'ancestors.source_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'); + // Exclude those who seem to have a root relation somewhere upwards that's not part of our own deletion + .whereNotExists(function otherAncestors(outer) { + outer + .from('ancestors') + .where( + 'ancestors.subject', + '=', + tx.ref('descendants.entity_ref'), + ) + .whereNotNull('ancestors.source_key') + .andWhere(function differentRoot(inner) { + inner + .where('ancestors.source_key', '!=', sourceKey) + .orWhereNotIn('ancestors.target_entity_ref', refs); + }); }) - .whereNull('ancestors.root_id') ); }) .delete();