From 9b1a985d29fe6866b06a7c06e2ccf0746953d2d5 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Fri, 30 Aug 2024 12:44:24 -0400 Subject: [PATCH] fix(server): tag upsert (#12141) --- e2e/src/api/specs/tag.e2e-spec.ts | 58 +++++++++++++++--- .../openapi/lib/model/tag_response_dto.dart | Bin 4316 -> 4997 bytes open-api/immich-openapi-specs.json | 3 + open-api/typescript-sdk/src/fetch-client.ts | 1 + server/src/dtos/tag.dto.ts | 2 + server/src/entities/tag.entity.ts | 7 ++- server/src/interfaces/tag.interface.ts | 1 + .../1725023079109-FixTagUniqueness.ts | 16 +++++ server/src/queries/asset.repository.sql | 2 +- server/src/repositories/tag.repository.ts | 42 +++++++++++++ server/src/services/metadata.service.spec.ts | 31 ++++------ server/src/services/tag.service.spec.ts | 14 ++--- server/src/utils/tag.ts | 7 +-- .../test/repositories/tag.repository.mock.ts | 1 + 14 files changed, 145 insertions(+), 40 deletions(-) create mode 100644 server/src/migrations/1725023079109-FixTagUniqueness.ts diff --git a/e2e/src/api/specs/tag.e2e-spec.ts b/e2e/src/api/specs/tag.e2e-spec.ts index 0a26ccef0e..a4cbc99ed3 100644 --- a/e2e/src/api/specs/tag.e2e-spec.ts +++ b/e2e/src/api/specs/tag.e2e-spec.ts @@ -3,6 +3,7 @@ import { LoginResponseDto, Permission, TagCreateDto, + TagResponseDto, createTag, getAllTags, tagAssets, @@ -81,15 +82,31 @@ describe('/tags', () => { expect(status).toBe(201); }); + it('should allow multiple users to create tags with the same value', async () => { + await create(admin.accessToken, { name: 'TagA' }); + const { status, body } = await request(app) + .post('/tags') + .set('Authorization', `Bearer ${user.accessToken}`) + .send({ name: 'TagA' }); + expect(body).toEqual({ + id: expect.any(String), + name: 'TagA', + value: 'TagA', + createdAt: expect.any(String), + updatedAt: expect.any(String), + }); + expect(status).toBe(201); + }); + it('should create a nested tag', async () => { const parent = await create(admin.accessToken, { name: 'TagA' }); - const { status, body } = await request(app) .post('/tags') .set('Authorization', `Bearer ${admin.accessToken}`) .send({ name: 'TagB', parentId: parent.id }); expect(body).toEqual({ id: expect.any(String), + parentId: parent.id, name: 'TagB', value: 'TagA/TagB', createdAt: expect.any(String), @@ -134,14 +151,20 @@ describe('/tags', () => { it('should return a nested tags', async () => { await upsert(admin.accessToken, ['TagA/TagB/TagC', 'TagD']); const { status, body } = await request(app).get('/tags').set('Authorization', `Bearer ${admin.accessToken}`); + expect(body).toHaveLength(4); - expect(body).toEqual([ - expect.objectContaining({ name: 'TagA', value: 'TagA' }), - expect.objectContaining({ name: 'TagB', value: 'TagA/TagB' }), - expect.objectContaining({ name: 'TagC', value: 'TagA/TagB/TagC' }), - expect.objectContaining({ name: 'TagD', value: 'TagD' }), - ]); expect(status).toEqual(200); + + const tags = body as TagResponseDto[]; + const tagA = tags.find((tag) => tag.value === 'TagA') as TagResponseDto; + const tagB = tags.find((tag) => tag.value === 'TagA/TagB') as TagResponseDto; + const tagC = tags.find((tag) => tag.value === 'TagA/TagB/TagC') as TagResponseDto; + const tagD = tags.find((tag) => tag.value === 'TagD') as TagResponseDto; + + expect(tagA).toEqual(expect.objectContaining({ name: 'TagA', value: 'TagA' })); + expect(tagB).toEqual(expect.objectContaining({ name: 'TagB', value: 'TagA/TagB', parentId: tagA.id })); + expect(tagC).toEqual(expect.objectContaining({ name: 'TagC', value: 'TagA/TagB/TagC', parentId: tagB.id })); + expect(tagD).toEqual(expect.objectContaining({ name: 'TagD', value: 'TagD' })); }); }); @@ -167,6 +190,26 @@ describe('/tags', () => { expect(status).toBe(200); expect(body).toEqual([expect.objectContaining({ name: 'TagD', value: 'TagA/TagB/TagC/TagD' })]); }); + + it('should upsert tags in parallel without conflicts', async () => { + const [[tag1], [tag2], [tag3], [tag4]] = await Promise.all([ + upsert(admin.accessToken, ['TagA/TagB/TagC/TagD']), + upsert(admin.accessToken, ['TagA/TagB/TagC/TagD']), + upsert(admin.accessToken, ['TagA/TagB/TagC/TagD']), + upsert(admin.accessToken, ['TagA/TagB/TagC/TagD']), + ]); + + const { id, parentId, createdAt } = tag1; + for (const tag of [tag1, tag2, tag3, tag4]) { + expect(tag).toMatchObject({ + id, + parentId, + createdAt, + name: 'TagD', + value: 'TagA/TagB/TagC/TagD', + }); + } + }); }); describe('PUT /tags/assets', () => { @@ -296,6 +339,7 @@ describe('/tags', () => { expect(status).toBe(200); expect(body).toEqual({ id: expect.any(String), + parentId: tagC.id, name: 'TagD', value: 'TagA/TagB/TagC/TagD', createdAt: expect.any(String), diff --git a/mobile/openapi/lib/model/tag_response_dto.dart b/mobile/openapi/lib/model/tag_response_dto.dart index 4f0a62a8b9669e2440a869b2275fbe0a4773c3ae..1d1a88c3cff29b9b8e814590fcb3031744ee213a 100644 GIT binary patch delta 255 zcmcbk*s8waCL?c2MrN^IL1IyAUWsSQ~-`ckN^;cDc>F*=IAcLp4l(&#A-(*Q~kO Ihid{G07ZXSPXGV_ delta 40 ycmV+@0N4M8C)^>h*#Wcc0p9_$Jp}~;lVk?;lZpqJvo;860ka+oqyn@542A|#Ne_Ym diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 50bd57b527..97a31ead26 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -12024,6 +12024,9 @@ "name": { "type": "string" }, + "parentId": { + "type": "string" + }, "updatedAt": { "format": "date-time", "type": "string" diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts index 0bd67c231e..2c336f98be 100644 --- a/open-api/typescript-sdk/src/fetch-client.ts +++ b/open-api/typescript-sdk/src/fetch-client.ts @@ -232,6 +232,7 @@ export type TagResponseDto = { createdAt: string; id: string; name: string; + parentId?: string; updatedAt: string; value: string; }; diff --git a/server/src/dtos/tag.dto.ts b/server/src/dtos/tag.dto.ts index 40c5b176ff..cff11962d7 100644 --- a/server/src/dtos/tag.dto.ts +++ b/server/src/dtos/tag.dto.ts @@ -45,6 +45,7 @@ export class TagBulkAssetsResponseDto { export class TagResponseDto { id!: string; + parentId?: string; name!: string; value!: string; createdAt!: Date; @@ -55,6 +56,7 @@ export class TagResponseDto { export function mapTag(entity: TagEntity): TagResponseDto { return { id: entity.id, + parentId: entity.parentId ?? undefined, name: entity.value.split('/').at(-1) as string, value: entity.value, createdAt: entity.createdAt, diff --git a/server/src/entities/tag.entity.ts b/server/src/entities/tag.entity.ts index 940b446aea..ebcc6853c9 100644 --- a/server/src/entities/tag.entity.ts +++ b/server/src/entities/tag.entity.ts @@ -10,16 +10,18 @@ import { Tree, TreeChildren, TreeParent, + Unique, UpdateDateColumn, } from 'typeorm'; @Entity('tags') +@Unique(['userId', 'value']) @Tree('closure-table') export class TagEntity { @PrimaryGeneratedColumn('uuid') id!: string; - @Column({ unique: true }) + @Column() value!: string; @CreateDateColumn({ type: 'timestamptz' }) @@ -31,6 +33,9 @@ export class TagEntity { @Column({ type: 'varchar', nullable: true, default: null }) color!: string | null; + @Column({ nullable: true }) + parentId?: string; + @TreeParent({ onDelete: 'CASCADE' }) parent?: TagEntity; diff --git a/server/src/interfaces/tag.interface.ts b/server/src/interfaces/tag.interface.ts index f9f3784f06..aca9c223d5 100644 --- a/server/src/interfaces/tag.interface.ts +++ b/server/src/interfaces/tag.interface.ts @@ -8,6 +8,7 @@ export type AssetTagItem = { assetId: string; tagId: string }; export interface ITagRepository extends IBulkAsset { getAll(userId: string): Promise; getByValue(userId: string, value: string): Promise; + upsertValue(request: { userId: string; value: string; parent?: TagEntity }): Promise; create(tag: Partial): Promise; get(id: string): Promise; diff --git a/server/src/migrations/1725023079109-FixTagUniqueness.ts b/server/src/migrations/1725023079109-FixTagUniqueness.ts new file mode 100644 index 0000000000..859712621c --- /dev/null +++ b/server/src/migrations/1725023079109-FixTagUniqueness.ts @@ -0,0 +1,16 @@ +import { MigrationInterface, QueryRunner } from "typeorm"; + +export class FixTagUniqueness1725023079109 implements MigrationInterface { + name = 'FixTagUniqueness1725023079109' + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(`ALTER TABLE "tags" DROP CONSTRAINT "UQ_d090e09fe86ebe2ec0aec27b451"`); + await queryRunner.query(`ALTER TABLE "tags" ADD CONSTRAINT "UQ_79d6f16e52bb2c7130375246793" UNIQUE ("userId", "value")`); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(`ALTER TABLE "tags" DROP CONSTRAINT "UQ_79d6f16e52bb2c7130375246793"`); + await queryRunner.query(`ALTER TABLE "tags" ADD CONSTRAINT "UQ_d090e09fe86ebe2ec0aec27b451" UNIQUE ("value")`); + } + +} diff --git a/server/src/queries/asset.repository.sql b/server/src/queries/asset.repository.sql index ba52f7d148..3852439936 100644 --- a/server/src/queries/asset.repository.sql +++ b/server/src/queries/asset.repository.sql @@ -188,8 +188,8 @@ SELECT "AssetEntity__AssetEntity_tags"."createdAt" AS "AssetEntity__AssetEntity_tags_createdAt", "AssetEntity__AssetEntity_tags"."updatedAt" AS "AssetEntity__AssetEntity_tags_updatedAt", "AssetEntity__AssetEntity_tags"."color" AS "AssetEntity__AssetEntity_tags_color", - "AssetEntity__AssetEntity_tags"."userId" AS "AssetEntity__AssetEntity_tags_userId", "AssetEntity__AssetEntity_tags"."parentId" AS "AssetEntity__AssetEntity_tags_parentId", + "AssetEntity__AssetEntity_tags"."userId" AS "AssetEntity__AssetEntity_tags_userId", "AssetEntity__AssetEntity_faces"."id" AS "AssetEntity__AssetEntity_faces_id", "AssetEntity__AssetEntity_faces"."assetId" AS "AssetEntity__AssetEntity_faces_assetId", "AssetEntity__AssetEntity_faces"."personId" AS "AssetEntity__AssetEntity_faces_personId", diff --git a/server/src/repositories/tag.repository.ts b/server/src/repositories/tag.repository.ts index 7699d5897a..9389aeb13b 100644 --- a/server/src/repositories/tag.repository.ts +++ b/server/src/repositories/tag.repository.ts @@ -22,6 +22,48 @@ export class TagRepository implements ITagRepository { return this.repository.findOne({ where: { userId, value } }); } + async upsertValue({ + userId, + value, + parent, + }: { + userId: string; + value: string; + parent?: TagEntity; + }): Promise { + return this.dataSource.transaction(async (manager) => { + // upsert tag + const { identifiers } = await manager.upsert( + TagEntity, + { userId, value, parentId: parent?.id }, + { conflictPaths: { userId: true, value: true } }, + ); + const id = identifiers[0]?.id; + if (!id) { + throw new Error('Failed to upsert tag'); + } + + // update closure table + await manager.query( + `INSERT INTO tags_closure (id_ancestor, id_descendant) + VALUES ($1, $1) + ON CONFLICT DO NOTHING;`, + [id], + ); + + if (parent) { + await manager.query( + `INSERT INTO tags_closure (id_ancestor, id_descendant) + SELECT id_ancestor, '${id}' as id_descendant FROM tags_closure WHERE id_descendant = $1 + ON CONFLICT DO NOTHING`, + [parent.id], + ); + } + + return manager.findOneOrFail(TagEntity, { where: { id } }); + }); + } + async getAll(userId: string): Promise { const tags = await this.repository.find({ where: { userId }, diff --git a/server/src/services/metadata.service.spec.ts b/server/src/services/metadata.service.spec.ts index cb89de184a..8f44962279 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -365,25 +365,23 @@ describe(MetadataService.name, () => { it('should extract tags from TagsList', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); metadataMock.readTags.mockResolvedValue({ TagsList: ['Parent'] }); - tagMock.getByValue.mockResolvedValue(null); - tagMock.create.mockResolvedValue(tagStub.parent); + tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); - expect(tagMock.create).toHaveBeenCalledWith({ userId: 'user-id', value: 'Parent', parent: undefined }); + expect(tagMock.upsertValue).toHaveBeenCalledWith({ userId: 'user-id', value: 'Parent', parent: undefined }); }); it('should extract hierarchy from TagsList', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); metadataMock.readTags.mockResolvedValue({ TagsList: ['Parent/Child'] }); - tagMock.getByValue.mockResolvedValue(null); - tagMock.create.mockResolvedValueOnce(tagStub.parent); - tagMock.create.mockResolvedValueOnce(tagStub.child); + tagMock.upsertValue.mockResolvedValueOnce(tagStub.parent); + tagMock.upsertValue.mockResolvedValueOnce(tagStub.child); await sut.handleMetadataExtraction({ id: assetStub.image.id }); - expect(tagMock.create).toHaveBeenNthCalledWith(1, { userId: 'user-id', value: 'Parent', parent: undefined }); - expect(tagMock.create).toHaveBeenNthCalledWith(2, { + expect(tagMock.upsertValue).toHaveBeenNthCalledWith(1, { userId: 'user-id', value: 'Parent', parent: undefined }); + expect(tagMock.upsertValue).toHaveBeenNthCalledWith(2, { userId: 'user-id', value: 'Parent/Child', parent: tagStub.parent, @@ -393,35 +391,32 @@ describe(MetadataService.name, () => { it('should extract tags from Keywords as a string', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); metadataMock.readTags.mockResolvedValue({ Keywords: 'Parent' }); - tagMock.getByValue.mockResolvedValue(null); - tagMock.create.mockResolvedValue(tagStub.parent); + tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); - expect(tagMock.create).toHaveBeenCalledWith({ userId: 'user-id', value: 'Parent', parent: undefined }); + expect(tagMock.upsertValue).toHaveBeenCalledWith({ userId: 'user-id', value: 'Parent', parent: undefined }); }); it('should extract tags from Keywords as a list', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); metadataMock.readTags.mockResolvedValue({ Keywords: ['Parent'] }); - tagMock.getByValue.mockResolvedValue(null); - tagMock.create.mockResolvedValue(tagStub.parent); + tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); - expect(tagMock.create).toHaveBeenCalledWith({ userId: 'user-id', value: 'Parent', parent: undefined }); + expect(tagMock.upsertValue).toHaveBeenCalledWith({ userId: 'user-id', value: 'Parent', parent: undefined }); }); it('should extract hierarchal tags from Keywords', async () => { assetMock.getByIds.mockResolvedValue([assetStub.image]); metadataMock.readTags.mockResolvedValue({ Keywords: 'Parent/Child' }); - tagMock.getByValue.mockResolvedValue(null); - tagMock.create.mockResolvedValue(tagStub.parent); + tagMock.upsertValue.mockResolvedValue(tagStub.parent); await sut.handleMetadataExtraction({ id: assetStub.image.id }); - expect(tagMock.create).toHaveBeenNthCalledWith(1, { userId: 'user-id', value: 'Parent', parent: undefined }); - expect(tagMock.create).toHaveBeenNthCalledWith(2, { + expect(tagMock.upsertValue).toHaveBeenNthCalledWith(1, { userId: 'user-id', value: 'Parent', parent: undefined }); + expect(tagMock.upsertValue).toHaveBeenNthCalledWith(2, { userId: 'user-id', value: 'Parent/Child', parent: tagStub.parent, diff --git a/server/src/services/tag.service.spec.ts b/server/src/services/tag.service.spec.ts index ffa7895cb4..de270777b0 100644 --- a/server/src/services/tag.service.spec.ts +++ b/server/src/services/tag.service.spec.ts @@ -115,9 +115,9 @@ describe(TagService.name, () => { describe('upsert', () => { it('should upsert a new tag', async () => { - tagMock.create.mockResolvedValue(tagStub.parent); + tagMock.upsertValue.mockResolvedValue(tagStub.parent); await expect(sut.upsert(authStub.admin, { tags: ['Parent'] })).resolves.toBeDefined(); - expect(tagMock.create).toHaveBeenCalledWith({ + expect(tagMock.upsertValue).toHaveBeenCalledWith({ value: 'Parent', userId: 'admin_id', parentId: undefined, @@ -126,15 +126,15 @@ describe(TagService.name, () => { it('should upsert a nested tag', async () => { tagMock.getByValue.mockResolvedValueOnce(null); - tagMock.create.mockResolvedValueOnce(tagStub.parent); - tagMock.create.mockResolvedValueOnce(tagStub.child); + tagMock.upsertValue.mockResolvedValueOnce(tagStub.parent); + tagMock.upsertValue.mockResolvedValueOnce(tagStub.child); await expect(sut.upsert(authStub.admin, { tags: ['Parent/Child'] })).resolves.toBeDefined(); - expect(tagMock.create).toHaveBeenNthCalledWith(1, { + expect(tagMock.upsertValue).toHaveBeenNthCalledWith(1, { value: 'Parent', userId: 'admin_id', - parentId: undefined, + parent: undefined, }); - expect(tagMock.create).toHaveBeenNthCalledWith(2, { + expect(tagMock.upsertValue).toHaveBeenNthCalledWith(2, { value: 'Parent/Child', userId: 'admin_id', parent: expect.objectContaining({ id: 'tag-parent' }), diff --git a/server/src/utils/tag.ts b/server/src/utils/tag.ts index 12c46d2440..6d6c70f1d7 100644 --- a/server/src/utils/tag.ts +++ b/server/src/utils/tag.ts @@ -13,12 +13,7 @@ export const upsertTags = async (repository: ITagRepository, { userId, tags }: U for (const part of parts) { const value = parent ? `${parent.value}/${part}` : part; - let tag = await repository.getByValue(userId, value); - if (!tag) { - tag = await repository.create({ userId, value, parent }); - } - - parent = tag; + parent = await repository.upsertValue({ userId, value, parent }); } if (parent) { diff --git a/server/test/repositories/tag.repository.mock.ts b/server/test/repositories/tag.repository.mock.ts index 35b3de1576..a3fc0e77e0 100644 --- a/server/test/repositories/tag.repository.mock.ts +++ b/server/test/repositories/tag.repository.mock.ts @@ -5,6 +5,7 @@ export const newTagRepositoryMock = (): Mocked => { return { getAll: vitest.fn(), getByValue: vitest.fn(), + upsertValue: vitest.fn(), upsertAssetTags: vitest.fn(), get: vitest.fn(),