From 8be34b3e1394322ad791c359fa45498498a1d8de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Tue, 7 Apr 2026 16:59:23 +0200 Subject: [PATCH] Simplify updateLocation: single UPDATE RETURNING + add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Made-with: Cursor --- .../providers/DefaultLocationStore.test.ts | 54 ++++++++++++++++ .../src/providers/DefaultLocationStore.ts | 63 +++++++++---------- .../src/service/createRouter.test.ts | 1 - 3 files changed, 83 insertions(+), 35 deletions(-) diff --git a/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts b/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts index 01a974c7ef..5ef242aa11 100644 --- a/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts +++ b/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', diff --git a/plugins/catalog-backend/src/providers/DefaultLocationStore.ts b/plugins/catalog-backend/src/providers/DefaultLocationStore.ts index 6a5f829f22..808d3cf9a7 100644 --- a/plugins/catalog-backend/src/providers/DefaultLocationStore.ts +++ b/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('locations') - .where({ id }) - .select(); - - if (!old) { - throw new NotFoundError(`Found no location with ID ${id}`); - } - - await tx('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('locations') + .where({ id }) + .update({ type: location.type, target: location.target }); + if (Number(count) > 0) { + [row] = await tx('locations').where({ id }).select(); + } }); - - const [row] = await tx('locations') + } else { + [row] = await this.db('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, }; } diff --git a/plugins/catalog-backend/src/service/createRouter.test.ts b/plugins/catalog-backend/src/service/createRouter.test.ts index 289e75585c..c196161a40 100644 --- a/plugins/catalog-backend/src/service/createRouter.test.ts +++ b/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';