Skip to content

Commit 6ae0402

Browse files
benjamonnguyenbenjamonnguyen
committed
add tag closure logic to tag create method; add data fix script; use tag closures to efficiently query for updated tag descendants
1 parent 24d0ee1 commit 6ae0402

File tree

7 files changed

+200
-88
lines changed

7 files changed

+200
-88
lines changed

‎open-api/immich-openapi-specs.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13765,7 +13765,7 @@
1376513765
"nullable": true,
1376613766
"type": "string"
1376713767
},
13768-
"value": {
13768+
"name": {
1376913769
"type": "string"
1377013770
}
1377113771
},

‎server/src/dtos/tag.dto.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ export class TagCreateDto {
1919
export class TagUpdateDto {
2020
@IsString()
2121
@Optional({ emptyToNull: true, nullable: true })
22-
value?: string;
22+
name?: string;
2323

2424
@Optional({ emptyToNull: true, nullable: true })
2525
@ValidateHexColor()

‎server/src/migrations/1745813980616-CreateMissingTagClosures.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
import { Kysely, sql } from 'kysely';
2+
import { columns } from 'src/database';
3+
4+
export async function up(db: Kysely<any>): Promise<void> {
5+
const tags = await db.selectFrom('tags').select(columns.tag).execute();
6+
return db.transaction().execute(async (tx) => {
7+
for (const tag of tags) {
8+
await tx
9+
.insertInto('tags_closure')
10+
.values({ id_ancestor: tag.id, id_descendant: tag.id })
11+
.onConflict((oc) => oc.doNothing())
12+
.execute();
13+
14+
if (tag.parentId) {
15+
await tx
16+
.insertInto('tags_closure')
17+
.columns(['id_ancestor', 'id_descendant'])
18+
.expression(
19+
db
20+
.selectFrom('tags_closure')
21+
.select(['id_ancestor', sql.raw<string>(`'${tag.id}'`).as('id_descendant')])
22+
.where('id_descendant', '=', tag.parentId),
23+
)
24+
.onConflict((oc) => oc.doNothing())
25+
.execute();
26+
}
27+
}
28+
});
29+
}
30+
31+
export async function down(_: Kysely<any>): Promise<void> { }

‎server/src/repositories/tag.repository.ts‎

Lines changed: 106 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { Injectable } from '@nestjs/common';
2-
import { Insertable, Kysely, Selectable, sql, Updateable } from 'kysely';
2+
import { Insertable, Kysely, Selectable, sql, Transaction, Updateable } from 'kysely';
33
import { InjectKysely } from 'nestjs-kysely';
44
import { columns } from 'src/database';
55
import { DB, TagAsset, Tags } from 'src/db';
@@ -15,11 +15,17 @@ export class TagRepository {
1515
this.logger.setContext(TagRepository.name);
1616
}
1717

18+
// #region tags
1819
@GenerateSql({ params: [DummyValue.UUID] })
19-
get(id: string) {
20+
getOne(id: string) {
2021
return this.db.selectFrom('tags').select(columns.tag).where('id', '=', id).executeTakeFirst();
2122
}
2223

24+
@GenerateSql({ params: [DummyValue.UUID] })
25+
async getMany(ids: string[]) {
26+
return await this.db.selectFrom('tags').select(columns.tag).where('id', 'in', ids).execute();
27+
}
28+
2329
@GenerateSql({ params: [DummyValue.UUID, DummyValue.STRING] })
2430
getByValue(userId: string, value: string) {
2531
return this.db
@@ -41,26 +47,7 @@ export class TagRepository {
4147
.returning(columns.tag)
4248
.executeTakeFirstOrThrow();
4349

44-
// update closure table
45-
await tx
46-
.insertInto('tags_closure')
47-
.values({ id_ancestor: tag.id, id_descendant: tag.id })
48-
.onConflict((oc) => oc.doNothing())
49-
.execute();
50-
51-
if (parentId) {
52-
await tx
53-
.insertInto('tags_closure')
54-
.columns(['id_ancestor', 'id_descendant'])
55-
.expression(
56-
this.db
57-
.selectFrom('tags_closure')
58-
.select(['id_ancestor', sql.raw<string>(`'${tag.id}'`).as('id_descendant')])
59-
.where('id_descendant', '=', parentId),
60-
)
61-
.onConflict((oc) => oc.doNothing())
62-
.execute();
63-
}
50+
await this.updateTagClosures(tag, tx);
6451

6552
return tag;
6653
});
@@ -72,13 +59,13 @@ export class TagRepository {
7259
}
7360

7461
@GenerateSql({ params: [{ userId: DummyValue.UUID, color: DummyValue.STRING, value: DummyValue.STRING }] })
75-
create(tag: Insertable<Tags>) {
76-
return this.db.insertInto('tags').values(tag).returningAll().executeTakeFirstOrThrow();
77-
}
78-
79-
@GenerateSql({ params: [DummyValue.UUID] })
80-
async getChildren(parentId: string) {
81-
return await this.db.selectFrom('tags').select(columns.tag).where('parentId', '=', parentId).execute();
62+
async create(tag: Insertable<Tags>) {
63+
let createdTag: Selectable<Tags>;
64+
await this.db.transaction().execute(async (tx) => {
65+
createdTag = await tx.insertInto('tags').values(tag).returningAll().executeTakeFirstOrThrow();
66+
await this.updateTagClosures(createdTag, tx);
67+
});
68+
return createdTag!;
8269
}
8370

8471
@GenerateSql({ params: [DummyValue.UUID, { value: DummyValue.STRING, color: DummyValue.STRING }] })
@@ -89,21 +76,45 @@ export class TagRepository {
8976

9077
if (dto.value) {
9178
// propagate value update downstream
92-
let i = 0;
93-
const queue: { tagId: string; newValue: string }[] = [{tagId: id, newValue: updated.value}];
94-
while (i < queue.length) {
95-
const { tagId, newValue } = queue[i++];
96-
if (tagId !== id) {
97-
await tx.updateTable('tags').set({value: newValue}).where('id', '=', tagId).execute();
79+
const descendantIds = await this.getDescendantIds(id);
80+
const descendants = await this.getMany(descendantIds.filter(_id => _id !== id));
81+
const childrenByParentId = new Map<string, { id: string, value: string }[]>();
82+
for (const descendant of descendants) {
83+
const parentId = descendant.parentId;
84+
if (parentId) {
85+
if (!childrenByParentId.has(parentId)) {
86+
childrenByParentId.set(parentId, []);
87+
}
88+
childrenByParentId.get(parentId)!.push(descendant);
9889
}
90+
}
9991

100-
const children = await this.getChildren(tagId);
92+
const queue: { id: string; value: string }[] = [{id, value: updated.value}];
93+
for (let i = 0; i < queue.length; i++) {
94+
const { id, value } = queue[i];
95+
const children = childrenByParentId.get(id) ?? [];
10196
for (const child of children) {
102-
const name = child.value.split('/').at(-1) as string;
103-
const item = { tagId: child.id, newValue: `${newValue}/${name}` };
97+
const name = child.value.split('/').at(-1)!;
98+
const item = { id: child.id, value: `${value}/${name}` };
10499
queue.push(item);
105100
}
106-
}
101+
}
102+
103+
const toUpdate = queue.slice(1);
104+
if (toUpdate.length > 0) {
105+
await sql`
106+
UPDATE tags
107+
SET value = updates.value
108+
FROM (
109+
VALUES
110+
${sql.join(
111+
toUpdate.map(u => sql`(${sql`${u.id}::uuid`}, ${u.value})`),
112+
sql`, `
113+
)}
114+
) AS updates(id, value)
115+
WHERE tags.id = updates.id
116+
`.execute(tx);
117+
};
107118
}
108119
});
109120

@@ -115,6 +126,29 @@ export class TagRepository {
115126
await this.db.deleteFrom('tags').where('id', '=', id).execute();
116127
}
117128

129+
@GenerateSql()
130+
async deleteEmptyTags() {
131+
// TODO rewrite as a single statement
132+
await this.db.transaction().execute(async (tx) => {
133+
const result = await tx
134+
.selectFrom('assets')
135+
.innerJoin('tag_asset', 'tag_asset.assetsId', 'assets.id')
136+
.innerJoin('tags_closure', 'tags_closure.id_descendant', 'tag_asset.tagsId')
137+
.innerJoin('tags', 'tags.id', 'tags_closure.id_descendant')
138+
.select((eb) => ['tags.id', eb.fn.count<number>('assets.id').as('count')])
139+
.groupBy('tags.id')
140+
.execute();
141+
142+
const ids = result.filter(({ count }) => count === 0).map(({ id }) => id);
143+
if (ids.length > 0) {
144+
await this.db.deleteFrom('tags').where('id', 'in', ids).execute();
145+
this.logger.log(`Deleted ${ids.length} empty tags`);
146+
}
147+
});
148+
}
149+
// #endregion
150+
151+
// #region tag_asset
118152
@ChunkedSet({ paramIndex: 1 })
119153
@GenerateSql({ params: [DummyValue.UUID, [DummyValue.UUID]] })
120154
async getAssetIds(tagId: string, assetIds: string[]): Promise<Set<string>> {
@@ -188,25 +222,40 @@ export class TagRepository {
188222
.execute();
189223
});
190224
}
225+
// #endregion
191226

192-
@GenerateSql()
193-
async deleteEmptyTags() {
194-
// TODO rewrite as a single statement
195-
await this.db.transaction().execute(async (tx) => {
196-
const result = await tx
197-
.selectFrom('assets')
198-
.innerJoin('tag_asset', 'tag_asset.assetsId', 'assets.id')
199-
.innerJoin('tags_closure', 'tags_closure.id_descendant', 'tag_asset.tagsId')
200-
.innerJoin('tags', 'tags.id', 'tags_closure.id_descendant')
201-
.select((eb) => ['tags.id', eb.fn.count<number>('assets.id').as('count')])
202-
.groupBy('tags.id')
203-
.execute();
227+
// #region tag_closure
228+
async getDescendantIds(ancestorId: string) {
229+
const results = await this.db
230+
.selectFrom('tags_closure')
231+
.select('id_descendant')
232+
.where('id_ancestor', '=', ancestorId)
233+
.execute();
204234

205-
const ids = result.filter(({ count }) => count === 0).map(({ id }) => id);
206-
if (ids.length > 0) {
207-
await this.db.deleteFrom('tags').where('id', 'in', ids).execute();
208-
this.logger.log(`Deleted ${ids.length} empty tags`);
209-
}
210-
});
235+
return results.map(r => r.id_descendant);
236+
}
237+
238+
async updateTagClosures(tag: { id: string, parentId?: string | null }, tx: Transaction<DB>) {
239+
// update closure table
240+
await tx
241+
.insertInto('tags_closure')
242+
.values({ id_ancestor: tag.id, id_descendant: tag.id })
243+
.onConflict((oc) => oc.doNothing())
244+
.execute();
245+
246+
if (tag.parentId) {
247+
await tx
248+
.insertInto('tags_closure')
249+
.columns(['id_ancestor', 'id_descendant'])
250+
.expression(
251+
this.db
252+
.selectFrom('tags_closure')
253+
.select(['id_ancestor', sql.raw<string>(`'${tag.id}'`).as('id_descendant')])
254+
.where('id_descendant', '=', tag.parentId),
255+
)
256+
.onConflict((oc) => oc.doNothing())
257+
.execute();
258+
}
211259
}
260+
// #endregion
212261
}

‎server/src/services/tag.service.spec.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,13 @@ describe(TagService.name, () => {
3131
describe('get', () => {
3232
it('should throw an error for an invalid id', async () => {
3333
await expect(sut.get(authStub.admin, 'tag-1')).rejects.toBeInstanceOf(BadRequestException);
34-
expect(mocks.tag.get).toHaveBeenCalledWith('tag-1');
34+
expect(mocks.tag.getOne).toHaveBeenCalledWith('tag-1');
3535
});
3636

3737
it('should return a tag for a user', async () => {
38-
mocks.tag.get.mockResolvedValue(tagStub.tag);
38+
mocks.tag.getOne.mockResolvedValue(tagStub.tag);
3939
await expect(sut.get(authStub.admin, 'tag-1')).resolves.toEqual(tagResponseStub.tag1);
40-
expect(mocks.tag.get).toHaveBeenCalledWith('tag-1');
40+
expect(mocks.tag.getOne).toHaveBeenCalledWith('tag-1');
4141
});
4242
});
4343

@@ -53,8 +53,8 @@ describe(TagService.name, () => {
5353
it('should create a tag with a parent', async () => {
5454
mocks.access.tag.checkOwnerAccess.mockResolvedValue(new Set(['tag-parent']));
5555
mocks.tag.create.mockResolvedValue(tagStub.tagCreate);
56-
mocks.tag.get.mockResolvedValueOnce(tagStub.parentUpsert);
57-
mocks.tag.get.mockResolvedValueOnce(tagStub.childUpsert);
56+
mocks.tag.getOne.mockResolvedValueOnce(tagStub.parentUpsert);
57+
mocks.tag.getOne.mockResolvedValueOnce(tagStub.childUpsert);
5858
await expect(sut.create(authStub.admin, { name: 'tagA', parentId: 'tag-parent' })).resolves.toBeDefined();
5959
expect(mocks.tag.create).toHaveBeenCalledWith(expect.objectContaining({ value: 'Parent/tagA' }));
6060
});
@@ -170,7 +170,7 @@ describe(TagService.name, () => {
170170
});
171171

172172
it('should remove a tag', async () => {
173-
mocks.tag.get.mockResolvedValue(tagStub.tag);
173+
mocks.tag.getOne.mockResolvedValue(tagStub.tag);
174174
mocks.tag.delete.mockResolvedValue();
175175

176176
await sut.remove(authStub.admin, 'tag-1');
@@ -226,7 +226,7 @@ describe(TagService.name, () => {
226226
});
227227

228228
it('should accept accept ids that are new and reject the rest', async () => {
229-
mocks.tag.get.mockResolvedValue(tagStub.tag);
229+
mocks.tag.getOne.mockResolvedValue(tagStub.tag);
230230
mocks.tag.getAssetIds.mockResolvedValue(new Set(['asset-1']));
231231
mocks.tag.addAssetIds.mockResolvedValue();
232232
mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-2']));
@@ -256,7 +256,7 @@ describe(TagService.name, () => {
256256
});
257257

258258
it('should accept accept ids that are tagged and reject the rest', async () => {
259-
mocks.tag.get.mockResolvedValue(tagStub.tag);
259+
mocks.tag.getOne.mockResolvedValue(tagStub.tag);
260260
mocks.tag.getAssetIds.mockResolvedValue(new Set(['asset-1']));
261261
mocks.tag.removeAssetIds.mockResolvedValue();
262262

‎server/src/services/tag.service.ts‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,19 @@ export class TagService extends BaseService {
3434
async create(auth: AuthDto, dto: TagCreateDto) {
3535
const parent = await this.findParent(auth, dto.parentId);
3636
const userId = auth.user.id;
37-
const value = parent ? `${parent.value}/${dto.name}` : dto.name;
37+
38+
const { name, color } = dto;
39+
40+
if (name.includes('/')) {
41+
throw new BadRequestException(`Tag name cannot contain slash characters ("/")`);
42+
}
43+
44+
const value = parent ? `${parent.value}/${name}` : name;
3845
const duplicate = await this.tagRepository.getByValue(userId, value);
3946
if (duplicate) {
4047
throw new BadRequestException(`A tag with that name already exists`);
4148
}
4249

43-
const { color } = dto;
4450
const tag = await this.tagRepository.create({ userId, value, color, parentId: parent?.id });
4551

4652
return mapTag(tag);
@@ -49,7 +55,24 @@ export class TagService extends BaseService {
4955
async update(auth: AuthDto, id: string, dto: TagUpdateDto): Promise<TagResponseDto> {
5056
await this.requireAccess({ auth, permission: Permission.TAG_UPDATE, ids: [id] });
5157

52-
const { value, color } = dto;
58+
const { name, color } = dto;
59+
60+
if (name?.includes('/')) {
61+
throw new BadRequestException(`Tag name cannot contain slash characters ("/")`);
62+
}
63+
64+
const existing = await this.tagRepository.getOne(id);
65+
if (!existing) {
66+
throw new BadRequestException(`Tag not found with id: ${id}`);
67+
}
68+
69+
let value;
70+
if (name) {
71+
const parts = existing.value.split("/");
72+
parts[parts.length - 1] = name;
73+
value = parts.join("/");
74+
}
75+
5376
const tag = await this.tagRepository.update(id, { value, color });
5477
return mapTag(tag);
5578
}
@@ -131,7 +154,7 @@ export class TagService extends BaseService {
131154
}
132155

133156
private async findOrFail(id: string) {
134-
const tag = await this.tagRepository.get(id);
157+
const tag = await this.tagRepository.getOne(id);
135158
if (!tag) {
136159
throw new BadRequestException('Tag not found');
137160
}
@@ -141,7 +164,7 @@ export class TagService extends BaseService {
141164
private async findParent(auth: AuthDto, parentId?: string | null) {
142165
if (parentId) {
143166
await this.requireAccess({ auth, permission: Permission.TAG_READ, ids: [parentId] });
144-
const parent = await this.tagRepository.get(parentId);
167+
const parent = await this.tagRepository.getOne(parentId);
145168
if (!parent) {
146169
throw new BadRequestException('Tag not found');
147170
}

0 commit comments

Comments
 (0)