Simplify updateLocation: single UPDATE RETURNING + add tests

Use UPDATE...RETURNING for Postgres/SQLite and UPDATE+count-check+SELECT
(in a transaction) for MySQL. Removes the old read-before-write approach.
Also adds test coverage for updateLocation across all database engines.

Signed-off-by: Fredrik Adelöw <freben@spotify.com>
Made-with: Cursor

plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts

@@ -272,6 +272,60 @@ describe('DefaultLocationStore', () => {
     );
   });
 
+  describe('updateLocation', () => {
+    it.each(databases.eachSupportedId())(
+      'throws if the location does not exist, %p',
+      async databaseId => {
+        const { store } = await createLocationStore(databaseId);
+        const id = uuid();
+        await expect(() =>
+          store.updateLocation(id, {
+            type: 'url',
+            target: 'https://example.com',
+          }),
+        ).rejects.toThrow(new RegExp(`Found no location with ID ${id}`));
+      },
+    );
+
+    it.each(databases.eachSupportedId())(
+      'updates type and target and issues a delta mutation with the new entity, %p',
+      async databaseId => {
+        const { store, connection } = await createLocationStore(databaseId);
+
+        const created = await store.createLocation({
+          type: 'url',
+          target: 'https://example.com/old',
+        });
+
+        jest.clearAllMocks();
+
+        const updated = await store.updateLocation(created.id, {
+          type: 'url',
+          target: 'https://example.com/new',
+        });
+
+        expect(updated.id).toBe(created.id);
+        expect(updated.type).toBe('url');
+        expect(updated.target).toBe('https://example.com/new');
+        // entityRef (location_entity_ref) is stable across updates
+        expect(updated.entityRef).toBe(created.entityRef);
+
+        expect(connection.applyMutation).toHaveBeenCalledWith({
+          type: 'delta',
+          removed: [],
+          added: [
+            {
+              entity: expect.objectContaining({
+                spec: { type: 'url', target: 'https://example.com/new' },
+              }),
+              locationKey: 'url:https://example.com/new',
+            },
+          ],
+        });
+      },
+    );
+  });
+
   describe('getLocationByEntity', () => {
     it.each(databases.eachSupportedId())(
       'loads correctly, %p',

plugins/catalog-backend/src/providers/DefaultLocationStore.ts

@@ -210,50 +210,45 @@ export class DefaultLocationStore implements LocationStore, EntityProvider {
       throw new Error('location store is not initialized');
     }
 
-    const updated = await this.db.transaction(async tx => {
-      const [old] = await tx<DbLocationsRow>('locations')
-        .where({ id })
-        .select();
-
-      if (!old) {
-        throw new NotFoundError(`Found no location with ID ${id}`);
-      }
-
-      await tx<DbLocationsRow>('locations').where({ id }).update({
-        type: location.type,
-        target: location.target,
+    // MySQL doesn't support UPDATE ... RETURNING, so we fall back to checking
+    // the affected row count and then doing a separate SELECT.
+    let row: DbLocationsRow | undefined;
+    if (this.db.client.config.client.includes('mysql')) {
+      await this.db.transaction(async tx => {
+        const count = await tx<DbLocationsRow>('locations')
+          .where({ id })
+          .update({ type: location.type, target: location.target });
+        if (Number(count) > 0) {
+          [row] = await tx<DbLocationsRow>('locations').where({ id }).select();
+        }
       });
-
-      const [row] = await tx<DbLocationsRow>('locations')
+    } else {
+      [row] = await this.db<DbLocationsRow>('locations')
         .where({ id })
-        .select();
-      return { old, row };
-    });
+        .update({ type: location.type, target: location.target })
+        .returning('*');
+    }
 
-    const oldEntity = locationSpecToLocationEntity({
-      location: updated.old,
-      locationEntityRef: updated.old.location_entity_ref,
-    });
-    const newEntity = locationSpecToLocationEntity({
-      location: updated.row,
-      locationEntityRef: updated.row.location_entity_ref,
+    if (!row) {
+      throw new NotFoundError(`Found no location with ID ${id}`);
+    }
+
+    const entity = locationSpecToLocationEntity({
+      location: row,
+      locationEntityRef: row.location_entity_ref,
     });
 
     await this.connection.applyMutation({
       type: 'delta',
-      added: [
-        { entity: newEntity, locationKey: getEntityLocationRef(newEntity) },
-      ],
-      removed: [
-        { entity: oldEntity, locationKey: getEntityLocationRef(oldEntity) },
-      ],
+      added: [{ entity, locationKey: getEntityLocationRef(entity) }],
+      removed: [],
     });
 
     return {
-      id: updated.row.id,
-      type: updated.row.type,
-      target: updated.row.target,
-      entityRef: updated.row.location_entity_ref,
+      id: row.id,
+      type: row.type,
+      target: row.target,
+      entityRef: row.location_entity_ref,
     };
   }
 

plugins/catalog-backend/src/service/createRouter.test.ts

@@ -38,7 +38,6 @@ import request from 'supertest';
 import { Cursor, EntitiesCatalog } from '../catalog/types';
 import { applyDatabaseMigrations } from '../database/migrations';
 import { DbLocationsRow } from '../database/tables';
-import { computeLocationEntityRef } from '../util/conversion';
 import { CatalogProcessingOrchestrator } from '../processing/types';
 import { DefaultLocationStore } from '../providers/DefaultLocationStore';
 import { createRouter } from './createRouter';