fix(server): tag upsert (#12141)

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),

open-api/immich-openapi-specs.json

@@ -12024,6 +12024,9 @@
           "name": {
             "type": "string"
           },
+          "parentId": {
+            "type": "string"
+          },
           "updatedAt": {
             "format": "date-time",
             "type": "string"

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;
 };

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,

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;
 

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<TagEntity[]>;
   getByValue(userId: string, value: string): Promise<TagEntity | null>;
+  upsertValue(request: { userId: string; value: string; parent?: TagEntity }): Promise<TagEntity>;
 
   create(tag: Partial<TagEntity>): Promise<TagEntity>;
   get(id: string): Promise<TagEntity | null>;

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<void> {
+        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<void> {
+        await queryRunner.query(`ALTER TABLE "tags" DROP CONSTRAINT "UQ_79d6f16e52bb2c7130375246793"`);
+        await queryRunner.query(`ALTER TABLE "tags" ADD CONSTRAINT "UQ_d090e09fe86ebe2ec0aec27b451" UNIQUE ("value")`);
+    }
+
+}

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",

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<TagEntity> {
+    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<TagEntity[]> {
     const tags = await this.repository.find({
       where: { userId },

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.upsertValue.mockResolvedValue(tagStub.parent);
-      tagMock.create.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.upsertValue.mockResolvedValueOnce(tagStub.parent);
-      tagMock.create.mockResolvedValueOnce(tagStub.parent);
+      tagMock.upsertValue.mockResolvedValueOnce(tagStub.child);
-      tagMock.create.mockResolvedValueOnce(tagStub.child);
 
       await sut.handleMetadataExtraction({ id: assetStub.image.id });
 
-      expect(tagMock.create).toHaveBeenNthCalledWith(1, { userId: 'user-id', value: 'Parent', parent: undefined });
+      expect(tagMock.upsertValue).toHaveBeenNthCalledWith(1, { userId: 'user-id', value: 'Parent', parent: undefined });
-      expect(tagMock.create).toHaveBeenNthCalledWith(2, {
+      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.upsertValue.mockResolvedValue(tagStub.parent);
-      tagMock.create.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.upsertValue.mockResolvedValue(tagStub.parent);
-      tagMock.create.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.upsertValue.mockResolvedValue(tagStub.parent);
-      tagMock.create.mockResolvedValue(tagStub.parent);
 
       await sut.handleMetadataExtraction({ id: assetStub.image.id });
 
-      expect(tagMock.create).toHaveBeenNthCalledWith(1, { userId: 'user-id', value: 'Parent', parent: undefined });
+      expect(tagMock.upsertValue).toHaveBeenNthCalledWith(1, { userId: 'user-id', value: 'Parent', parent: undefined });
-      expect(tagMock.create).toHaveBeenNthCalledWith(2, {
+      expect(tagMock.upsertValue).toHaveBeenNthCalledWith(2, {
         userId: 'user-id',
         value: 'Parent/Child',
         parent: tagStub.parent,

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.upsertValue.mockResolvedValueOnce(tagStub.parent);
-      tagMock.create.mockResolvedValueOnce(tagStub.child);
+      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' }),

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);
+      parent = await repository.upsertValue({ userId, value, parent });
-      if (!tag) {
-        tag = await repository.create({ userId, value, parent });
-      }
-
-      parent = tag;
     }
 
     if (parent) {

server/test/repositories/tag.repository.mock.ts

@@ -5,6 +5,7 @@ export const newTagRepositoryMock = (): Mocked<ITagRepository> => {
   return {
     getAll: vitest.fn(),
     getByValue: vitest.fn(),
+    upsertValue: vitest.fn(),
     upsertAssetTags: vitest.fn(),
 
     get: vitest.fn(),