Skip to content

Commit 9d63960

Browse files
MontejoJorgemertalev
MontejoJorge
and
mertalev
authored
fix: tag clean up query and add tests (#22633)
* fix delete empty tags query * rewrite as a single statement * create tag service medium test * single tag exists, connected to one asset, and is not deleted * do not delete parent tag if children have an asset * hierarchical tag tests * fix query to match 3 test * remove transaction and format:fix * remove transaction and format:fix * simplify query, handle nested empty tag * unused helper --------- Co-authored-by: mertalev <[email protected]>
1 parent 74a9be4 commit 9d63960

File tree

3 files changed

+171
-17
lines changed

3 files changed

+171
-17
lines changed

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

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -163,22 +163,22 @@ export class TagRepository {
163163
}
164164

165165
async deleteEmptyTags() {
166-
// TODO rewrite as a single statement
167-
await this.db.transaction().execute(async (tx) => {
168-
const result = await tx
169-
.selectFrom('asset')
170-
.innerJoin('tag_asset', 'tag_asset.assetsId', 'asset.id')
171-
.innerJoin('tag_closure', 'tag_closure.id_descendant', 'tag_asset.tagsId')
172-
.innerJoin('tag', 'tag.id', 'tag_closure.id_descendant')
173-
.select((eb) => ['tag.id', eb.fn.count<number>('asset.id').as('count')])
174-
.groupBy('tag.id')
175-
.execute();
166+
const result = await this.db
167+
.deleteFrom('tag')
168+
.where(({ not, exists, selectFrom }) =>
169+
not(
170+
exists(
171+
selectFrom('tag_closure')
172+
.whereRef('tag.id', '=', 'tag_closure.id_ancestor')
173+
.innerJoin('tag_asset', 'tag_closure.id_descendant', 'tag_asset.tagsId'),
174+
),
175+
),
176+
)
177+
.executeTakeFirst();
176178

177-
const ids = result.filter(({ count }) => count === 0).map(({ id }) => id);
178-
if (ids.length > 0) {
179-
await this.db.deleteFrom('tag').where('id', 'in', ids).execute();
180-
this.logger.log(`Deleted ${ids.length} empty tags`);
181-
}
182-
});
179+
const deletedRows = Number(result.numDeletedRows);
180+
if (deletedRows > 0) {
181+
this.logger.log(`Deleted ${deletedRows} empty tags`);
182+
}
183183
}
184184
}

‎server/test/medium.factory.ts‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import { StorageRepository } from 'src/repositories/storage.repository';
3939
import { SyncCheckpointRepository } from 'src/repositories/sync-checkpoint.repository';
4040
import { SyncRepository } from 'src/repositories/sync.repository';
4141
import { SystemMetadataRepository } from 'src/repositories/system-metadata.repository';
42+
import { TagRepository } from 'src/repositories/tag.repository';
4243
import { TelemetryRepository } from 'src/repositories/telemetry.repository';
4344
import { UserRepository } from 'src/repositories/user.repository';
4445
import { VersionHistoryRepository } from 'src/repositories/version-history.repository';
@@ -52,6 +53,8 @@ import { MemoryTable } from 'src/schema/tables/memory.table';
5253
import { PersonTable } from 'src/schema/tables/person.table';
5354
import { SessionTable } from 'src/schema/tables/session.table';
5455
import { StackTable } from 'src/schema/tables/stack.table';
56+
import { TagAssetTable } from 'src/schema/tables/tag-asset.table';
57+
import { TagTable } from 'src/schema/tables/tag.table';
5558
import { UserTable } from 'src/schema/tables/user.table';
5659
import { BASE_SERVICE_DEPENDENCIES, BaseService } from 'src/services/base.service';
5760
import { SyncService } from 'src/services/sync.service';
@@ -240,6 +243,18 @@ export class MediumTestContext<S extends BaseService = BaseService> {
240243
user,
241244
};
242245
}
246+
247+
async newTagAsset(tagBulkAssets: { tagIds: string[]; assetIds: string[] }) {
248+
const tagsAssets: Insertable<TagAssetTable>[] = [];
249+
for (const tagsId of tagBulkAssets.tagIds) {
250+
for (const assetsId of tagBulkAssets.assetIds) {
251+
tagsAssets.push({ tagsId, assetsId });
252+
}
253+
}
254+
255+
const result = await this.get(TagRepository).upsertAssetIds(tagsAssets);
256+
return { tagsAssets, result };
257+
}
243258
}
244259

245260
export class SyncTestContext extends MediumTestContext<SyncService> {
@@ -318,6 +333,10 @@ const newRealRepository = <T>(key: ClassConstructor<T>, db: Kysely<DB>): T => {
318333
return new key(LoggingRepository.create());
319334
}
320335

336+
case TagRepository: {
337+
return new key(db, LoggingRepository.create());
338+
}
339+
321340
case LoggingRepository as unknown as ClassConstructor<LoggingRepository>: {
322341
return new key() as unknown as T;
323342
}
@@ -345,7 +364,8 @@ const newMockRepository = <T>(key: ClassConstructor<T>) => {
345364
case SyncCheckpointRepository:
346365
case SystemMetadataRepository:
347366
case UserRepository:
348-
case VersionHistoryRepository: {
367+
case VersionHistoryRepository:
368+
case TagRepository: {
349369
return automock(key);
350370
}
351371

@@ -567,6 +587,23 @@ const memoryInsert = (memory: Partial<Insertable<MemoryTable>> = {}) => {
567587
return { ...defaults, ...memory, id };
568588
};
569589

590+
const tagInsert = (tag: Partial<Insertable<TagTable>>) => {
591+
const id = tag.id || newUuid();
592+
593+
const defaults: Insertable<TagTable> = {
594+
id,
595+
userId: '',
596+
value: '',
597+
createdAt: newDate(),
598+
updatedAt: newDate(),
599+
color: '',
600+
parentId: null,
601+
updateId: newUuid(),
602+
};
603+
604+
return { ...defaults, ...tag, id };
605+
};
606+
570607
class CustomWritable extends Writable {
571608
private data = '';
572609

@@ -619,4 +656,5 @@ export const mediumFactory = {
619656
memoryInsert,
620657
loginDetails,
621658
loginResponse,
659+
tagInsert,
622660
};

‎server/test/medium/specs/services/tag.service.spec.ts‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
import { Kysely } from 'kysely';
2+
import { JobStatus } from 'src/enum';
3+
import { AccessRepository } from 'src/repositories/access.repository';
4+
import { LoggingRepository } from 'src/repositories/logging.repository';
5+
import { TagRepository } from 'src/repositories/tag.repository';
6+
import { DB } from 'src/schema';
7+
import { TagService } from 'src/services/tag.service';
8+
import { upsertTags } from 'src/utils/tag';
9+
import { newMediumService } from 'test/medium.factory';
10+
import { getKyselyDB } from 'test/utils';
11+
12+
let defaultDatabase: Kysely<DB>;
13+
14+
const setup = (db?: Kysely<DB>) => {
15+
return newMediumService(TagService, {
16+
database: db || defaultDatabase,
17+
real: [TagRepository, AccessRepository],
18+
mock: [LoggingRepository],
19+
});
20+
};
21+
22+
beforeAll(async () => {
23+
defaultDatabase = await getKyselyDB();
24+
});
25+
26+
describe(TagService.name, () => {
27+
describe('deleteEmptyTags', () => {
28+
it('single tag exists, not connected to any assets, and is deleted', async () => {
29+
const { sut, ctx } = setup();
30+
const { user } = await ctx.newUser();
31+
const tagRepo = ctx.get(TagRepository);
32+
const [tag] = await upsertTags(tagRepo, { userId: user.id, tags: ['tag-1'] });
33+
34+
await expect(tagRepo.getByValue(user.id, 'tag-1')).resolves.toEqual(expect.objectContaining({ id: tag.id }));
35+
await expect(sut.handleTagCleanup()).resolves.toBe(JobStatus.Success);
36+
await expect(tagRepo.getByValue(user.id, 'tag-1')).resolves.toBeUndefined();
37+
});
38+
39+
it('single tag exists, connected to one asset, and is not deleted', async () => {
40+
const { sut, ctx } = setup();
41+
const { user } = await ctx.newUser();
42+
const { asset } = await ctx.newAsset({ ownerId: user.id });
43+
const tagRepo = ctx.get(TagRepository);
44+
const [tag] = await upsertTags(tagRepo, { userId: user.id, tags: ['tag-1'] });
45+
46+
await ctx.newTagAsset({ tagIds: [tag.id], assetIds: [asset.id] });
47+
48+
await expect(tagRepo.getByValue(user.id, 'tag-1')).resolves.toEqual(expect.objectContaining({ id: tag.id }));
49+
await expect(sut.handleTagCleanup()).resolves.toBe(JobStatus.Success);
50+
await expect(tagRepo.getByValue(user.id, 'tag-1')).resolves.toEqual(expect.objectContaining({ id: tag.id }));
51+
});
52+
53+
it('hierarchical tag exists, and the parent is connected to an asset, and the child is deleted', async () => {
54+
const { sut, ctx } = setup();
55+
const { user } = await ctx.newUser();
56+
const { asset } = await ctx.newAsset({ ownerId: user.id });
57+
const tagRepo = ctx.get(TagRepository);
58+
const [parentTag, childTag] = await upsertTags(tagRepo, { userId: user.id, tags: ['parent', 'parent/child'] });
59+
60+
await ctx.newTagAsset({ tagIds: [parentTag.id], assetIds: [asset.id] });
61+
62+
await expect(tagRepo.getByValue(user.id, 'parent')).resolves.toEqual(
63+
expect.objectContaining({ id: parentTag.id }),
64+
);
65+
await expect(tagRepo.getByValue(user.id, 'parent/child')).resolves.toEqual(
66+
expect.objectContaining({ id: childTag.id }),
67+
);
68+
await expect(sut.handleTagCleanup()).resolves.toBe(JobStatus.Success);
69+
await expect(tagRepo.getByValue(user.id, 'parent')).resolves.toEqual(
70+
expect.objectContaining({ id: parentTag.id }),
71+
);
72+
await expect(tagRepo.getByValue(user.id, 'parent/child')).resolves.toBeUndefined();
73+
});
74+
75+
it('hierarchical tag exists, and only the child is connected to an asset, and nothing is deleted', async () => {
76+
const { sut, ctx } = setup();
77+
const { user } = await ctx.newUser();
78+
const { asset } = await ctx.newAsset({ ownerId: user.id });
79+
const tagRepo = ctx.get(TagRepository);
80+
const [parentTag, childTag] = await upsertTags(tagRepo, { userId: user.id, tags: ['parent', 'parent/child'] });
81+
82+
await ctx.newTagAsset({ tagIds: [childTag.id], assetIds: [asset.id] });
83+
84+
await expect(tagRepo.getByValue(user.id, 'parent')).resolves.toEqual(
85+
expect.objectContaining({ id: parentTag.id }),
86+
);
87+
await expect(tagRepo.getByValue(user.id, 'parent/child')).resolves.toEqual(
88+
expect.objectContaining({ id: childTag.id }),
89+
);
90+
await expect(sut.handleTagCleanup()).resolves.toBe(JobStatus.Success);
91+
await expect(tagRepo.getByValue(user.id, 'parent')).resolves.toEqual(
92+
expect.objectContaining({ id: parentTag.id }),
93+
);
94+
await expect(tagRepo.getByValue(user.id, 'parent/child')).resolves.toEqual(
95+
expect.objectContaining({ id: childTag.id }),
96+
);
97+
});
98+
99+
it('hierarchical tag exists, and neither parent nor child is connected to an asset, and both are deleted', async () => {
100+
const { sut, ctx } = setup();
101+
const { user } = await ctx.newUser();
102+
const tagRepo = ctx.get(TagRepository);
103+
const [parentTag, childTag] = await upsertTags(tagRepo, { userId: user.id, tags: ['parent', 'parent/child'] });
104+
105+
await expect(tagRepo.getByValue(user.id, 'parent')).resolves.toEqual(
106+
expect.objectContaining({ id: parentTag.id }),
107+
);
108+
await expect(tagRepo.getByValue(user.id, 'parent/child')).resolves.toEqual(
109+
expect.objectContaining({ id: childTag.id }),
110+
);
111+
await expect(sut.handleTagCleanup()).resolves.toBe(JobStatus.Success);
112+
await expect(tagRepo.getByValue(user.id, 'parent/child')).resolves.toBeUndefined();
113+
await expect(tagRepo.getByValue(user.id, 'parent')).resolves.toBeUndefined();
114+
});
115+
});
116+
});

0 commit comments

Comments
 (0)