diff --git a/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts b/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts index 5ef242aa11..b11687e33f 100644 --- a/plugins/catalog-backend/src/providers/DefaultLocationStore.test.ts +++ b/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 => { diff --git a/plugins/catalog-backend/src/providers/DefaultLocationStore.ts b/plugins/catalog-backend/src/providers/DefaultLocationStore.ts index 1bf744f1c9..519835439f 100644 --- a/plugins/catalog-backend/src/providers/DefaultLocationStore.ts +++ b/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('locations').where({ id }).select(); - if (row) { - await tx('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('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('locations') + .where({ id }) + .update({ type: location.type, target: location.target }); + row = { ...row, type: location.type, target: location.target }; }); } else { - [row] = await this.db('locations') - .where({ id }) - .update({ type: location.type, target: location.target }) - .returning('*'); + await this.db.transaction(async tx => { + const [conflict] = await tx('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('locations') + .where({ id }) + .update({ type: location.type, target: location.target }) + .returning('*'); + }); } if (!row) { diff --git a/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts b/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts index e705f88386..6c25f85f1f 100644 --- a/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts +++ b/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();