fix(server): correct tag create operations (#30877)

2026-08-24 17:57:32 +02:00
parent 6b7b0fe3a6
commit 1b3aa9cd51
6 changed files with 347 additions and 53 deletions
@@ -28123,6 +28123,7 @@
          },
          "name": {
            "description": "Tag name",
+            "pattern": "^[^/]*$",
            "type": "string"
          },
          "parentId": {
@@ -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', () => {
@@ -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)'),
  })
@@ -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
@@ -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<string>(`'${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<TagTable>) {
-    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<DB>) => InsertQueryBuilder<DB, 'tag', Selectable<TagTable>>) {
+    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();
+  }
 }
@@ -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 }]);
+    });
+  });
 });