fix(CQ-API-7): Fix N+1 query in knowledge tag lookup
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
Replace Promise.all of individual findUnique queries per tag with a single findMany batch query. Only missing tags are created individually. Tag associations now use createMany instead of individual creates. Also deduplicates tags by slug via Map, preventing duplicate entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
353
apps/api/src/knowledge/knowledge.service.sync-tags.spec.ts
Normal file
353
apps/api/src/knowledge/knowledge.service.sync-tags.spec.ts
Normal file
@@ -0,0 +1,353 @@
|
|||||||
|
import { describe, it, expect, beforeEach, vi } from "vitest";
|
||||||
|
import { Test, TestingModule } from "@nestjs/testing";
|
||||||
|
import { KnowledgeService } from "./knowledge.service";
|
||||||
|
import { PrismaService } from "../prisma/prisma.service";
|
||||||
|
import { LinkSyncService } from "./services/link-sync.service";
|
||||||
|
import { KnowledgeCacheService } from "./services/cache.service";
|
||||||
|
import { EmbeddingService } from "./services/embedding.service";
|
||||||
|
import { OllamaEmbeddingService } from "./services/ollama-embedding.service";
|
||||||
|
import { EmbeddingQueueService } from "./queues/embedding-queue.service";
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Tests for syncTags N+1 query fix (CQ-API-7).
|
||||||
|
*
|
||||||
|
* syncTags is a private method invoked via create(). These tests verify
|
||||||
|
* that the batch findMany pattern is used instead of individual findUnique
|
||||||
|
* queries per tag, and that missing tags are created correctly.
|
||||||
|
*/
|
||||||
|
describe("KnowledgeService - syncTags (N+1 fix)", () => {
|
||||||
|
let service: KnowledgeService;
|
||||||
|
|
||||||
|
const workspaceId = "workspace-123";
|
||||||
|
const userId = "user-456";
|
||||||
|
const entryId = "entry-789";
|
||||||
|
|
||||||
|
// Transaction mock objects - these simulate the Prisma transaction client
|
||||||
|
const mockTx = {
|
||||||
|
knowledgeEntry: {
|
||||||
|
create: vi.fn(),
|
||||||
|
findUnique: vi.fn(),
|
||||||
|
},
|
||||||
|
knowledgeEntryVersion: {
|
||||||
|
create: vi.fn(),
|
||||||
|
},
|
||||||
|
knowledgeTag: {
|
||||||
|
findMany: vi.fn(),
|
||||||
|
create: vi.fn(),
|
||||||
|
},
|
||||||
|
knowledgeEntryTag: {
|
||||||
|
deleteMany: vi.fn(),
|
||||||
|
createMany: vi.fn(),
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockPrismaService = {
|
||||||
|
knowledgeEntry: {
|
||||||
|
findUnique: vi.fn(),
|
||||||
|
},
|
||||||
|
$transaction: vi.fn(),
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockLinkSyncService = {
|
||||||
|
syncLinks: vi.fn().mockResolvedValue(undefined),
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockCacheService = {
|
||||||
|
getEntry: vi.fn().mockResolvedValue(null),
|
||||||
|
setEntry: vi.fn().mockResolvedValue(undefined),
|
||||||
|
invalidateEntry: vi.fn().mockResolvedValue(undefined),
|
||||||
|
getSearch: vi.fn().mockResolvedValue(null),
|
||||||
|
setSearch: vi.fn().mockResolvedValue(undefined),
|
||||||
|
invalidateSearches: vi.fn().mockResolvedValue(undefined),
|
||||||
|
getGraph: vi.fn().mockResolvedValue(null),
|
||||||
|
setGraph: vi.fn().mockResolvedValue(undefined),
|
||||||
|
invalidateGraphs: vi.fn().mockResolvedValue(undefined),
|
||||||
|
invalidateGraphsForEntry: vi.fn().mockResolvedValue(undefined),
|
||||||
|
clearWorkspaceCache: vi.fn().mockResolvedValue(undefined),
|
||||||
|
getStats: vi.fn().mockReturnValue({ hits: 0, misses: 0, sets: 0, deletes: 0, hitRate: 0 }),
|
||||||
|
resetStats: vi.fn(),
|
||||||
|
isEnabled: vi.fn().mockReturnValue(false),
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockEmbeddingService = {
|
||||||
|
isConfigured: vi.fn().mockReturnValue(false),
|
||||||
|
generateEmbedding: vi.fn().mockResolvedValue(null),
|
||||||
|
batchGenerateEmbeddings: vi.fn().mockResolvedValue([]),
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockOllamaEmbeddingService = {
|
||||||
|
isConfigured: vi.fn().mockResolvedValue(false),
|
||||||
|
generateEmbedding: vi.fn().mockResolvedValue([]),
|
||||||
|
generateAndStoreEmbedding: vi.fn().mockResolvedValue(undefined),
|
||||||
|
batchGenerateEmbeddings: vi.fn().mockResolvedValue(0),
|
||||||
|
prepareContentForEmbedding: vi.fn().mockReturnValue("combined content"),
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockEmbeddingQueueService = {
|
||||||
|
queueEmbeddingJob: vi.fn().mockResolvedValue("job-123"),
|
||||||
|
};
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Helper to set up the $transaction mock so it executes the callback
|
||||||
|
* with our mockTx and returns a properly shaped entry result.
|
||||||
|
*/
|
||||||
|
function setupTransactionForCreate(
|
||||||
|
tags: Array<{ id: string; name: string; slug: string; color: string | null }>
|
||||||
|
): void {
|
||||||
|
const createdEntry = {
|
||||||
|
id: entryId,
|
||||||
|
workspaceId,
|
||||||
|
slug: "test-entry",
|
||||||
|
title: "Test Entry",
|
||||||
|
content: "# Test",
|
||||||
|
contentHtml: "<h1>Test</h1>",
|
||||||
|
summary: null,
|
||||||
|
status: "DRAFT",
|
||||||
|
visibility: "PRIVATE",
|
||||||
|
createdBy: userId,
|
||||||
|
updatedBy: userId,
|
||||||
|
createdAt: new Date("2026-01-01"),
|
||||||
|
updatedAt: new Date("2026-01-01"),
|
||||||
|
tags: tags.map((t) => ({
|
||||||
|
entryId,
|
||||||
|
tagId: t.id,
|
||||||
|
tag: t,
|
||||||
|
})),
|
||||||
|
};
|
||||||
|
|
||||||
|
mockTx.knowledgeEntry.create.mockResolvedValue(createdEntry);
|
||||||
|
mockTx.knowledgeEntryVersion.create.mockResolvedValue({});
|
||||||
|
mockTx.knowledgeEntryTag.deleteMany.mockResolvedValue({ count: 0 });
|
||||||
|
mockTx.knowledgeEntryTag.createMany.mockResolvedValue({ count: tags.length });
|
||||||
|
mockTx.knowledgeEntry.findUnique.mockResolvedValue(createdEntry);
|
||||||
|
|
||||||
|
// ensureUniqueSlug uses prisma (not tx), so mock the outer prisma
|
||||||
|
mockPrismaService.knowledgeEntry.findUnique.mockResolvedValue(null);
|
||||||
|
|
||||||
|
mockPrismaService.$transaction.mockImplementation(
|
||||||
|
async (callback: (tx: typeof mockTx) => Promise<typeof createdEntry>) => {
|
||||||
|
return callback(mockTx);
|
||||||
|
}
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
beforeEach(async () => {
|
||||||
|
const module: TestingModule = await Test.createTestingModule({
|
||||||
|
providers: [
|
||||||
|
KnowledgeService,
|
||||||
|
{ provide: PrismaService, useValue: mockPrismaService },
|
||||||
|
{ provide: LinkSyncService, useValue: mockLinkSyncService },
|
||||||
|
{ provide: KnowledgeCacheService, useValue: mockCacheService },
|
||||||
|
{ provide: EmbeddingService, useValue: mockEmbeddingService },
|
||||||
|
{ provide: OllamaEmbeddingService, useValue: mockOllamaEmbeddingService },
|
||||||
|
{ provide: EmbeddingQueueService, useValue: mockEmbeddingQueueService },
|
||||||
|
],
|
||||||
|
}).compile();
|
||||||
|
|
||||||
|
service = module.get<KnowledgeService>(KnowledgeService);
|
||||||
|
vi.clearAllMocks();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should use findMany to batch-fetch existing tags instead of individual queries", async () => {
|
||||||
|
const existingTag = {
|
||||||
|
id: "tag-1",
|
||||||
|
workspaceId,
|
||||||
|
name: "JavaScript",
|
||||||
|
slug: "javascript",
|
||||||
|
color: null,
|
||||||
|
};
|
||||||
|
mockTx.knowledgeTag.findMany.mockResolvedValue([existingTag]);
|
||||||
|
|
||||||
|
setupTransactionForCreate([existingTag]);
|
||||||
|
|
||||||
|
await service.create(workspaceId, userId, {
|
||||||
|
title: "Test Entry",
|
||||||
|
content: "# Test",
|
||||||
|
tags: ["JavaScript"],
|
||||||
|
});
|
||||||
|
|
||||||
|
// Verify findMany was called with slug IN array (batch query)
|
||||||
|
expect(mockTx.knowledgeTag.findMany).toHaveBeenCalledWith({
|
||||||
|
where: {
|
||||||
|
workspaceId,
|
||||||
|
slug: { in: ["javascript"] },
|
||||||
|
},
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should create only missing tags when some already exist", async () => {
|
||||||
|
const existingTag = {
|
||||||
|
id: "tag-1",
|
||||||
|
workspaceId,
|
||||||
|
name: "JavaScript",
|
||||||
|
slug: "javascript",
|
||||||
|
color: null,
|
||||||
|
};
|
||||||
|
const newTag = {
|
||||||
|
id: "tag-2",
|
||||||
|
workspaceId,
|
||||||
|
name: "TypeScript",
|
||||||
|
slug: "typescript",
|
||||||
|
color: null,
|
||||||
|
};
|
||||||
|
|
||||||
|
// findMany returns only the existing tag
|
||||||
|
mockTx.knowledgeTag.findMany.mockResolvedValue([existingTag]);
|
||||||
|
// create is called only for the missing tag
|
||||||
|
mockTx.knowledgeTag.create.mockResolvedValue(newTag);
|
||||||
|
|
||||||
|
setupTransactionForCreate([existingTag, newTag]);
|
||||||
|
|
||||||
|
await service.create(workspaceId, userId, {
|
||||||
|
title: "Test Entry",
|
||||||
|
content: "# Test",
|
||||||
|
tags: ["JavaScript", "TypeScript"],
|
||||||
|
});
|
||||||
|
|
||||||
|
// findMany should be called once with both slugs
|
||||||
|
expect(mockTx.knowledgeTag.findMany).toHaveBeenCalledTimes(1);
|
||||||
|
expect(mockTx.knowledgeTag.findMany).toHaveBeenCalledWith({
|
||||||
|
where: {
|
||||||
|
workspaceId,
|
||||||
|
slug: { in: ["javascript", "typescript"] },
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
// Only the missing tag should be created
|
||||||
|
expect(mockTx.knowledgeTag.create).toHaveBeenCalledTimes(1);
|
||||||
|
expect(mockTx.knowledgeTag.create).toHaveBeenCalledWith({
|
||||||
|
data: {
|
||||||
|
workspaceId,
|
||||||
|
name: "TypeScript",
|
||||||
|
slug: "typescript",
|
||||||
|
},
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should create all tags when none exist", async () => {
|
||||||
|
const tag1 = { id: "tag-1", workspaceId, name: "React", slug: "react", color: null };
|
||||||
|
const tag2 = { id: "tag-2", workspaceId, name: "Vue", slug: "vue", color: null };
|
||||||
|
|
||||||
|
// No existing tags found
|
||||||
|
mockTx.knowledgeTag.findMany.mockResolvedValue([]);
|
||||||
|
mockTx.knowledgeTag.create.mockResolvedValueOnce(tag1).mockResolvedValueOnce(tag2);
|
||||||
|
|
||||||
|
setupTransactionForCreate([tag1, tag2]);
|
||||||
|
|
||||||
|
await service.create(workspaceId, userId, {
|
||||||
|
title: "Test Entry",
|
||||||
|
content: "# Test",
|
||||||
|
tags: ["React", "Vue"],
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(mockTx.knowledgeTag.findMany).toHaveBeenCalledTimes(1);
|
||||||
|
expect(mockTx.knowledgeTag.create).toHaveBeenCalledTimes(2);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should not create any tags when all already exist", async () => {
|
||||||
|
const tag1 = { id: "tag-1", workspaceId, name: "Python", slug: "python", color: null };
|
||||||
|
const tag2 = { id: "tag-2", workspaceId, name: "Go", slug: "go", color: null };
|
||||||
|
|
||||||
|
mockTx.knowledgeTag.findMany.mockResolvedValue([tag1, tag2]);
|
||||||
|
|
||||||
|
setupTransactionForCreate([tag1, tag2]);
|
||||||
|
|
||||||
|
await service.create(workspaceId, userId, {
|
||||||
|
title: "Test Entry",
|
||||||
|
content: "# Test",
|
||||||
|
tags: ["Python", "Go"],
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(mockTx.knowledgeTag.findMany).toHaveBeenCalledTimes(1);
|
||||||
|
expect(mockTx.knowledgeTag.create).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should use createMany for tag associations instead of individual creates", async () => {
|
||||||
|
const tag1 = { id: "tag-1", workspaceId, name: "Rust", slug: "rust", color: null };
|
||||||
|
const tag2 = { id: "tag-2", workspaceId, name: "Zig", slug: "zig", color: null };
|
||||||
|
|
||||||
|
mockTx.knowledgeTag.findMany.mockResolvedValue([tag1, tag2]);
|
||||||
|
|
||||||
|
setupTransactionForCreate([tag1, tag2]);
|
||||||
|
|
||||||
|
await service.create(workspaceId, userId, {
|
||||||
|
title: "Test Entry",
|
||||||
|
content: "# Test",
|
||||||
|
tags: ["Rust", "Zig"],
|
||||||
|
});
|
||||||
|
|
||||||
|
// createMany should be called once with all associations
|
||||||
|
expect(mockTx.knowledgeEntryTag.createMany).toHaveBeenCalledTimes(1);
|
||||||
|
expect(mockTx.knowledgeEntryTag.createMany).toHaveBeenCalledWith({
|
||||||
|
data: [
|
||||||
|
{ entryId, tagId: "tag-1" },
|
||||||
|
{ entryId, tagId: "tag-2" },
|
||||||
|
],
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should skip tag sync when no tags are provided", async () => {
|
||||||
|
setupTransactionForCreate([]);
|
||||||
|
|
||||||
|
await service.create(workspaceId, userId, {
|
||||||
|
title: "Test Entry",
|
||||||
|
content: "# Test",
|
||||||
|
tags: [],
|
||||||
|
});
|
||||||
|
|
||||||
|
// No tag queries should be made when tags array is empty
|
||||||
|
expect(mockTx.knowledgeTag.findMany).not.toHaveBeenCalled();
|
||||||
|
expect(mockTx.knowledgeTag.create).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should deduplicate tags with the same slug", async () => {
|
||||||
|
// "JavaScript" and "javascript" produce the same slug
|
||||||
|
const existingTag = {
|
||||||
|
id: "tag-1",
|
||||||
|
workspaceId,
|
||||||
|
name: "JavaScript",
|
||||||
|
slug: "javascript",
|
||||||
|
color: null,
|
||||||
|
};
|
||||||
|
|
||||||
|
mockTx.knowledgeTag.findMany.mockResolvedValue([existingTag]);
|
||||||
|
|
||||||
|
setupTransactionForCreate([existingTag]);
|
||||||
|
|
||||||
|
await service.create(workspaceId, userId, {
|
||||||
|
title: "Test Entry",
|
||||||
|
content: "# Test",
|
||||||
|
tags: ["JavaScript", "javascript"],
|
||||||
|
});
|
||||||
|
|
||||||
|
// findMany should be called with deduplicated slugs
|
||||||
|
expect(mockTx.knowledgeTag.findMany).toHaveBeenCalledWith({
|
||||||
|
where: {
|
||||||
|
workspaceId,
|
||||||
|
slug: { in: ["javascript"] },
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
// Only one association created (deduped by slug)
|
||||||
|
expect(mockTx.knowledgeEntryTag.createMany).toHaveBeenCalledWith({
|
||||||
|
data: [{ entryId, tagId: "tag-1" }],
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should delete existing tag associations before syncing", async () => {
|
||||||
|
const tag1 = { id: "tag-1", workspaceId, name: "Node", slug: "node", color: null };
|
||||||
|
mockTx.knowledgeTag.findMany.mockResolvedValue([tag1]);
|
||||||
|
|
||||||
|
setupTransactionForCreate([tag1]);
|
||||||
|
|
||||||
|
await service.create(workspaceId, userId, {
|
||||||
|
title: "Test Entry",
|
||||||
|
content: "# Test",
|
||||||
|
tags: ["Node"],
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(mockTx.knowledgeEntryTag.deleteMany).toHaveBeenCalledWith({
|
||||||
|
where: { entryId },
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -821,45 +821,48 @@ export class KnowledgeService {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Get or create tags
|
// Build slug map: slug -> original tag name
|
||||||
const tags = await Promise.all(
|
const slugToName = new Map<string, string>();
|
||||||
tagNames.map(async (name) => {
|
for (const name of tagNames) {
|
||||||
const tagSlug = this.generateSlug(name);
|
slugToName.set(this.generateSlug(name), name);
|
||||||
|
}
|
||||||
|
const tagSlugs = [...slugToName.keys()];
|
||||||
|
|
||||||
// Try to find existing tag
|
// Batch fetch all existing tags in a single query (fixes N+1)
|
||||||
let tag = await tx.knowledgeTag.findUnique({
|
const existingTags = await tx.knowledgeTag.findMany({
|
||||||
where: {
|
where: {
|
||||||
workspaceId_slug: {
|
workspaceId,
|
||||||
workspaceId,
|
slug: { in: tagSlugs },
|
||||||
slug: tagSlug,
|
},
|
||||||
},
|
});
|
||||||
},
|
|
||||||
});
|
|
||||||
|
|
||||||
// Create if doesn't exist
|
// Determine which tags need to be created
|
||||||
tag ??= await tx.knowledgeTag.create({
|
const existingSlugs = new Set(existingTags.map((t) => t.slug));
|
||||||
|
const missingSlugs = tagSlugs.filter((s) => !existingSlugs.has(s));
|
||||||
|
|
||||||
|
// Create missing tags
|
||||||
|
const newTags = await Promise.all(
|
||||||
|
missingSlugs.map((slug) => {
|
||||||
|
const name = slugToName.get(slug) ?? slug;
|
||||||
|
return tx.knowledgeTag.create({
|
||||||
data: {
|
data: {
|
||||||
workspaceId,
|
workspaceId,
|
||||||
name,
|
name,
|
||||||
slug: tagSlug,
|
slug,
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
|
|
||||||
return tag;
|
|
||||||
})
|
})
|
||||||
);
|
);
|
||||||
|
|
||||||
// Create tag associations
|
const allTags = [...existingTags, ...newTags];
|
||||||
await Promise.all(
|
|
||||||
tags.map((tag) =>
|
// Create tag associations in a single batch
|
||||||
tx.knowledgeEntryTag.create({
|
await tx.knowledgeEntryTag.createMany({
|
||||||
data: {
|
data: allTags.map((tag) => ({
|
||||||
entryId,
|
entryId,
|
||||||
tagId: tag.id,
|
tagId: tag.id,
|
||||||
},
|
})),
|
||||||
})
|
});
|
||||||
)
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
Reference in New Issue
Block a user