review updates

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

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');
   });
 };

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') {

plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts

@@ -684,7 +684,6 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog {
       const parentRows = await this.database<DbRefreshStateReferencesRow>(
         'refresh_state_references',
       )
-
         .innerJoin<DbFinalEntitiesRow>('final_entities', {
           'refresh_state_references.source_entity_ref':
             'final_entities.entity_ref',

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