From 4a22d3786d801a57302de45fcfcfdb3a918119a3 Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 24 Oct 2024 16:53:32 +0200 Subject: [PATCH 1/9] chore: remove the refresh_state table from the read path, and have `entity_ref` in `final_entities` Signed-off-by: blam Signed-off-by: blam --- ...104700_add_entity_ref_to_final_entities.js | 47 ++++++++++++ .../operations/stitcher/performStitching.ts | 3 +- .../catalog-backend/src/database/tables.ts | 1 + .../service/DefaultEntitiesCatalog.test.ts | 1 + .../src/service/DefaultEntitiesCatalog.ts | 36 +++------ .../src/tests/migrations.test.ts | 75 +++++++++++++++++++ 6 files changed, 136 insertions(+), 27 deletions(-) create mode 100644 plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js diff --git a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js new file mode 100644 index 0000000000..4ae97d810c --- /dev/null +++ b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js @@ -0,0 +1,47 @@ +/* + * Copyright 2024 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. + */ + +// @ts-check + +/** + * @param { import("knex").Knex } knex + * @returns { Promise } + */ +exports.up = async function up(knex) { + await knex.schema.alterTable('final_entities', table => { + table + .text('entity_ref') + .nullable() + .comment( + 'The entity reference of the entity that was created from the catalog processing', + ); + }); + + // Set the default entity_ref in final_entities from the refresh_state table + await knex.raw( + `UPDATE final_entities SET entity_ref = refresh_state.entity_ref FROM refresh_state WHERE refresh_state.entity_id = final_entities.entity_id`, + ); +}; + +/** + * @param { import("knex").Knex } knex + * @returns { Promise } + */ +exports.down = async function down(knex) { + await knex.schema.alterTable('final_entities', table => { + table.dropColumn('entity_ref'); + }); +}; diff --git a/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts b/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts index 0832ba5db8..dc990fefc3 100644 --- a/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts +++ b/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts @@ -212,11 +212,12 @@ export async function performStitching(options: { final_entity: JSON.stringify(entity), hash, last_updated_at: knex.fn.now(), + entity_ref: entityRef, }) .where('entity_id', entityId) .where('stitch_ticket', stitchTicket) .onConflict('entity_id') - .merge(['final_entity', 'hash', 'last_updated_at']); + .merge(['final_entity', 'hash', 'last_updated_at', 'entity_ref']); const markDeferred = async () => { if (options.strategy.mode !== 'deferred') { diff --git a/plugins/catalog-backend/src/database/tables.ts b/plugins/catalog-backend/src/database/tables.ts index 57731e3d27..df171ebb18 100644 --- a/plugins/catalog-backend/src/database/tables.ts +++ b/plugins/catalog-backend/src/database/tables.ts @@ -179,6 +179,7 @@ export type DbFinalEntitiesRow = { stitch_ticket: string; final_entity?: string; last_updated_at?: string | Date; + entity_ref?: string; }; export type DbSearchRow = { diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts index 92acd9e3ce..5b369bfa41 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts @@ -76,6 +76,7 @@ describe('DefaultEntitiesCatalog', () => { await knex('final_entities').insert({ entity_id: id, + entity_ref: entityRef, final_entity: entityJson, hash: 'h', stitch_ticket: '', diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts index c71f242969..830abd16ab 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts @@ -251,13 +251,7 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { }); if (!request?.order) { - entitiesQuery = entitiesQuery - .leftOuterJoin( - 'refresh_state', - 'refresh_state.entity_id', - 'final_entities.entity_id', - ) - .orderBy('refresh_state.entity_ref', 'asc'); // default sort + entitiesQuery = entitiesQuery.orderBy('final_entities.entity_ref', 'asc'); // default sort } else { entitiesQuery.orderBy('final_entities.entity_id', 'asc'); // stable sort } @@ -326,16 +320,11 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { for (const chunk of lodashChunk(request.entityRefs, 200)) { let query = this.database('final_entities') - .innerJoin( - 'refresh_state', - 'refresh_state.entity_id', - 'final_entities.entity_id', - ) .select({ - entityRef: 'refresh_state.entity_ref', + entityRef: 'final_entities.entity_ref', entity: 'final_entities.final_entity', }) - .whereIn('refresh_state.entity_ref', chunk); + .whereIn('final_entities.entity_ref', chunk); if (request?.filter) { query = parseFilter( @@ -343,7 +332,7 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { query, this.database, false, - 'refresh_state.entity_id', + 'final_entities.entity_id', ); } @@ -669,11 +658,8 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { } async entityAncestry(rootRef: string): Promise { - const [rootRow] = await this.database('refresh_state') - .leftJoin('final_entities', { - 'refresh_state.entity_id': 'final_entities.entity_id', - }) - .where('refresh_state.entity_ref', '=', rootRef) + const [rootRow] = await this.database('final_entities') + .where('final_entities.entity_ref', '=', rootRef) .select({ entityJson: 'final_entities.final_entity', }); @@ -698,16 +684,14 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { const parentRows = await this.database( 'refresh_state_references', ) - .innerJoin('refresh_state', { - 'refresh_state_references.source_entity_ref': - 'refresh_state.entity_ref', - }) + .innerJoin('final_entities', { - 'refresh_state.entity_id': 'final_entities.entity_id', + 'refresh_state_references.source_entity_ref': + 'final_entities.entity_ref', }) .where('refresh_state_references.target_entity_ref', '=', currentRef) .select({ - parentEntityRef: 'refresh_state.entity_ref', + parentEntityRef: 'final_entities.entity_ref', parentEntityJson: 'final_entities.final_entity', }); diff --git a/plugins/catalog-backend/src/tests/migrations.test.ts b/plugins/catalog-backend/src/tests/migrations.test.ts index 873746ac2b..69e347bad0 100644 --- a/plugins/catalog-backend/src/tests/migrations.test.ts +++ b/plugins/catalog-backend/src/tests/migrations.test.ts @@ -395,4 +395,79 @@ describe('migrations', () => { await knex.destroy(); }, ); + + it.each(databases.eachSupportedId())( + '20241024104700_add_entity_ref_to_final_entities.js, %p', + async databaseId => { + const knex = await databases.init(databaseId); + + await migrateUntilBefore( + knex, + '20241024104700_add_entity_ref_to_final_entities.js', + ); + + await knex + .insert({ + entity_id: '8222246a-b572-49cf-a702-1a0fcfaae901', + entity_ref: 'k:ns/n', + unprocessed_entity: '{}', + processed_entity: '{}', + errors: '[]', + next_update_at: knex.fn.now(), + last_discovery_at: knex.fn.now(), + }) + .into('refresh_state'); + + // Insert a simple URL before the migration + await knex + .insert({ + entity_id: '8222246a-b572-49cf-a702-1a0fcfaae901', + hash: '3f5a4d6ba8507be297bb7cd87c4b55b63e3f4c14', + stitch_ticket: '52367ed7-120b-405f-b7e0-cdd90f956312', + final_entity: '{}', + }) + .into('final_entities'); + + await migrateUpOnce(knex); + + // verify that the entity_ref column has been added + const columnInfo = await knex('final_entities').columnInfo(); + expect(columnInfo.entity_ref).not.toBeUndefined(); + + // verify that the contents of the entity_ref column are correct + await expect(knex('final_entities')).resolves.toEqual( + expect.arrayContaining([ + { + entity_id: '8222246a-b572-49cf-a702-1a0fcfaae901', + hash: '3f5a4d6ba8507be297bb7cd87c4b55b63e3f4c14', + stitch_ticket: '52367ed7-120b-405f-b7e0-cdd90f956312', + final_entity: '{}', + entity_ref: 'k:ns/n', + last_updated_at: null, + }, + ]), + ); + + await migrateDownOnce(knex); + + // verify that the entity_ref column has been removed + const revertedColumnInfo = await knex('final_entities').columnInfo(); + expect(revertedColumnInfo.entity_ref).toBeUndefined(); + + // verify that the contents are correct + await expect(knex('final_entities')).resolves.toEqual( + expect.arrayContaining([ + { + entity_id: '8222246a-b572-49cf-a702-1a0fcfaae901', + hash: '3f5a4d6ba8507be297bb7cd87c4b55b63e3f4c14', + stitch_ticket: '52367ed7-120b-405f-b7e0-cdd90f956312', + final_entity: '{}', + last_updated_at: null, + }, + ]), + ); + + await knex.destroy(); + }, + ); }); From 76857dae2ea882c926f7449ad490c30061951ba0 Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 24 Oct 2024 17:01:47 +0200 Subject: [PATCH 2/9] chore: added changeset Signed-off-by: blam --- .changeset/tender-feet-scream.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tender-feet-scream.md diff --git a/.changeset/tender-feet-scream.md b/.changeset/tender-feet-scream.md new file mode 100644 index 0000000000..081c33b17c --- /dev/null +++ b/.changeset/tender-feet-scream.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Added `entity_ref` column to `final_entities` in order to move `refresh_state` away from the read path From af653034efb312aa7f4ef4fbf25d2480e3fffaa2 Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 24 Oct 2024 17:23:47 +0200 Subject: [PATCH 3/9] chore: test if this works on all dbs Signed-off-by: blam --- .changeset/tender-feet-scream.md | 2 +- ...4104700_add_entity_ref_to_final_entities.js | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/.changeset/tender-feet-scream.md b/.changeset/tender-feet-scream.md index 081c33b17c..b8ed5dd792 100644 --- a/.changeset/tender-feet-scream.md +++ b/.changeset/tender-feet-scream.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-catalog-backend': patch +'@backstage/plugin-catalog-backend': minor --- Added `entity_ref` column to `final_entities` in order to move `refresh_state` away from the read path diff --git a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js index 4ae97d810c..a2d4a64b85 100644 --- a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js +++ b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js @@ -30,10 +30,20 @@ exports.up = async function up(knex) { ); }); - // Set the default entity_ref in final_entities from the refresh_state table - await knex.raw( - `UPDATE final_entities SET entity_ref = refresh_state.entity_ref FROM refresh_state WHERE refresh_state.entity_id = final_entities.entity_id`, - ); + // // Set the default entity_ref in final_entities from the table + // await knex.raw( + // `UPDATE final_entities SET entity_ref = refresh_state.entity_ref FROM refresh_state WHERE refresh_state.entity_id = final_entities.entity_id`, + // ); + + // 2 + await knex + .update({ + entity_ref: knex + .select('r.entity_ref') + .from('refresh_state as r') + .where('r.entity_id', knex.raw('f.entity_id')), + }) + .table('final_entities as f'); }; /** From fb62dd18d39c338f6b4ebf12eb47f911ae1b55cc Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 25 Oct 2024 08:14:11 +0200 Subject: [PATCH 4/9] chore: cleanup :tada: Signed-off-by: blam Signed-off-by: blam --- .../20241024104700_add_entity_ref_to_final_entities.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js index a2d4a64b85..3a7c98f425 100644 --- a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js +++ b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js @@ -30,12 +30,6 @@ exports.up = async function up(knex) { ); }); - // // Set the default entity_ref in final_entities from the table - // await knex.raw( - // `UPDATE final_entities SET entity_ref = refresh_state.entity_ref FROM refresh_state WHERE refresh_state.entity_id = final_entities.entity_id`, - // ); - - // 2 await knex .update({ entity_ref: knex From 7fbfd113894b68e30dfcf1759fb562725cebcba9 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 25 Oct 2024 13:54:04 +0200 Subject: [PATCH 5/9] chore: set as non-nullable after migration has completed Signed-off-by: blam --- .../20241024104700_add_entity_ref_to_final_entities.js | 7 +++++++ plugins/catalog-backend/src/database/tables.ts | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js index 3a7c98f425..b68bf1d7ef 100644 --- a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js +++ b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js @@ -38,6 +38,13 @@ exports.up = async function up(knex) { .where('r.entity_id', knex.raw('f.entity_id')), }) .table('final_entities as f'); + + // SQLite does not support ALTER COLUMN. + if (!knex.client.config.client.includes('sqlite3')) { + await knex.schema.alterTable('final_entities', table => { + table.text('entity_ref').notNullable().alter({ alterNullable: true }); + }); + } }; /** diff --git a/plugins/catalog-backend/src/database/tables.ts b/plugins/catalog-backend/src/database/tables.ts index df171ebb18..33e6250413 100644 --- a/plugins/catalog-backend/src/database/tables.ts +++ b/plugins/catalog-backend/src/database/tables.ts @@ -179,7 +179,7 @@ export type DbFinalEntitiesRow = { stitch_ticket: string; final_entity?: string; last_updated_at?: string | Date; - entity_ref?: string; + entity_ref: string; }; export type DbSearchRow = { From b0fcb44902862f2090a9f66589c26411b3ac0a6a Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 25 Oct 2024 14:10:17 +0200 Subject: [PATCH 6/9] chore: adding an index to the entity_ref column Signed-off-by: blam --- .../20241024104700_add_entity_ref_to_final_entities.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js index b68bf1d7ef..40d07a6860 100644 --- a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js +++ b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js @@ -28,6 +28,8 @@ exports.up = async function up(knex) { .comment( 'The entity reference of the entity that was created from the catalog processing', ); + + table.index('entity_ref', 'entity_ref_idx'); }); await knex @@ -53,6 +55,7 @@ exports.up = async function up(knex) { */ exports.down = async function down(knex) { await knex.schema.alterTable('final_entities', table => { + table.dropIndex('entity_ref_idx'); table.dropColumn('entity_ref'); }); }; From 53b90600dff3ae0ae73dcd1d4dca766b78b49c1c Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 5 Nov 2024 12:28:21 +0100 Subject: [PATCH 7/9] chore: fixing entity_ref insert Signed-off-by: blam --- .../src/database/operations/stitcher/performStitching.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts b/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts index dc990fefc3..5681ec3aa0 100644 --- a/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts +++ b/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts @@ -69,6 +69,7 @@ export async function performStitching(options: { .insert({ entity_id: entityResult[0].entity_id, hash: '', + entity_ref: entityRef, stitch_ticket: stitchTicket, }) .onConflict('entity_id') From 4eb0475795c43a263bf89ab43bdc27f579a8440f Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 5 Nov 2024 13:37:15 +0100 Subject: [PATCH 8/9] chore: adjust the column type and fix some tetss Signed-off-by: blam --- ...20241024104700_add_entity_ref_to_final_entities.js | 10 ++++------ .../operations/stitcher/markForStitching.test.ts | 4 ++++ .../operations/util/deleteOrphanedEntities.test.ts | 1 + .../src/providers/DefaultLocationStore.test.ts | 1 + .../src/service/DefaultEntitiesCatalog.test.ts | 1 + plugins/catalog-backend/src/tests/migrations.test.ts | 11 ++++++++--- 6 files changed, 19 insertions(+), 9 deletions(-) diff --git a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js index 40d07a6860..962ea05e22 100644 --- a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js +++ b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js @@ -23,11 +23,9 @@ exports.up = async function up(knex) { await knex.schema.alterTable('final_entities', table => { table - .text('entity_ref') + .string('entity_ref') .nullable() - .comment( - 'The entity reference of the entity that was created from the catalog processing', - ); + .comment('The entity reference of the entity'); table.index('entity_ref', 'entity_ref_idx'); }); @@ -44,7 +42,7 @@ exports.up = async function up(knex) { // SQLite does not support ALTER COLUMN. if (!knex.client.config.client.includes('sqlite3')) { await knex.schema.alterTable('final_entities', table => { - table.text('entity_ref').notNullable().alter({ alterNullable: true }); + table.string('entity_ref').notNullable().alter({ alterNullable: true }); }); } }; @@ -55,7 +53,7 @@ exports.up = async function up(knex) { */ exports.down = async function down(knex) { await knex.schema.alterTable('final_entities', table => { - table.dropIndex('entity_ref_idx'); + table.dropIndex([], 'entity_ref_idx'); table.dropColumn('entity_ref'); }); }; diff --git a/plugins/catalog-backend/src/database/operations/stitcher/markForStitching.test.ts b/plugins/catalog-backend/src/database/operations/stitcher/markForStitching.test.ts index 011fe54e83..1afa0b9c30 100644 --- a/plugins/catalog-backend/src/database/operations/stitcher/markForStitching.test.ts +++ b/plugins/catalog-backend/src/database/operations/stitcher/markForStitching.test.ts @@ -252,24 +252,28 @@ describe('markForStitching', () => { { entity_id: '1', final_entity: '{}', + entity_ref: 'k:ns/one', hash: 'old', stitch_ticket: 'old', }, { entity_id: '2', final_entity: '{}', + entity_ref: 'k:ns/two', hash: 'old', stitch_ticket: 'old', }, { entity_id: '3', final_entity: '{}', + entity_ref: 'k:ns/three', hash: 'old', stitch_ticket: 'old', }, { entity_id: '4', final_entity: '{}', + entity_ref: 'k:ns/four', hash: 'old', stitch_ticket: 'old', }, diff --git a/plugins/catalog-backend/src/database/operations/util/deleteOrphanedEntities.test.ts b/plugins/catalog-backend/src/database/operations/util/deleteOrphanedEntities.test.ts index 1dc8a42d5f..0f4f5f0c3d 100644 --- a/plugins/catalog-backend/src/database/operations/util/deleteOrphanedEntities.test.ts +++ b/plugins/catalog-backend/src/database/operations/util/deleteOrphanedEntities.test.ts @@ -69,6 +69,7 @@ describe('deleteOrphanedEntities', () => { await knex('final_entities').insert({ entity_id: `id-${ref}`, hash: 'original', + entity_ref: ref, stitch_ticket: '', }); } diff --git a/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts b/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts index 4f302308ae..5e3f3e0b17 100644 --- a/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts +++ b/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts @@ -204,6 +204,7 @@ describe('DefaultLocationStore', () => { hash: 'hash', last_updated_at: new Date(), stitch_ticket: '', + entity_ref: 'k:ns/n', }); await knex('search').insert({ diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts index 5b369bfa41..1e795e9eb0 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts @@ -114,6 +114,7 @@ describe('DefaultEntitiesCatalog', () => { await knex('final_entities').insert({ entity_id: id, + entity_ref: entityRef, final_entity: entityJson, hash: 'h', stitch_ticket: '', diff --git a/plugins/catalog-backend/src/tests/migrations.test.ts b/plugins/catalog-backend/src/tests/migrations.test.ts index 69e347bad0..5e92eda614 100644 --- a/plugins/catalog-backend/src/tests/migrations.test.ts +++ b/plugins/catalog-backend/src/tests/migrations.test.ts @@ -93,6 +93,7 @@ describe('migrations', () => { hash: 'h', stitch_ticket: '', final_entity: '{}', + entity_ref: 'k:ns/n1', }) .into('final_entities'); @@ -418,7 +419,7 @@ describe('migrations', () => { }) .into('refresh_state'); - // Insert a simple URL before the migration + // Insert a simple entity before the migration await knex .insert({ entity_id: '8222246a-b572-49cf-a702-1a0fcfaae901', @@ -428,11 +429,15 @@ describe('migrations', () => { }) .into('final_entities'); + // verify that the entity_ref column is not present + const preColumnInfo = await knex('final_entities').columnInfo(); + expect(preColumnInfo.entity_ref).toBeUndefined(); + await migrateUpOnce(knex); // verify that the entity_ref column has been added - const columnInfo = await knex('final_entities').columnInfo(); - expect(columnInfo.entity_ref).not.toBeUndefined(); + const afterColumnInfo = await knex('final_entities').columnInfo(); + expect(afterColumnInfo.entity_ref).not.toBeUndefined(); // verify that the contents of the entity_ref column are correct await expect(knex('final_entities')).resolves.toEqual( From 3897fc920a56bd4a4ea6537fc35069a713a9fb17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 11 Nov 2024 13:19:43 +0100 Subject: [PATCH 9/9] review updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- ...104700_add_entity_ref_to_final_entities.js | 14 ++++---- .../operations/stitcher/performStitching.ts | 2 +- .../src/service/DefaultEntitiesCatalog.ts | 1 - .../src/tests/migrations.test.ts | 35 ++++++++++++++++--- 4 files changed, 38 insertions(+), 14 deletions(-) diff --git a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js index 962ea05e22..ef81dbdfe9 100644 --- a/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js +++ b/plugins/catalog-backend/migrations/20241024104700_add_entity_ref_to_final_entities.js @@ -26,8 +26,6 @@ exports.up = async function up(knex) { .string('entity_ref') .nullable() .comment('The entity reference of the entity'); - - table.index('entity_ref', 'entity_ref_idx'); }); await knex @@ -39,12 +37,12 @@ exports.up = async function up(knex) { }) .table('final_entities as f'); - // SQLite does not support ALTER COLUMN. - if (!knex.client.config.client.includes('sqlite3')) { - await knex.schema.alterTable('final_entities', table => { - table.string('entity_ref').notNullable().alter({ alterNullable: true }); + await knex.schema.alterTable('final_entities', table => { + table.string('entity_ref').notNullable().alter({ alterNullable: true }); + table.unique(['entity_ref'], { + indexName: 'final_entities_entity_ref_uniq', }); - } + }); }; /** @@ -53,7 +51,7 @@ exports.up = async function up(knex) { */ exports.down = async function down(knex) { await knex.schema.alterTable('final_entities', table => { - table.dropIndex([], 'entity_ref_idx'); + table.dropUnique([], 'final_entities_entity_ref_uniq'); table.dropColumn('entity_ref'); }); }; diff --git a/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts b/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts index 5681ec3aa0..198de7209d 100644 --- a/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts +++ b/plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts @@ -218,7 +218,7 @@ export async function performStitching(options: { .where('entity_id', entityId) .where('stitch_ticket', stitchTicket) .onConflict('entity_id') - .merge(['final_entity', 'hash', 'last_updated_at', 'entity_ref']); + .merge(['final_entity', 'hash', 'last_updated_at']); const markDeferred = async () => { if (options.strategy.mode !== 'deferred') { diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts index 830abd16ab..d1cabbf575 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts @@ -684,7 +684,6 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { const parentRows = await this.database( 'refresh_state_references', ) - .innerJoin('final_entities', { 'refresh_state_references.source_entity_ref': 'final_entities.entity_ref', diff --git a/plugins/catalog-backend/src/tests/migrations.test.ts b/plugins/catalog-backend/src/tests/migrations.test.ts index 5e92eda614..8ee228dd5c 100644 --- a/plugins/catalog-backend/src/tests/migrations.test.ts +++ b/plugins/catalog-backend/src/tests/migrations.test.ts @@ -409,7 +409,7 @@ describe('migrations', () => { await knex .insert({ - entity_id: '8222246a-b572-49cf-a702-1a0fcfaae901', + entity_id: 'id1', entity_ref: 'k:ns/n', unprocessed_entity: '{}', processed_entity: '{}', @@ -418,11 +418,22 @@ describe('migrations', () => { last_discovery_at: knex.fn.now(), }) .into('refresh_state'); + await knex + .insert({ + entity_id: 'id2', + entity_ref: 'k:ns/n2', + unprocessed_entity: '{}', + processed_entity: '{}', + errors: '[]', + next_update_at: knex.fn.now(), + last_discovery_at: knex.fn.now(), + }) + .into('refresh_state'); // Insert a simple entity before the migration await knex .insert({ - entity_id: '8222246a-b572-49cf-a702-1a0fcfaae901', + entity_id: 'id1', hash: '3f5a4d6ba8507be297bb7cd87c4b55b63e3f4c14', stitch_ticket: '52367ed7-120b-405f-b7e0-cdd90f956312', final_entity: '{}', @@ -443,7 +454,7 @@ describe('migrations', () => { await expect(knex('final_entities')).resolves.toEqual( expect.arrayContaining([ { - entity_id: '8222246a-b572-49cf-a702-1a0fcfaae901', + entity_id: 'id1', hash: '3f5a4d6ba8507be297bb7cd87c4b55b63e3f4c14', stitch_ticket: '52367ed7-120b-405f-b7e0-cdd90f956312', final_entity: '{}', @@ -453,6 +464,22 @@ describe('migrations', () => { ]), ); + // Verify that duplicates of entity_ref are not allowed. We lie about the + // ref to make it easier to trigger the problem. Also using a weak + // expectation due to sqlite flakiness; it rejects, but not necessarily + // with a valid error. + await expect( + knex + .insert({ + entity_id: 'id2', + entity_ref: 'k:ns/n', + hash: 'other', + stitch_ticket: 'other', + final_entity: '{}', + }) + .into('final_entities'), + ).rejects.toEqual(expect.anything()); + await migrateDownOnce(knex); // verify that the entity_ref column has been removed @@ -463,7 +490,7 @@ describe('migrations', () => { await expect(knex('final_entities')).resolves.toEqual( expect.arrayContaining([ { - entity_id: '8222246a-b572-49cf-a702-1a0fcfaae901', + entity_id: 'id1', hash: '3f5a4d6ba8507be297bb7cd87c4b55b63e3f4c14', stitch_ticket: '52367ed7-120b-405f-b7e0-cdd90f956312', final_entity: '{}',