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] 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: '{}',