From a68c0b0ec38232e56e3c6aa9298bc86052ef3814 Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 16 Jan 2025 08:55:40 +0100 Subject: [PATCH 1/5] feat: ensure that refresh_state_references are cleaned Signed-off-by: blam --- .../src/database/DefaultProviderDatabase.ts | 1 - .../src/tests/integration.test.ts | 164 ++++++++++++++++-- 2 files changed, 149 insertions(+), 16 deletions(-) diff --git a/plugins/catalog-backend/src/database/DefaultProviderDatabase.ts b/plugins/catalog-backend/src/database/DefaultProviderDatabase.ts index 03f72e06b77a2..a078b578e49e0 100644 --- a/plugins/catalog-backend/src/database/DefaultProviderDatabase.ts +++ b/plugins/catalog-backend/src/database/DefaultProviderDatabase.ts @@ -165,7 +165,6 @@ export class DefaultProviderDatabase implements ProviderDatabase { await tx('refresh_state_references') .where('target_entity_ref', entityRef) - .andWhere({ source_key: options.sourceKey }) .delete(); if (ok) { diff --git a/plugins/catalog-backend/src/tests/integration.test.ts b/plugins/catalog-backend/src/tests/integration.test.ts index e85ebe48ced96..5aa1a03f6e238 100644 --- a/plugins/catalog-backend/src/tests/integration.test.ts +++ b/plugins/catalog-backend/src/tests/integration.test.ts @@ -31,7 +31,7 @@ import { } from '@backstage/plugin-catalog-node'; import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { createHash } from 'crypto'; -import { Knex } from 'knex'; +import knexFactory, { Knex } from 'knex'; import { EntitiesCatalog } from '../catalog/types'; import { DefaultCatalogDatabase } from '../database/DefaultCatalogDatabase'; import { DefaultProcessingDatabase } from '../database/DefaultProcessingDatabase'; @@ -55,6 +55,7 @@ import { mockServices } from '@backstage/backend-test-utils'; import { LoggerService } from '@backstage/backend-plugin-api'; import { DatabaseManager } from '@backstage/backend-common'; import { entitiesResponseToObjects } from '../service/response'; +import { DbRefreshStateReferencesRow } from '../database/tables'; const voidLogger = mockServices.logger.mock(); @@ -194,7 +195,7 @@ class TestHarness { readonly #catalog: EntitiesCatalog; readonly #engine: CatalogProcessingEngine; readonly #refresh: RefreshService; - readonly #provider: TestProvider; + readonly #providers: TestProvider[]; readonly #proxyProgressTracker: ProxyProgressTracker; static async create(options?: { @@ -202,6 +203,7 @@ class TestHarness { logger?: LoggerService; db?: Knex; permissions?: PermissionEvaluator; + providers?: TestProvider[]; processEntity?( entity: Entity, location: LocationSpec, @@ -300,9 +302,8 @@ class TestHarness { const refresh = new DefaultRefreshService({ database: catalogDatabase }); - const provider = new TestProvider(); - - await connectEntityProviders(providerDatabase, [provider]); + const providers = options?.providers ?? [new TestProvider()]; + await connectEntityProviders(providerDatabase, providers); return new TestHarness( catalog, @@ -317,7 +318,7 @@ class TestHarness { }, }, refresh, - provider, + providers, proxyProgressTracker, ); } @@ -326,13 +327,13 @@ class TestHarness { catalog: EntitiesCatalog, engine: CatalogProcessingEngine, refresh: RefreshService, - provider: TestProvider, + providers: TestProvider[], proxyProgressTracker: ProxyProgressTracker, ) { this.#catalog = catalog; this.#engine = engine; this.#refresh = refresh; - this.#provider = provider; + this.#providers = providers; this.#proxyProgressTracker = proxyProgressTracker; } @@ -353,13 +354,15 @@ class TestHarness { } async setInputEntities(entities: (Entity & { locationKey?: string })[]) { - return this.#provider.getConnection().applyMutation({ - type: 'full', - entities: entities.map(({ locationKey, ...entity }) => ({ - entity, - locationKey, - })), - }); + for (const provider of this.#providers) { + await provider.getConnection().applyMutation({ + type: 'full', + entities: entities.map(({ locationKey, ...entity }) => ({ + entity, + locationKey, + })), + }); + } } async getOutputEntities(): Promise> { @@ -838,4 +841,135 @@ describe('Catalog Backend Integration', () => { }, }); }); + + it('should replace any refresh_state_references that are dangling after claiming an entityRef with locationKey', async () => { + const db = knexFactory({ + client: 'better-sqlite3', + connection: ':memory:', + useNullAsDefault: true, + }); + + const firstProvider = new TestProvider(); + firstProvider.getProviderName = () => 'first'; + + const secondProvider = new TestProvider(); + secondProvider.getProviderName = () => 'second'; + + const harness = await TestHarness.create({ + providers: [firstProvider, secondProvider], + db, + }); + + await firstProvider.getConnection().applyMutation({ + type: 'full', + entities: [ + { + entity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'component-1', + annotations: { + 'backstage.io/managed-by-location': 'url:.', + 'backstage.io/managed-by-origin-location': 'url:.', + }, + }, + spec: { + type: 'service', + owner: 'no-location-key', + }, + }, + }, + ], + }); + + await expect(harness.process()).resolves.toEqual({}); + + await expect(harness.getOutputEntities()).resolves.toEqual({ + 'component:default/component-1': expect.objectContaining({ + spec: { + type: 'service', + owner: 'no-location-key', + }, + }), + }); + + await secondProvider.getConnection().applyMutation({ + type: 'full', + entities: [ + { + locationKey: 'takeover', + entity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'component-1', + annotations: { + 'backstage.io/managed-by-location': 'url:.', + 'backstage.io/managed-by-origin-location': 'url:.', + }, + }, + spec: { + type: 'service', + owner: 'location-key', + }, + }, + }, + ], + }); + + await expect(harness.process()).resolves.toEqual({}); + + await expect(harness.getOutputEntities()).resolves.toEqual({ + 'component:default/component-1': expect.objectContaining({ + spec: { + type: 'service', + owner: 'location-key', + }, + }), + }); + + await expect( + db('refresh_state_references') + .where({ target_entity_ref: 'component:default/component-1' }) + .select(), + ).resolves.toEqual([ + expect.objectContaining({ + source_key: 'second', + target_entity_ref: 'component:default/component-1', + }), + ]); + + await secondProvider.getConnection().applyMutation({ + type: 'full', + entities: [ + { + locationKey: 'takeover', + entity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'component-2', + annotations: { + 'backstage.io/managed-by-location': 'url:.', + 'backstage.io/managed-by-origin-location': 'url:.', + }, + }, + spec: { + type: 'service', + owner: 'location-key', + }, + }, + }, + ], + }); + + await expect(harness.process()).resolves.toEqual({}); + + await expect( + db('refresh_state_references') + .where({ target_entity_ref: 'component:default/component-1' }) + .select(), + ).resolves.toEqual([]); + }); }); From 22dfc634047af9da6e3568af7fd9e1c87ab323d4 Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 16 Jan 2025 08:57:14 +0100 Subject: [PATCH 2/5] chore: add another assert to check output entities Signed-off-by: blam --- plugins/catalog-backend/src/tests/integration.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/plugins/catalog-backend/src/tests/integration.test.ts b/plugins/catalog-backend/src/tests/integration.test.ts index 5aa1a03f6e238..3ef45c024dd01 100644 --- a/plugins/catalog-backend/src/tests/integration.test.ts +++ b/plugins/catalog-backend/src/tests/integration.test.ts @@ -971,5 +971,14 @@ describe('Catalog Backend Integration', () => { .where({ target_entity_ref: 'component:default/component-1' }) .select(), ).resolves.toEqual([]); + + await expect(harness.getOutputEntities()).resolves.toEqual({ + 'component:default/component-2': expect.objectContaining({ + spec: { + type: 'service', + owner: 'location-key', + }, + }), + }); }); }); From f178b129b4e8d4d79f1da43011376fbd69943acf Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 16 Jan 2025 08:58:12 +0100 Subject: [PATCH 3/5] chore: changeset Signed-off-by: blam --- .changeset/fair-mangos-sleep.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fair-mangos-sleep.md diff --git a/.changeset/fair-mangos-sleep.md b/.changeset/fair-mangos-sleep.md new file mode 100644 index 0000000000000..e2b24d441d2a2 --- /dev/null +++ b/.changeset/fair-mangos-sleep.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Cleanup `refresh_state_references` for providers that are no longer in control of a `refresh_state` row for entity From ad39179ce73b9bea83cc5d58b1961c9734c384db Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 16 Jan 2025 09:09:59 +0100 Subject: [PATCH 4/5] chore: only remove if ok Signed-off-by: blam Signed-off-by: blam --- .../catalog-backend/src/database/DefaultProviderDatabase.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/plugins/catalog-backend/src/database/DefaultProviderDatabase.ts b/plugins/catalog-backend/src/database/DefaultProviderDatabase.ts index a078b578e49e0..9ec5d1359166b 100644 --- a/plugins/catalog-backend/src/database/DefaultProviderDatabase.ts +++ b/plugins/catalog-backend/src/database/DefaultProviderDatabase.ts @@ -165,9 +165,14 @@ export class DefaultProviderDatabase implements ProviderDatabase { await tx('refresh_state_references') .where('target_entity_ref', entityRef) + .andWhere({ source_key: options.sourceKey }) .delete(); if (ok) { + await tx('refresh_state_references') + .where('target_entity_ref', entityRef) + .delete(); + await tx( 'refresh_state_references', ).insert({ From 67bf279c8cf1416aa0e2fe731600c03ae1cdf1b9 Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 16 Jan 2025 10:30:34 +0100 Subject: [PATCH 5/5] chore: refactor tests a little bit Signed-off-by: blam --- .../src/tests/integration.test.ts | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/plugins/catalog-backend/src/tests/integration.test.ts b/plugins/catalog-backend/src/tests/integration.test.ts index 3ef45c024dd01..bc4a68e5c35c6 100644 --- a/plugins/catalog-backend/src/tests/integration.test.ts +++ b/plugins/catalog-backend/src/tests/integration.test.ts @@ -195,7 +195,7 @@ class TestHarness { readonly #catalog: EntitiesCatalog; readonly #engine: CatalogProcessingEngine; readonly #refresh: RefreshService; - readonly #providers: TestProvider[]; + readonly #provider: TestProvider; readonly #proxyProgressTracker: ProxyProgressTracker; static async create(options?: { @@ -203,7 +203,7 @@ class TestHarness { logger?: LoggerService; db?: Knex; permissions?: PermissionEvaluator; - providers?: TestProvider[]; + additionalProviders?: EntityProvider[]; processEntity?( entity: Entity, location: LocationSpec, @@ -302,7 +302,13 @@ class TestHarness { const refresh = new DefaultRefreshService({ database: catalogDatabase }); - const providers = options?.providers ?? [new TestProvider()]; + const provider = new TestProvider(); + const providers: EntityProvider[] = [provider]; + + if (options?.additionalProviders) { + providers.push(...options.additionalProviders); + } + await connectEntityProviders(providerDatabase, providers); return new TestHarness( @@ -318,7 +324,7 @@ class TestHarness { }, }, refresh, - providers, + provider, proxyProgressTracker, ); } @@ -327,13 +333,13 @@ class TestHarness { catalog: EntitiesCatalog, engine: CatalogProcessingEngine, refresh: RefreshService, - providers: TestProvider[], + provider: TestProvider, proxyProgressTracker: ProxyProgressTracker, ) { this.#catalog = catalog; this.#engine = engine; this.#refresh = refresh; - this.#providers = providers; + this.#provider = provider; this.#proxyProgressTracker = proxyProgressTracker; } @@ -354,15 +360,13 @@ class TestHarness { } async setInputEntities(entities: (Entity & { locationKey?: string })[]) { - for (const provider of this.#providers) { - await provider.getConnection().applyMutation({ - type: 'full', - entities: entities.map(({ locationKey, ...entity }) => ({ - entity, - locationKey, - })), - }); - } + await this.#provider.getConnection().applyMutation({ + type: 'full', + entities: entities.map(({ locationKey, ...entity }) => ({ + entity, + locationKey, + })), + }); } async getOutputEntities(): Promise> { @@ -856,7 +860,7 @@ describe('Catalog Backend Integration', () => { secondProvider.getProviderName = () => 'second'; const harness = await TestHarness.create({ - providers: [firstProvider, secondProvider], + additionalProviders: [firstProvider, secondProvider], db, });