Add conflict check on updateLocation and authorization tests

Prevent updating a location to a {type, target} already used by another
location, matching the invariant enforced by createLocation. Also adds
ALLOW/DENY tests for updateLocation in AuthorizedLocationService.

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

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

@@ -287,6 +287,29 @@ describe('DefaultLocationStore', () => {
       },
     );
 
+    it.each(databases.eachSupportedId())(
+      'throws ConflictError when updating to a type+target already used by another location, %p',
+      async databaseId => {
+        const { store } = await createLocationStore(databaseId);
+
+        await store.createLocation({
+          type: 'url',
+          target: 'https://example.com/a',
+        });
+        const b = await store.createLocation({
+          type: 'url',
+          target: 'https://example.com/b',
+        });
+
+        await expect(() =>
+          store.updateLocation(b.id, {
+            type: 'url',
+            target: 'https://example.com/a',
+          }),
+        ).rejects.toThrow(/already exists/);
+      },
+    );
+
     it.each(databases.eachSupportedId())(
       'updates type and target and issues a delta mutation with the new entity, %p',
       async databaseId => {

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

@@ -218,18 +218,42 @@ export class DefaultLocationStore implements LocationStore, EntityProvider {
     if (this.db.client.config.client.includes('mysql')) {
       await this.db.transaction(async tx => {
         [row] = await tx<DbLocationsRow>('locations').where({ id }).select();
-        if (row) {
-          await tx<DbLocationsRow>('locations')
-            .where({ id })
-            .update({ type: location.type, target: location.target });
-          row = { ...row, type: location.type, target: location.target };
+        if (!row) {
+          return;
         }
+
+        const [conflict] = await tx<DbLocationsRow>('locations')
+          .where({ type: location.type, target: location.target })
+          .whereNot({ id })
+          .select();
+        if (conflict) {
+          throw new ConflictError(
+            `Location ${location.type}:${location.target} already exists`,
+          );
+        }
+
+        await tx<DbLocationsRow>('locations')
+          .where({ id })
+          .update({ type: location.type, target: location.target });
+        row = { ...row, type: location.type, target: location.target };
       });
     } else {
-      [row] = await this.db<DbLocationsRow>('locations')
-        .where({ id })
-        .update({ type: location.type, target: location.target })
-        .returning('*');
+      await this.db.transaction(async tx => {
+        const [conflict] = await tx<DbLocationsRow>('locations')
+          .where({ type: location.type, target: location.target })
+          .whereNot({ id })
+          .select();
+        if (conflict) {
+          throw new ConflictError(
+            `Location ${location.type}:${location.target} already exists`,
+          );
+        }
+
+        [row] = await tx<DbLocationsRow>('locations')
+          .where({ id })
+          .update({ type: location.type, target: location.target })
+          .returning('*');
+      });
     }
 
     if (!row) {

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

@@ -125,6 +125,35 @@ describe('AuthorizedLocationService', () => {
     });
   });
 
+  describe('updateLocation', () => {
+    it('calls underlying service to update location on ALLOW', async () => {
+      mockAllow();
+      const service = createService();
+
+      const spec = { type: 'type', target: 'target' };
+      await service.updateLocation('id', spec, mockOptions);
+
+      expect(fakeLocationService.updateLocation).toHaveBeenCalledWith(
+        'id',
+        spec,
+        mockOptions,
+      );
+    });
+
+    it('throws NotAllowedError on DENY', async () => {
+      mockDeny();
+      const service = createService();
+
+      await expect(() =>
+        service.updateLocation(
+          'id',
+          { type: 'type', target: 'target' },
+          mockOptions,
+        ),
+      ).rejects.toThrow(NotAllowedError);
+    });
+  });
+
   describe('deleteLocation', () => {
     it('calls underlying service to delete location on ALLOW', async () => {
       mockAllow();