Browse Source

refactor(suggest-path): unify ES search to single query shared by evaluate and category pipelines

Previously generateCategorySuggestion performed its own ES search independently,
duplicating the search done by retrieveSearchCandidates with the same keywords.
Now search runs once and results are passed to both pipelines, halving ES load.

Also simplified generateCategorySuggestion to accept SearchCandidate[] directly,
and inlined the category description generation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VANELLOPE\tomoyuki-t 1 month ago
parent
commit
ecbcc658aa

+ 1 - 7
apps/app/src/features/ai-tools/suggest-path/server/routes/apiv3/index.ts

@@ -95,13 +95,7 @@ export const suggestPathHandlersFactory = (crowi: Crowi): RequestHandler[] => {
                 searchService: typedSearchService,
               }),
             evaluateCandidates,
-            generateCategorySuggestion: (keywords, u, groups) =>
-              generateCategorySuggestion(
-                keywords,
-                u,
-                groups,
-                typedSearchService,
-              ),
+            generateCategorySuggestion,
             resolveParentGrant,
           },
         );

+ 43 - 185
apps/app/src/features/ai-tools/suggest-path/server/services/generate-category-suggestion.spec.ts

@@ -1,8 +1,6 @@
-import type { IUserHasId } from '@growi/core/dist/interfaces';
-
+import type { SearchCandidate } from '../../interfaces/suggest-path-types';
 import {
-  extractTopLevelSegment,
-  generateCategoryDescription,
+  extractTopLevelSegmentName,
   generateCategorySuggestion,
 } from './generate-category-suggestion';
 
@@ -19,58 +17,33 @@ vi.mock('./resolve-parent-grant', () => ({
 const GRANT_PUBLIC = 1;
 const GRANT_OWNER = 4;
 
-function createSearchResult(pages: { path: string; score: number }[]) {
-  return {
-    data: pages.map((p) => ({
-      _id: `id-${p.path}`,
-      _score: p.score,
-      _source: { path: p.path },
-    })),
-    meta: { total: pages.length, hitsCount: pages.length },
-  };
-}
-
-function createMockSearchService(
-  result: ReturnType<typeof createSearchResult>,
-) {
-  return {
-    searchKeyword: vi.fn().mockResolvedValue([result, 'DEFAULT']),
-  };
+function createCandidates(
+  pages: { path: string; score: number }[],
+): SearchCandidate[] {
+  return pages.map((p) => ({
+    pagePath: p.path,
+    snippet: '',
+    score: p.score,
+  }));
 }
 
-const mockUser = { _id: 'user1', username: 'alice' } as unknown as IUserHasId;
-
-describe('extractTopLevelSegment', () => {
-  it('should extract top-level segment from nested path', () => {
-    expect(extractTopLevelSegment('/tech-notes/React/hooks')).toBe(
-      '/tech-notes/',
+describe('extractTopLevelSegmentName', () => {
+  it('should extract segment name from nested path', () => {
+    expect(extractTopLevelSegmentName('/tech-notes/React/hooks')).toBe(
+      'tech-notes',
     );
   });
 
-  it('should extract top-level segment from two-level path', () => {
-    expect(extractTopLevelSegment('/tech-notes/React')).toBe('/tech-notes/');
-  });
-
-  it('should extract top-level segment from single-level path', () => {
-    expect(extractTopLevelSegment('/tech-notes')).toBe('/tech-notes/');
-  });
-
-  it('should return root for root path', () => {
-    expect(extractTopLevelSegment('/')).toBe('/');
+  it('should extract segment name from two-level path', () => {
+    expect(extractTopLevelSegmentName('/tech-notes/React')).toBe('tech-notes');
   });
-});
 
-describe('generateCategoryDescription', () => {
-  it('should generate description from segment name', () => {
-    expect(generateCategoryDescription('tech-notes')).toBe(
-      'Top-level category: tech-notes',
-    );
+  it('should extract segment name from single-level path', () => {
+    expect(extractTopLevelSegmentName('/tech-notes')).toBe('tech-notes');
   });
 
-  it('should handle single word segment', () => {
-    expect(generateCategoryDescription('guides')).toBe(
-      'Top-level category: guides',
-    );
+  it('should return null for root path', () => {
+    expect(extractTopLevelSegmentName('/')).toBeNull();
   });
 });
 
@@ -80,118 +53,76 @@ describe('generateCategorySuggestion', () => {
     mocks.resolveParentGrantMock.mockResolvedValue(GRANT_PUBLIC);
   });
 
-  describe('when search returns results', () => {
+  describe('when candidates are provided', () => {
     it('should return a suggestion with type "category"', async () => {
-      const searchResult = createSearchResult([
+      const candidates = createCandidates([
         { path: '/tech-notes/React/hooks', score: 10 },
       ]);
-      const searchService = createMockSearchService(searchResult);
 
-      const result = await generateCategorySuggestion(
-        ['React', 'hooks'],
-        mockUser,
-        [],
-        searchService,
-      );
+      const result = await generateCategorySuggestion(candidates);
 
       expect(result).not.toBeNull();
       expect(result?.type).toBe('category');
     });
 
-    it('should extract top-level segment from top result path', async () => {
-      const searchResult = createSearchResult([
+    it('should extract top-level segment from top candidate path', async () => {
+      const candidates = createCandidates([
         { path: '/tech-notes/React/hooks', score: 10 },
         { path: '/guides/TypeScript/basics', score: 8 },
       ]);
-      const searchService = createMockSearchService(searchResult);
 
-      const result = await generateCategorySuggestion(
-        ['React'],
-        mockUser,
-        [],
-        searchService,
-      );
+      const result = await generateCategorySuggestion(candidates);
 
       expect(result?.path).toBe('/tech-notes/');
     });
 
     it('should return path with trailing slash', async () => {
-      const searchResult = createSearchResult([
+      const candidates = createCandidates([
         { path: '/tech-notes/React/hooks', score: 10 },
       ]);
-      const searchService = createMockSearchService(searchResult);
 
-      const result = await generateCategorySuggestion(
-        ['React'],
-        mockUser,
-        [],
-        searchService,
-      );
+      const result = await generateCategorySuggestion(candidates);
 
       expect(result?.path).toMatch(/\/$/);
     });
 
     it('should extract top-level even from deeply nested path', async () => {
-      const searchResult = createSearchResult([
+      const candidates = createCandidates([
         { path: '/guides/a/b/c/d', score: 10 },
       ]);
-      const searchService = createMockSearchService(searchResult);
 
-      const result = await generateCategorySuggestion(
-        ['keyword'],
-        mockUser,
-        [],
-        searchService,
-      );
+      const result = await generateCategorySuggestion(candidates);
 
       expect(result?.path).toBe('/guides/');
     });
 
     it('should generate description from top-level segment name', async () => {
-      const searchResult = createSearchResult([
+      const candidates = createCandidates([
         { path: '/tech-notes/React/hooks', score: 10 },
       ]);
-      const searchService = createMockSearchService(searchResult);
 
-      const result = await generateCategorySuggestion(
-        ['React'],
-        mockUser,
-        [],
-        searchService,
-      );
+      const result = await generateCategorySuggestion(candidates);
 
       expect(result?.description).toBe('Top-level category: tech-notes');
     });
 
     it('should have label "Save under category"', async () => {
-      const searchResult = createSearchResult([
+      const candidates = createCandidates([
         { path: '/tech-notes/React/hooks', score: 10 },
       ]);
-      const searchService = createMockSearchService(searchResult);
 
-      const result = await generateCategorySuggestion(
-        ['React'],
-        mockUser,
-        [],
-        searchService,
-      );
+      const result = await generateCategorySuggestion(candidates);
 
       expect(result?.label).toBe('Save under category');
     });
 
     it('should resolve grant from top-level directory', async () => {
       mocks.resolveParentGrantMock.mockResolvedValue(GRANT_PUBLIC);
-      const searchResult = createSearchResult([
+      const candidates = createCandidates([
         { path: '/tech-notes/React/hooks', score: 10 },
       ]);
-      const searchService = createMockSearchService(searchResult);
 
-      const result = await generateCategorySuggestion(
-        ['React'],
-        mockUser,
-        [],
-        searchService,
-      );
+      const result = await generateCategorySuggestion(candidates);
 
       expect(mocks.resolveParentGrantMock).toHaveBeenCalledWith('/tech-notes/');
       expect(result?.grant).toBe(GRANT_PUBLIC);
@@ -199,80 +130,23 @@ describe('generateCategorySuggestion', () => {
 
     it('should return GRANT_OWNER when parent page not found', async () => {
       mocks.resolveParentGrantMock.mockResolvedValue(GRANT_OWNER);
-      const searchResult = createSearchResult([
+      const candidates = createCandidates([
         { path: '/nonexistent/page', score: 10 },
       ]);
-      const searchService = createMockSearchService(searchResult);
 
-      const result = await generateCategorySuggestion(
-        ['keyword'],
-        mockUser,
-        [],
-        searchService,
-      );
+      const result = await generateCategorySuggestion(candidates);
 
       expect(result?.grant).toBe(GRANT_OWNER);
     });
-
-    it('should join keywords with spaces for search query', async () => {
-      const searchResult = createSearchResult([
-        { path: '/tech-notes/React/hooks', score: 10 },
-      ]);
-      const searchService = createMockSearchService(searchResult);
-
-      await generateCategorySuggestion(
-        ['React', 'hooks', 'useState'],
-        mockUser,
-        [],
-        searchService,
-      );
-
-      expect(searchService.searchKeyword).toHaveBeenCalledWith(
-        'React hooks useState',
-        null,
-        mockUser,
-        [],
-        expect.objectContaining({ limit: expect.any(Number) }),
-      );
-    });
-
-    it('should pass user and userGroups to searchKeyword', async () => {
-      const searchResult = createSearchResult([
-        { path: '/tech-notes/React/hooks', score: 10 },
-      ]);
-      const searchService = createMockSearchService(searchResult);
-      const mockUserGroups = ['group1', 'group2'];
-
-      await generateCategorySuggestion(
-        ['React'],
-        mockUser,
-        mockUserGroups,
-        searchService,
-      );
-
-      expect(searchService.searchKeyword).toHaveBeenCalledWith(
-        expect.any(String),
-        null,
-        mockUser,
-        mockUserGroups,
-        expect.any(Object),
-      );
-    });
   });
 
   describe('when top result is a single-segment page', () => {
     it('should return the page path as category', async () => {
-      const searchResult = createSearchResult([
+      const candidates = createCandidates([
         { path: '/engineering', score: 10 },
       ]);
-      const searchService = createMockSearchService(searchResult);
 
-      const result = await generateCategorySuggestion(
-        ['keyword'],
-        mockUser,
-        [],
-        searchService,
-      );
+      const result = await generateCategorySuggestion(candidates);
 
       expect(result).not.toBeNull();
       expect(result?.path).toBe('/engineering/');
@@ -280,31 +154,15 @@ describe('generateCategorySuggestion', () => {
     });
   });
 
-  describe('when search returns no results', () => {
+  describe('when candidates are empty', () => {
     it('should return null', async () => {
-      const searchResult = createSearchResult([]);
-      const searchService = createMockSearchService(searchResult);
-
-      const result = await generateCategorySuggestion(
-        ['nonexistent'],
-        mockUser,
-        [],
-        searchService,
-      );
+      const result = await generateCategorySuggestion([]);
 
       expect(result).toBeNull();
     });
 
     it('should not call resolveParentGrant', async () => {
-      const searchResult = createSearchResult([]);
-      const searchService = createMockSearchService(searchResult);
-
-      await generateCategorySuggestion(
-        ['nonexistent'],
-        mockUser,
-        [],
-        searchService,
-      );
+      await generateCategorySuggestion([]);
 
       expect(mocks.resolveParentGrantMock).not.toHaveBeenCalled();
     });

+ 9 - 37
apps/app/src/features/ai-tools/suggest-path/server/services/generate-category-suggestion.ts

@@ -1,65 +1,37 @@
-import type { IUserHasId } from '@growi/core/dist/interfaces';
-
 import type {
   PathSuggestion,
-  SearchService,
+  SearchCandidate,
 } from '../../interfaces/suggest-path-types';
 import { SuggestionType } from '../../interfaces/suggest-path-types';
 import { resolveParentGrant } from './resolve-parent-grant';
 
 const CATEGORY_LABEL = 'Save under category';
-const SEARCH_RESULT_LIMIT = 10;
 
-export function extractTopLevelSegment(pagePath: string): string {
+export function extractTopLevelSegmentName(pagePath: string): string | null {
   const segments = pagePath.split('/').filter(Boolean);
-  if (segments.length === 0) {
-    return '/';
-  }
-  return `/${segments[0]}/`;
-}
-
-export function generateCategoryDescription(topLevelSegment: string): string {
-  return `Top-level category: ${topLevelSegment}`;
+  return segments[0] ?? null;
 }
 
 export const generateCategorySuggestion = async (
-  keywords: string[],
-  user: IUserHasId,
-  userGroups: unknown,
-  searchService: SearchService,
+  candidates: SearchCandidate[],
 ): Promise<PathSuggestion | null> => {
-  const keyword = keywords.join(' ');
-
-  const [searchResult] = await searchService.searchKeyword(
-    keyword,
-    null,
-    user,
-    userGroups,
-    { limit: SEARCH_RESULT_LIMIT },
-  );
-
-  const results = searchResult.data;
-  if (results.length === 0) {
+  if (candidates.length === 0) {
     return null;
   }
 
-  const topResult = results[0];
-  const topLevelPath = extractTopLevelSegment(topResult._source.path);
-
-  // Extract segment name (strip leading/trailing slashes)
-  const segmentName = topLevelPath.replace(/^\/|\/$/g, '');
-  if (segmentName === '') {
+  const segmentName = extractTopLevelSegmentName(candidates[0].pagePath);
+  if (segmentName == null) {
     return null;
   }
 
-  const description = generateCategoryDescription(segmentName);
+  const topLevelPath = `/${segmentName}/`;
   const grant = await resolveParentGrant(topLevelPath);
 
   return {
     type: SuggestionType.CATEGORY,
     path: topLevelPath,
     label: CATEGORY_LABEL,
-    description,
+    description: `Top-level category: ${segmentName}`,
     grant,
   };
 };

+ 11 - 20
apps/app/src/features/ai-tools/suggest-path/server/services/generate-suggestions.spec.ts

@@ -96,11 +96,7 @@ describe('generateSuggestions', () => {
       >(),
     generateCategorySuggestion:
       vi.fn<
-        (
-          keywords: string[],
-          user: IUserHasId,
-          userGroups: unknown,
-        ) => Promise<PathSuggestion | null>
+        (candidates: SearchCandidate[]) => Promise<PathSuggestion | null>
       >(),
     resolveParentGrant: vi.fn<(path: string) => Promise<number>>(),
   });
@@ -209,13 +205,11 @@ describe('generateSuggestions', () => {
       );
     });
 
-    it('should pass keywords from content analysis to generateCategorySuggestion', async () => {
+    it('should pass candidates to generateCategorySuggestion', async () => {
       await callGenerateSuggestions();
 
       expect(mockDeps.generateCategorySuggestion).toHaveBeenCalledWith(
-        ['React', 'hooks'],
-        mockUser,
-        mockUserGroups,
+        mockCandidates,
       );
     });
   });
@@ -244,16 +238,15 @@ describe('generateSuggestions', () => {
       expect(mocks.loggerErrorMock).toHaveBeenCalled();
     });
 
-    it('should return memo + category when search candidate retrieval fails', async () => {
+    it('should fall back to memo only when search candidate retrieval fails', async () => {
       mockDeps.analyzeContent.mockResolvedValue(mockAnalysis);
       mockDeps.retrieveSearchCandidates.mockRejectedValue(
         new Error('Search service down'),
       );
-      mockDeps.generateCategorySuggestion.mockResolvedValue(categorySuggestion);
 
       const result = await callGenerateSuggestions();
 
-      expect(result).toEqual([memoSuggestion, categorySuggestion]);
+      expect(result).toEqual([memoSuggestion]);
       expect(mocks.loggerErrorMock).toHaveBeenCalled();
     });
 
@@ -294,9 +287,6 @@ describe('generateSuggestions', () => {
       mockDeps.retrieveSearchCandidates.mockRejectedValue(
         new Error('Search down'),
       );
-      mockDeps.generateCategorySuggestion.mockRejectedValue(
-        new Error('Category failed'),
-      );
 
       const result = await callGenerateSuggestions();
 
@@ -306,11 +296,11 @@ describe('generateSuggestions', () => {
     it('should skip search suggestions when no candidates pass threshold (empty array)', async () => {
       mockDeps.analyzeContent.mockResolvedValue(mockAnalysis);
       mockDeps.retrieveSearchCandidates.mockResolvedValue([]);
-      mockDeps.generateCategorySuggestion.mockResolvedValue(categorySuggestion);
+      mockDeps.generateCategorySuggestion.mockResolvedValue(null);
 
       const result = await callGenerateSuggestions();
 
-      expect(result).toEqual([memoSuggestion, categorySuggestion]);
+      expect(result).toEqual([memoSuggestion]);
       expect(mockDeps.evaluateCandidates).not.toHaveBeenCalled();
     });
 
@@ -348,10 +338,11 @@ describe('generateSuggestions', () => {
   });
 
   describe('parallel execution', () => {
-    it('should run search-evaluate pipeline and category generation independently', async () => {
+    it('should run evaluate pipeline and category generation independently', async () => {
       mockDeps.analyzeContent.mockResolvedValue(mockAnalysis);
-      mockDeps.retrieveSearchCandidates.mockRejectedValue(
-        new Error('Search down'),
+      mockDeps.retrieveSearchCandidates.mockResolvedValue(mockCandidates);
+      mockDeps.evaluateCandidates.mockRejectedValue(
+        new Error('Evaluate failed'),
       );
       mockDeps.generateCategorySuggestion.mockResolvedValue(categorySuggestion);
 

+ 47 - 39
apps/app/src/features/ai-tools/suggest-path/server/services/generate-suggestions.ts

@@ -28,9 +28,7 @@ export type GenerateSuggestionsDeps = {
     candidates: SearchCandidate[],
   ) => Promise<EvaluatedSuggestion[]>;
   generateCategorySuggestion: (
-    keywords: string[],
-    user: IUserHasId,
-    userGroups: unknown,
+    candidates: SearchCandidate[],
   ) => Promise<PathSuggestion | null>;
   resolveParentGrant: (path: string) => Promise<number>;
 };
@@ -52,47 +50,57 @@ export const generateSuggestions = async (
     return [memoSuggestion];
   }
 
-  // Run search-evaluate pipeline and category generation in parallel
-  const [searchResult, categoryResult] = await Promise.allSettled([
-    // Search-evaluate pipeline: search → evaluate → grant resolution
-    (async (): Promise<PathSuggestion[]> => {
-      const candidates = await deps.retrieveSearchCandidates(
-        analysis.keywords,
-        user,
-        userGroups,
-      );
-      if (candidates.length === 0) {
-        return [];
-      }
-      const evaluated = await deps.evaluateCandidates(
-        body,
-        analysis,
-        candidates,
-      );
-      return Promise.all(
-        evaluated.map(async (s): Promise<PathSuggestion> => {
-          const grant = await deps.resolveParentGrant(s.path);
-          return {
-            type: SuggestionType.SEARCH,
-            path: s.path,
-            label: s.label,
-            description: s.description,
-            grant,
-            informationType: analysis.informationType,
-          };
-        }),
-      );
-    })(),
-    // Category generation (parallel, independent)
-    deps.generateCategorySuggestion(analysis.keywords, user, userGroups),
+  // Retrieve search candidates (single ES query, shared by evaluate and category)
+  let candidates: SearchCandidate[];
+  try {
+    candidates = await deps.retrieveSearchCandidates(
+      analysis.keywords,
+      user,
+      userGroups,
+    );
+  } catch (err) {
+    logger.error(
+      'Search candidate retrieval failed, falling back to memo only:',
+      err,
+    );
+    return [memoSuggestion];
+  }
+
+  // Run evaluate pipeline and category generation in parallel
+  const [evaluateResult, categoryResult] = await Promise.allSettled([
+    // Evaluate pipeline: evaluate → grant resolution (skip if no candidates)
+    candidates.length > 0
+      ? (async (): Promise<PathSuggestion[]> => {
+          const evaluated = await deps.evaluateCandidates(
+            body,
+            analysis,
+            candidates,
+          );
+          return Promise.all(
+            evaluated.map(async (s): Promise<PathSuggestion> => {
+              const grant = await deps.resolveParentGrant(s.path);
+              return {
+                type: SuggestionType.SEARCH,
+                path: s.path,
+                label: s.label,
+                description: s.description,
+                grant,
+                informationType: analysis.informationType,
+              };
+            }),
+          );
+        })()
+      : Promise.resolve([]),
+    // Category generation (uses same candidates, no extra ES query)
+    deps.generateCategorySuggestion(candidates),
   ]);
 
   const suggestions: PathSuggestion[] = [memoSuggestion];
 
-  if (searchResult.status === 'fulfilled') {
-    suggestions.push(...searchResult.value);
+  if (evaluateResult.status === 'fulfilled') {
+    suggestions.push(...evaluateResult.value);
   } else {
-    logger.error('Search-evaluate pipeline failed:', searchResult.reason);
+    logger.error('Evaluate pipeline failed:', evaluateResult.reason);
   }
 
   if (categoryResult.status === 'fulfilled' && categoryResult.value != null) {