filter out demo spaces for unauthenticated users search in elastic #4712
Conversation
WalkthroughThe pull request introduces modifications to the search functionality within the API. It updates the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/services/api/search/v2/extract/build.search.query.ts (2)
21-25: Consider adding parameter validationThe filter parameters should be validated before being used in the query construction.
export const buildSearchQuery = ( terms: string, spaceIdFilter?: string, onlyPublicResults: boolean = false -): QueryDslQueryContainer => ({ +): QueryDslQueryContainer => { + if (!terms?.trim()) { + throw new Error('Search terms cannot be empty'); + } + return { bool: { must: [ { multi_match: { query: terms, type: 'most_fields', fields: ['*'], }, }, ], filter: buildFilter( spaceIdFilter, onlyPublicResults === true ? SpaceVisibility.ACTIVE : undefined ), }, -}); + }; +};
33-59: Consider type narrowing for better type safetyThe filters array could benefit from more specific typing.
- const filters: QueryDslQueryContainer[] = []; + const filters: Required<QueryDslQueryContainer>[] = [];src/services/api/search/v2/extract/search.extract.service.ts (2)
88-92: Add type safety for the onlyPublicResults parameterWhile the implementation correctly passes the parameter, consider adding type safety:
- public async search( - searchData: SearchInput, - onlyPublicResults: boolean - ): Promise<ISearchResult[] | never> { + public async search( + searchData: SearchInput, + onlyPublicResults: boolean = false + ): Promise<ISearchResult[] | never> {
88-92: Consider adding caching for search resultsFor frequently accessed search results, especially with the public filter, consider implementing caching to improve performance.
You could:
- Use NestJS's built-in cache manager
- Implement cache invalidation when spaces/posts are updated
- Cache results with a reasonable TTL based on update frequency
Example implementation:
@Injectable() export class SearchExtractService { constructor( @Inject(CACHE_MANAGER) private cacheManager: Cache, // ... other dependencies ) {} private getCacheKey(searchData: SearchInput, onlyPublicResults: boolean): string { return `search:${JSON.stringify(searchData)}:${onlyPublicResults}`; } public async search( searchData: SearchInput, onlyPublicResults: boolean ): Promise<ISearchResult[] | never> { const cacheKey = this.getCacheKey(searchData, onlyPublicResults); const cached = await this.cacheManager.get(cacheKey); if (cached) return cached; // ... existing search logic ... await this.cacheManager.set(cacheKey, results, 3600); // Cache for 1 hour return results; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/services/api/search/v2/extract/build.search.query.ts(1 hunks)src/services/api/search/v2/extract/search.extract.service.ts(1 hunks)src/services/api/search/v2/result/search.result.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/services/api/search/v2/extract/build.search.query.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/api/search/v2/extract/search.extract.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/api/search/v2/result/search.result.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🔇 Additional comments (6)
src/services/api/search/v2/extract/build.search.query.ts (3)
1-7: LGTM! Clean function signature update with proper typing
The addition of the onlyPublicResults parameter with a default value maintains backward compatibility while enabling new filtering functionality.
12-18: LGTM! Effective full-text search implementation
The multi_match query with most_fields type appropriately accumulates scores across all text fields, providing relevant search results.
61-77: Verify visibility filter implementation across the codebase
The visibility filter implementation seems correct, but let's verify its usage across the codebase to ensure consistent behavior.
✅ Verification successful
Visibility filter implementation is consistent and well-structured
The visibility filter implementation in build.search.query.ts aligns with the codebase's visibility handling patterns:
- Correctly uses the
SpaceVisibilityenum defined in@common/enums/space.visibility - Follows the same visibility filtering approach used in
SpaceFilterService - Properly handles visibility in search queries with appropriate Elasticsearch term filters
- Maintains consistency with the visibility model used across innovation hubs and spaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other visibility filter implementations and usages
# Check for other visibility filter implementations
rg -l "visibility.*filter" --type ts
# Check for SpaceVisibility enum usage in filters
ast-grep --pattern 'SpaceVisibility.$_'
# Check for similar filter building patterns
rg -A 5 'buildFilter.*visibility'
Length of output: 5397
Script:
#!/bin/bash
# Let's check the visibility filter implementation in the found file
cat src/domain/innovation-hub/innovation.hub.interface.ts
# Let's also check the space filter service implementation
cat src/services/infrastructure/space-filter/space.filter.service.ts
# Check for visibility enum definition
rg -A 5 "enum.*SpaceVisibility"
# Check the context of visibility filter usage in search
cat src/services/api/search/v2/extract/build.search.query.ts
Length of output: 5071
src/services/api/search/v2/extract/search.extract.service.ts (2)
Line range hint 176-183: LGTM: Public indices filtering implementation
The implementation correctly filters indices for public results using lodash's intersection. This ensures that only allowed indices are searched when onlyPublicResults is true.
Line range hint 171-174: Review whiteboard handling implementation
There's a TODO comment indicating that whiteboard handling is temporary. This technical debt should be tracked.
Would you like me to create a new issue to track the whiteboard separation task?
✅ Verification successful
Technical debt for whiteboard handling is already tracked in issue #4252
The TODO comment about whiteboard handling corresponds to an existing issue that is actively tracking this technical debt. Issue #4252 "Whiteboards as a separate search result" comprehensively covers the need to separate whiteboards in search results and includes clear acceptance criteria.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's an existing issue for whiteboard separation
gh issue list --search "whiteboard separate search result" --json number,title,body
Length of output: 1190
src/services/api/search/v2/result/search.result.service.ts (1)
185-187: LGTM! The formatting changes improve readability.
The alignment of the where clause improves code consistency and readability.
Let's verify that the demo space filtering logic is implemented in the search query builder:
✅ Verification successful
The formatting changes are safe, and demo space filtering is properly implemented.
Based on the search results, I can confirm that demo space filtering is implemented in multiple places:
- In
src/services/api/search/v2/extract/build.search.query.ts, the visibility filter is applied through thebuildSearchQueryfunction which acceptsonlyPublicResultsparameter to filter bySpaceVisibility.ACTIVE - In
src/services/api/search/v2/extract/function.score.functions.ts, demo spaces are given lower weight (weight: 1) compared to active spaces (weight: 2) - The search query construction includes visibility filters in the elastic search query through
mustandfilterclauses
The formatting changes in the review are purely cosmetic and do not impact the functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of demo space filtering logic
# Expected: Find the filtering logic in the search query builder
# Search for demo space filtering in search-related files
rg -g "src/services/api/search/**" -l "demo.*space|visibility.*filter"
# Search for buildSearchQuery implementation
ast-grep --pattern 'function buildSearchQuery($_) {
$$$
}'
Length of output: 135
Script:
#!/bin/bash
# Let's try different search patterns and look for query building logic
# Search for any query building or filtering logic in search-related files
rg -g "src/services/api/search/**" "buildQuery|createQuery|filterQuery|searchQuery" -A 5
# Look for any visibility or space-related filtering
rg -g "src/services/api/search/**" "isPublic|visibility|spaceType" -A 5
# Search for elastic search query construction
rg -g "src/services/api/search/**" "elasticSearch|searchRequest|must_not|must|should" -A 5
Length of output: 33704
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/services/api/search/v2/search2.service.ts (1)
Line range hint
1-55: Well-structured service implementation following NestJS best practices.The service demonstrates good architectural patterns:
- Proper dependency injection using
@Injectable()and constructor injection- Clear separation of concerns between search, extract, and result services
- Robust error handling with custom exceptions and logging context
src/services/api/search/v2/extract/build.search.query.ts (2)
1-10: LGTM! Consider making the options parameter more type-safe.The type definitions and imports are correct. The function signature is well-structured with proper TypeScript types.
Consider creating a dedicated interface for the options parameter to improve type safety and reusability:
interface SearchQueryOptions { spaceIdFilter?: string; excludeDemoSpaces?: boolean; }
11-32: Consider optimizing the multi_match query configuration.The implementation is solid, with good use of TypeScript features and proper Elasticsearch query construction. However, the multi_match query could be optimized.
Consider specifying explicit field weights and analyzer settings in the multi_match query for better relevance:
multi_match: { query: terms, type: 'most_fields', - fields: ['*'], + fields: [ + 'name^3', + 'description^2', + '*' + ], + analyzer: 'standard' },This would give higher relevance to matches in name and description fields while still searching across all fields.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/services/api/search/v2/extract/build.search.query.ts(1 hunks)src/services/api/search/v2/extract/search.extract.service.ts(2 hunks)src/services/api/search/v2/search2.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/api/search/v2/extract/search.extract.service.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/services/api/search/v2/extract/build.search.query.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/api/search/v2/search2.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🔇 Additional comments (3)
src/services/api/search/v2/search2.service.ts (1)
48-48: Verify parameter handling in SearchExtractService.
Let's ensure that the searchExtractService.search() method properly handles the excludeDemoSpaces parameter in its implementation.
✅ Verification successful
Parameter excludeDemoSpaces is properly handled in SearchExtractService
The excludeDemoSpaces parameter is correctly implemented in the SearchExtractService:
- It's properly passed to the
buildSearchQueryfunction to filter results - It's used in
getIndicesmethod to restrict search to public indices when true - The parameter flows through the entire search pipeline affecting both query building and index selection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the search method implementation in SearchExtractService
# Find the search method implementation
ast-grep --pattern 'class SearchExtractService {
$$$
search($$$, excludeDemoSpaces$$$) {
$$$
}
$$$
}'
Length of output: 115
Script:
#!/bin/bash
# Let's try a broader search to find the SearchExtractService class and its implementation
# First, let's find files containing SearchExtractService
rg "class SearchExtractService" -l
# Then let's look for the search method implementation with more context
rg "search.*excludeDemoSpaces" -A 10
Length of output: 136
Script:
#!/bin/bash
# Now that we found the file, let's examine its content
cat src/services/api/search/v2/extract/search.extract.service.ts
Length of output: 7053
src/services/api/search/v2/extract/build.search.query.ts (2)
34-37: LGTM! Function signature is well-defined.
The function signature is consistent with buildSearchQuery and properly handles the case where no filters are applied.
38-86: Verify space visibility handling and consider input validation.
The filter construction logic is well-implemented with good comments explaining the approach. However, there are a few points to consider:
Let's verify if we need to handle additional visibility states:
Consider adding these improvements:
- Add input validation for spaceIdFilter format
- Consider handling multiple visibility states if needed
const buildFilter = (opts?: {
spaceIdFilter?: string;
excludeDemoSpaces?: boolean;
}): QueryDslQueryContainer | undefined => {
const { spaceIdFilter, excludeDemoSpaces } = opts ?? {};
+
+ // Validate spaceIdFilter format if provided
+ if (spaceIdFilter && !/^[a-zA-Z0-9-]+$/.test(spaceIdFilter)) {
+ throw new Error('Invalid space ID format');
+ }
const filters: QueryDslQueryContainer[] = [];✅ Verification successful
Based on the search results, I can now generate the final response since I have enough context about the SpaceVisibility usage across the codebase.
The current visibility handling is correct and complete
The current implementation of excludeDemoSpaces using SpaceVisibility.ACTIVE is appropriate for the search query builder. Here's why:
- The codebase consistently treats
ACTIVEspaces as the primary visibility state - When demo spaces need to be excluded, filtering for
ACTIVEis the correct approach ARCHIVEDspaces are handled separately in the ingestion phase (as seen in search.ingest.service.ts)- The visibility states are well-defined:
ACTIVE,DEMO, andARCHIVED
The suggestion to add input validation for spaceIdFilter remains valid, but the visibility handling needs no changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of SpaceVisibility to ensure we're handling all relevant cases
rg -A 2 "SpaceVisibility\." --type ts
Length of output: 12440
…4712) * filter out demo spaces for unauthenticated users search in elastic * improvements --------- Co-authored-by: Svetoslav <[email protected]>
* initial setup of invoking engines and receiving reponses trough CQRS * adding the new mesages/handlers * rename question to input and ask to invoke related to VCs adds room controller to be used by the server post replies to rooms and threads * complete implementation for the Expert engine * fix build * address comments and minor cleanups * change ask guidence question query to mutation back the guidance chat with a room implement guidence communication thorugh the event bus * update quickstart services * handle guidance room creaton trough the room service * Generate room on demand * Chat guidance room created only when needed * pending reset chat * it was already done * address comments * address some comments * Entitlements + license services (#4593) * first pass at two new modules for TemplatesManager, TemplateDefault * added templates manager to space; removed the SpaceDefaults entity (module still present until move all data to be via defaults * added templatesManager to platform * moved creating of default innovatin flow input to space defaults * back out space type on Template; tidy up Template module to use switch statements * created template applier module * tidy up naming * updated set of default template types * fixed circular dependency; moved logic for creating collaboration input to space defaults * removed loading of defaults from files for collaboration content * removed code based addition of callouts, innovation flow states * tidy up naming * added loading of default templates at platform level in to bootstrap * removed option to create new innovation flow template * added in migration: * loading in templates on bootstrap * added field for collaboration templates on templatesSet; added lookup for templatesManager * added mutation to create template from collaboration; added logic to prevent template used as default to be deleted; fixed removal of template set on template manager * initial creation of license + entitlements modules * add license into account * updated account to have license service + use that in mutations checking limits, removing notion of soft limits * ensure data is loaded properly on account for license checking * added mutation to reset the license calculations on account, including new auth privilege to be able to do so * renamed Licensing module to LicensingFramework module; trigger license reset on Account after assigning / removing license * removed usage of LicenseEngine outside of license services on space or account * renamed entitlement to licenseEntitlement as entity; first pass at migration * fixed issues in migration * fixed issues related to auth reset; tidied up loader creator imports * fixed auth cascade for templates of type post * license reset running * reset licenses on space after adding / removing license plans * removed need for license check in community; added entitlement check in roleset when adding a VC * remove auth reset when assigning / removing license plans * added License to RoleSet * added license to collaboration * tidied up retrieval of license for whiteboard; added license to collaboration in migration * fix typo; fix space spec file * fix additional tests * moved tempaltesManager to last migration in the list * fixed retrieval of template when creating collaboration * added logging * fixed bootstrap setting of templates * refactored inputCreator to do the data loading closer to usage; fixed picking up of templates; fixed bootstrap usage of templates * added ability to retrieve limits on entitlements + current usage * updated field names on entitlements * updated field names on entitlements * fixed account mutaiton logic bug * ensure that licenses are reset when assigning beta tester or vc campaign role to a user * added reset all account licenses mutation * fixed bug on space entitlements; refactored code to reduce duplication * fixed url generation for templates inside of TempaltesManager * fixed bootstrap order to create forum earlier * ensure collaboration creation on template provides some defaults for callouts * fix deletion of templates of type post * ensure more data is defaulted inside of template service for collaboration; add setting of isTemplate field on Collaboration, and also on contained Callouts * ensure isTempalte is passed to Collaboration entity * fixed groups in bootstrap space template; updated signature for creating callout from collaboration * fixed missing field * fixed type on mutation to create from collaboration * fixed typo * fixed groups in bootstrap space template; updated signature for creating callout from collaboration * fixed missing field * fixed type on mutation to create from collaboration * fixed typo * reworked applying collaboraiton template to collaboration * improved error message in wrong type of ID passed in * fixed build * made migration last in the list * rename migration to be last * removed read check when looking up collaboration * track free / plus / premium space entitlements separately * updated migration order * removed duplicate migration * moved auth reset to mutation for applying the template to another collaboration * extend lookup of entitlement usage to cover new types * updaed license policy to reflect new entitlements; made license engine work with entitlements, not license privileges; removed license privilege (no longer relevant) * updated migration to not drop indexes already removed * fix for license reset on space * added license policy rule for free space credential * ensure license entitlements are reset as part of the bootstrap * fixed typo * extended reset all to include resetting licenses on accounts + AI server; moved migration to be last * Address pr comment * Address PR feedback * Address PR comment * Address PR comments * Address PR comments * Address PR comment Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Improved types & naming * Address PR comments * Fixed switch-case logic in entitlements * Converge entitlements schema * Remove unused AuthorizationPrivilege --------- Co-authored-by: Carlos Cano <[email protected]> Co-authored-by: Valentin Yanakiev <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Major version bump * Add license lookup * removed redundant checks * Fixed proper L0 space agent selection in recursive license assignment (#4708) * Fixed proper L0 account agent selection in recursive license assignment * provide root space agent for sub-sub space * Look up correct entitlement for VC creation (#4707) Co-authored-by: Evgeni Dimitrov <[email protected]> * filter out demo spaces for unauthenticated users search in elastic (#4712) * filter out demo spaces for unauthenticated users search in elastic * improvements --------- Co-authored-by: Svetoslav <[email protected]> * get guidance room from user and not from me * Finish * wrapping up * fix roomId missing * Put the GuidanceVC into the platform * Guidance VC created on bootstrap * add support for guidance ingest * cleanup and handle sources label * fix build * address comments and update engine images --------- Co-authored-by: Valentin Yanakiev <[email protected]> Co-authored-by: Carlos Cano <[email protected]> Co-authored-by: Neil Smyth <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Evgeni Dimitrov <[email protected]> Co-authored-by: Svetoslav <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
Chores