From 1b3aa9cd512a01a44ffe807ac509c456310c7750 Mon Sep 17 00:00:00 2001 From: Jordan Brock <115588091+jorbrock@users.noreply.github.com> Date: Mon, 24 Aug 2026 08:57:32 -0700 Subject: [PATCH] fix(server): correct tag create operations (#30877) --- open-api/immich-openapi-specs.json | 1 + server/src/controllers/tag.controller.spec.ts | 35 +++ server/src/dtos/tag.dto.ts | 5 +- server/src/queries/tag.repository.sql | 83 +++++-- server/src/repositories/tag.repository.ts | 62 ++--- .../specs/repositories/tag.repository.spec.ts | 214 ++++++++++++++++++ 6 files changed, 347 insertions(+), 53 deletions(-) diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index ce0566524..8d128914f 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -28123,6 +28123,7 @@ }, "name": { "description": "Tag name", + "pattern": "^[^/]*$", "type": "string" }, "parentId": { diff --git a/server/src/controllers/tag.controller.spec.ts b/server/src/controllers/tag.controller.spec.ts index 432ef78f1..64d62ed52 100644 --- a/server/src/controllers/tag.controller.spec.ts +++ b/server/src/controllers/tag.controller.spec.ts @@ -24,6 +24,41 @@ describe(TagController.name, () => { await request(ctx.getHttpServer()).post(`/tags`).send({ name: 'tag', parentId: null }); expect(service.create).toHaveBeenCalledWith(undefined, expect.objectContaining({ parentId: null })); }); + it('should require a valid parentId', async () => { + const { status, body } = await request(ctx.getHttpServer()) + .post(`/tags`) + .send({ name: 'tag', parentId: 'invalid-id' }); + expect(status).toBe(400); + expect(body).toEqual(errorDto.validationError([{ path: ['parentId'], message: 'Invalid UUID' }])); + }); + it('should throw an error if a slash is in tag name', async () => { + const { status, body } = await request(ctx.getHttpServer()) + .post(`/tags`) + .send({ name: 'tagA/tagB', parentId: factory.uuid() }); + expect(status).toBe(400); + expect(body).toEqual( + errorDto.validationError([{ path: ['name'], message: 'Tag name cannot contain slash characters ("/")' }]), + ); + }); + it('should accept a null color', async () => { + const { status } = await request(ctx.getHttpServer()).post(`/tags`).send({ name: 'tagA', color: null }); + expect(status).toBe(201); + }); + it('should require a valid color', async () => { + const { status, body } = await request(ctx.getHttpServer()) + .post(`/tags`) + .send({ name: 'tagA', color: 'invalid-color' }); + expect(status).toBe(400); + expect(body).toEqual( + errorDto.validationError([ + { + path: ['color'], + message: + 'Invalid string: must match pattern /^#?([0-9A-Fa-f]{3}|[0-9A-Fa-f]{4}|[0-9A-Fa-f]{6}|[0-9A-Fa-f]{8})$/', + }, + ]), + ); + }); }); describe('GET /tags/:id', () => { diff --git a/server/src/dtos/tag.dto.ts b/server/src/dtos/tag.dto.ts index 1c376b9c3..7f6dfbf20 100644 --- a/server/src/dtos/tag.dto.ts +++ b/server/src/dtos/tag.dto.ts @@ -7,7 +7,10 @@ import z from 'zod'; const TagCreateSchema = z .object({ - name: z.string().describe('Tag name'), + name: z + .string() + .regex(/^[^/]*$/, `Tag name cannot contain slash characters ("/")`) + .describe('Tag name'), parentId: z.uuidv4().nullish().describe('Parent tag ID'), color: hexColor.nullable().optional().describe('Tag color (hex)'), }) diff --git a/server/src/queries/tag.repository.sql b/server/src/queries/tag.repository.sql index 317298bea..0670b5ab1 100644 --- a/server/src/queries/tag.repository.sql +++ b/server/src/queries/tag.repository.sql @@ -28,22 +28,39 @@ where and "value" = $2 -- TagRepository.upsertValue -begin -insert into - "tag" ("userId", "value", "parentId") -values - ($1, $2, $3) -on conflict ("userId", "value") do update -set - "parentId" = $4 -returning - "tag"."id", - "tag"."value", - "tag"."createdAt", - "tag"."updatedAt", - "tag"."color", - "tag"."parentId" -rollback +with + "created_tag" as ( + insert into + "tag" ("userId", "value", "parentId") + values + ($1, $2, $3) + on conflict ("userId", "value") do update + set + "parentId" = $4 + returning + * + ), + "created_tag_closures" as ( + insert into + "tag_closure" ("id_ancestor", "id_descendant") + select + "created_tag"."id" as "id_ancestor", + "created_tag"."id" as "id_descendant" + from + "created_tag" + union all + select + "tag_closure"."id_ancestor", + "created_tag"."id" as "id_descendant" + from + "created_tag" + inner join "tag_closure" on "tag_closure"."id_descendant" = "created_tag"."parentId" + on conflict do nothing + ) +select + * +from + "created_tag" -- TagRepository.getAll select @@ -61,12 +78,36 @@ order by "value" -- TagRepository.create -insert into - "tag" ("userId", "color", "value") -values - ($1, $2, $3) -returning +with + "created_tag" as ( + insert into + "tag" ("userId", "color", "value") + values + ($1, $2, $3) + returning + * + ), + "created_tag_closures" as ( + insert into + "tag_closure" ("id_ancestor", "id_descendant") + select + "created_tag"."id" as "id_ancestor", + "created_tag"."id" as "id_descendant" + from + "created_tag" + union all + select + "tag_closure"."id_ancestor", + "created_tag"."id" as "id_descendant" + from + "created_tag" + inner join "tag_closure" on "tag_closure"."id_descendant" = "created_tag"."parentId" + on conflict do nothing + ) +select * +from + "created_tag" -- TagRepository.update begin diff --git a/server/src/repositories/tag.repository.ts b/server/src/repositories/tag.repository.ts index c101f6b68..2ea264a97 100644 --- a/server/src/repositories/tag.repository.ts +++ b/server/src/repositories/tag.repository.ts @@ -1,5 +1,5 @@ import { Injectable } from '@nestjs/common'; -import { Insertable, Kysely, sql, Updateable } from 'kysely'; +import { Insertable, InsertQueryBuilder, Kysely, QueryCreator, Selectable, Updateable } from 'kysely'; import { InjectKysely } from 'nestjs-kysely'; import { columns } from 'src/database'; import { Chunked, ChunkedSet, DummyValue, GenerateSql } from 'src/decorators'; @@ -7,7 +7,6 @@ import { LoggingRepository } from 'src/repositories/logging.repository'; import { DB } from 'src/schema'; import { TagAssetTable } from 'src/schema/tables/tag-asset.table'; import { TagTable } from 'src/schema/tables/tag.table'; - @Injectable() export class TagRepository { constructor( @@ -35,37 +34,13 @@ export class TagRepository { @GenerateSql({ params: [{ userId: DummyValue.UUID, value: DummyValue.STRING, parentId: DummyValue.UUID }] }) async upsertValue({ userId, value, parentId: _parentId }: { userId: string; value: string; parentId?: string }) { const parentId = _parentId ?? null; - return this.db.transaction().execute(async (tx) => { - const tag = await this.db + return this.insertTagWithClosures((db) => + db .insertInto('tag') .values({ userId, value, parentId }) .onConflict((oc) => oc.columns(['userId', 'value']).doUpdateSet({ parentId })) - .returning(columns.tag) - .executeTakeFirstOrThrow(); - - // update closure table - await tx - .insertInto('tag_closure') - .values({ id_ancestor: tag.id, id_descendant: tag.id }) - .onConflict((oc) => oc.doNothing()) - .execute(); - - if (parentId) { - await tx - .insertInto('tag_closure') - .columns(['id_ancestor', 'id_descendant']) - .expression( - this.db - .selectFrom('tag_closure') - .select(['id_ancestor', sql.raw(`'${tag.id}'`).as('id_descendant')]) - .where('id_descendant', '=', parentId), - ) - .onConflict((oc) => oc.doNothing()) - .execute(); - } - - return tag; - }); + .returningAll(), + ); } @GenerateSql({ params: [DummyValue.UUID] }) @@ -75,7 +50,7 @@ export class TagRepository { @GenerateSql({ params: [{ userId: DummyValue.UUID, color: DummyValue.STRING, value: DummyValue.STRING }] }) create(tag: Insertable) { - return this.db.insertInto('tag').values(tag).returningAll().executeTakeFirstOrThrow(); + return this.insertTagWithClosures((db) => db.insertInto('tag').values(tag).returningAll()); } @GenerateSql({ params: [DummyValue.UUID, { value: DummyValue.STRING, color: DummyValue.STRING }] }) @@ -240,4 +215,29 @@ export class TagRepository { this.logger.log(`Deleted ${deletedRows} empty tags`); } } + + insertTagWithClosures(insertTag: (db: QueryCreator) => InsertQueryBuilder>) { + return this.db + .with('created_tag', insertTag) + .with('created_tag_closures', (db) => + db + .insertInto('tag_closure') + .columns(['id_ancestor', 'id_descendant']) + .expression((eb) => + eb + .selectFrom('created_tag') + .select(['created_tag.id as id_ancestor', 'created_tag.id as id_descendant']) + .unionAll( + eb + .selectFrom('created_tag') + .innerJoin('tag_closure', 'tag_closure.id_descendant', 'created_tag.parentId') + .select(['tag_closure.id_ancestor', 'created_tag.id as id_descendant']), + ), + ) + .onConflict((oc) => oc.doNothing()), + ) + .selectFrom('created_tag') + .selectAll() + .executeTakeFirstOrThrow(); + } } diff --git a/server/test/medium/specs/repositories/tag.repository.spec.ts b/server/test/medium/specs/repositories/tag.repository.spec.ts index 24a89961e..3f062b702 100644 --- a/server/test/medium/specs/repositories/tag.repository.spec.ts +++ b/server/test/medium/specs/repositories/tag.repository.spec.ts @@ -29,6 +29,86 @@ describe(TagRepository.name, () => { await ctx.database.deleteFrom('tag').execute(); }); + describe('create', () => { + it('should create a top-level tag with proper closures', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + + const result = await sut.create({ + userId: user.id, + value: 'tagA', + color: '#000000', + }); + + await expect( + ctx.database + .selectFrom('tag') + .select(['userId', 'value', 'color', 'parentId']) + .where('id', '=', result.id) + .executeTakeFirstOrThrow(), + ).resolves.toEqual({ userId: user.id, value: 'tagA', color: '#000000', parentId: null }); + + await expect( + ctx.database + .selectFrom('tag_closure') + .select(['id_ancestor', 'id_descendant']) + .where('id_ancestor', '=', result.id) + .execute(), + ).resolves.toEqual([{ id_ancestor: result.id, id_descendant: result.id }]); + }); + it('should create a child tag with proper closures', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + + const resultParent = await sut.create({ + userId: user.id, + value: 'tagA', + color: '#000000', + }); + const resultChild = await sut.create({ + userId: user.id, + value: 'tagA/tagB', + color: '#00FF00', + parentId: resultParent.id, + }); + + await expect( + ctx.database + .selectFrom('tag') + .select(['userId', 'value', 'color', 'parentId']) + .where('id', '=', resultParent.id) + .executeTakeFirstOrThrow(), + ).resolves.toEqual({ userId: user.id, value: 'tagA', color: '#000000', parentId: null }); + + await expect( + ctx.database + .selectFrom('tag') + .select(['userId', 'value', 'color', 'parentId']) + .where('id', '=', resultChild.id) + .executeTakeFirstOrThrow(), + ).resolves.toEqual({ userId: user.id, value: 'tagA/tagB', color: '#00FF00', parentId: resultParent.id }); + + await expect( + ctx.database + .selectFrom('tag_closure') + .select(['id_ancestor', 'id_descendant']) + .where('id_ancestor', '=', resultParent.id) + .execute(), + ).resolves.toEqual([ + { id_ancestor: resultParent.id, id_descendant: resultParent.id }, + { id_ancestor: resultParent.id, id_descendant: resultChild.id }, + ]); + + await expect( + ctx.database + .selectFrom('tag_closure') + .select(['id_ancestor', 'id_descendant']) + .where('id_ancestor', '=', resultChild.id) + .execute(), + ).resolves.toEqual([{ id_ancestor: resultChild.id, id_descendant: resultChild.id }]); + }); + }); + describe('update', () => { it('should update a tag color', async () => { const { ctx, sut } = setup(); @@ -71,4 +151,138 @@ describe(TagRepository.name, () => { ).resolves.toEqual({ userId: user.id, value: 'updatedTagA', color: '#000000', parentId: null }); }); }); + + describe('delete', () => { + it('should delete top-level tag without descendants', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + + const { tag } = await ctx.newTag({ + userId: user.id, + value: 'tagA', + color: '#000000', + }); + + await expect( + ctx.database + .selectFrom('tag') + .select(['userId', 'value', 'color', 'parentId']) + .where('id', '=', tag.id) + .executeTakeFirst(), + ).resolves.toEqual({ userId: user.id, value: 'tagA', color: '#000000', parentId: null }); + + await sut.delete(tag.id); + + await expect( + ctx.database.selectFrom('tag').selectAll().where('id', '=', tag.id).executeTakeFirst(), + ).resolves.toBeUndefined(); + }); + + it('should delete a parent tag and all its descendants', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + + const { tag: tag1 } = await ctx.newTag({ + userId: user.id, + value: 'tagA', + color: '#000000', + }); + + const { tag: tag2 } = await ctx.newTag({ + userId: user.id, + value: 'tagA/tagB', + color: '#00FF00', + parentId: tag1.id, + }); + + const { tag: tag3 } = await ctx.newTag({ + userId: user.id, + value: 'tagA/tagB/tagC', + color: '#0000FF', + parentId: tag2.id, + }); + + const { tag: tag4 } = await ctx.newTag({ + userId: user.id, + value: 'tagA/tagD', + color: '#FFFF00', + parentId: tag1.id, + }); + + const { tag: tag5 } = await ctx.newTag({ + userId: user.id, + value: 'tagA/tagD/tagE', + color: '#FF00FF', + parentId: tag4.id, + }); + await expect( + ctx.database + .selectFrom('tag') + .select(['userId', 'value', 'color', 'parentId']) + .where('id', 'in', [tag1.id, tag2.id, tag3.id, tag4.id, tag5.id]) + .execute(), + ).resolves.toEqual( + expect.arrayContaining([ + { userId: user.id, value: 'tagA', color: '#000000', parentId: null }, + { userId: user.id, value: 'tagA/tagB', color: '#00FF00', parentId: tag1.id }, + { userId: user.id, value: 'tagA/tagB/tagC', color: '#0000FF', parentId: tag2.id }, + { userId: user.id, value: 'tagA/tagD', color: '#FFFF00', parentId: tag1.id }, + { userId: user.id, value: 'tagA/tagD/tagE', color: '#FF00FF', parentId: tag4.id }, + ]), + ); + + await sut.delete(tag1.id); + + await expect( + ctx.database + .selectFrom('tag') + .where('id', 'in', [tag1.id, tag2.id, tag3.id, tag4.id, tag5.id]) + .executeTakeFirst(), + ).resolves.toBeUndefined(); + }); + + it('should not delete tags outside the hierarchy', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + + const { tag: tag1 } = await ctx.newTag({ + userId: user.id, + value: 'tagA', + color: '#000000', + }); + + const { tag: tag2 } = await ctx.newTag({ + userId: user.id, + value: 'tagA/tagB', + color: '#00FF00', + parentId: tag1.id, + }); + + const { tag: tag3 } = await ctx.newTag({ + userId: user.id, + value: 'tagC', + color: '#0000FF', + }); + + await expect( + ctx.database + .selectFrom('tag') + .select(['userId', 'value', 'color', 'parentId']) + .where('id', 'in', [tag1.id, tag2.id, tag3.id]) + .execute(), + ).resolves.toEqual( + expect.arrayContaining([ + { userId: user.id, value: 'tagA', color: '#000000', parentId: null }, + { userId: user.id, value: 'tagA/tagB', color: '#00FF00', parentId: tag1.id }, + { userId: user.id, value: 'tagC', color: '#0000FF', parentId: null }, + ]), + ); + + await sut.delete(tag1.id); + + await expect( + ctx.database.selectFrom('tag').select(['userId', 'value', 'color', 'parentId']).execute(), + ).resolves.toEqual([{ userId: user.id, value: 'tagC', color: '#0000FF', parentId: null }]); + }); + }); });