[WEB-2573] improvement: search-issues API optimization.#5727
[WEB-2573] improvement: search-issues API optimization.#5727
Conversation
* limit search results to 100 issues.
WalkthroughThe changes involve modifications to the filtering logic for issues within the Changes
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apiserver/plane/app/views/search/issue.py (3)
Line range hint
70-91: Add pagination to handle larger datasets gracefullyWhile limiting the results to 100 items improves performance, implementing pagination would offer a more flexible and user-friendly solution. This allows clients to request specific pages of results and manage data more efficiently, especially if the total number of issues exceeds the limit.
Consider integrating Django's pagination utilities. Here's an example of how you might modify the code:
from django.core.paginator import Paginator def get(self, request, slug, project_id): # ... (existing code) page_number = request.query_params.get('page', 1) page_size = request.query_params.get('page_size', 20) paginator = Paginator(issues.values( "name", "id", "start_date", "sequence_id", "project__name", "project__identifier", "project_id", "workspace__slug", "state__name", "state__group", "state__color", ), page_size) page_obj = paginator.get_page(page_number) return Response( { 'results': list(page_obj), 'count': paginator.count, 'num_pages': paginator.num_pages, 'current_page': page_obj.number, }, status=status.HTTP_200_OK, )This approach provides clients with additional context about the data and allows them to navigate through the results.
Line range hint
57-69: Handle exceptions when retrieving issue objectsIn the blocks where you retrieve an issue using
Issue.issue_objects.get(pk=issue_id), there's a possibility of aDoesNotExistexception if theissue_idis invalid or does not exist. This could lead to a server error.Consider adding exception handling to manage this scenario gracefully:
from django.shortcuts import get_object_or_404 if parent == "true" and issue_id: issue = get_object_or_404(Issue.issue_objects, pk=issue_id) issues = issues.filter( ~Q(pk=issue_id), ~Q(pk=issue.parent_id), ~Q(parent_id=issue_id) )Repeat similar changes for other instances where
issue = Issue.issue_objects.get(pk=issue_id)is called.
Line range hint
53-55: Ensure consistent boolean parameter handlingIn the line
target_date = request.query_params.get("target_date", True), the default value is set toTrue, which is a boolean, butrequest.query_params.getreturns a string orNone. This might lead to inconsistencies when comparingtarget_datelater.Consider using
"true"or"false"as default values to maintain consistency:- target_date = request.query_params.get("target_date", True) + target_date = request.query_params.get("target_date", "true")Alternatively, explicitly convert the parameter to a boolean where it's used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apiserver/plane/app/views/search/base.py (1 hunks)
- apiserver/plane/app/views/search/issue.py (1 hunks)
🔇 Additional comments (5)
apiserver/plane/app/views/search/base.py (3)
93-93: 🛠️ Refactor suggestionEnsure consistency across filter methods and update response structure
While the limitation of 100 results has been applied to the
filter_issuesmethod, it's important to ensure consistency across all filter methods in theGlobalSearchEndpointclass. Additionally, the current response structure in thegetmethod doesn't account for potentially truncated results.Consider the following improvements:
- Apply similar result limitations to other filter methods for consistency.
- Update the
getmethod to handle and communicate truncated results.Here's a suggested refactor for the
getmethod:def get(self, request, slug): # ... (existing parameter handling) results = {} truncated = {} for model, func in MODELS_MAPPER.items(): model_results = func(query, slug, project_id, workspace_search) if isinstance(model_results, dict) and 'truncated' in model_results: results[model] = model_results['results'] truncated[model] = model_results['truncated'] else: results[model] = model_results truncated[model] = False return Response({ "results": results, "truncated": truncated }, status=status.HTTP_200_OK)This refactor:
- Allows individual filter methods to return both results and a truncation flag.
- Includes a 'truncated' object in the response to indicate which result sets were limited.
To ensure this change is implemented correctly and doesn't break existing functionality, please run the following verification:
#!/bin/bash # Check all filter methods for consistent result limiting grep -r "def filter_" . # Verify the structure of the Response object in the get method grep -r "return Response(" . # Look for any external code that might be relying on the current response structure grep -r "results.*:" .
Line range hint
1-93: Overall assessment: Good start, but consider broader optimizationsThe change to limit the results in
filter_issuesis a step in the right direction for optimizing the search-issues API. However, there are opportunities for further improvement:
- Consistency: Consider applying similar result limitations to all filter methods in the
GlobalSearchEndpointclass.- Flexibility: Make the result limit configurable rather than hardcoded.
- Efficiency: Apply the limit earlier in the query chain for better database performance.
- Transparency: Update the response structure to indicate when results are truncated.
These improvements would make the API more robust, efficient, and user-friendly. They would also better align with the PR's objective of optimizing the search functionality.
For a more comprehensive optimization:
- Consider implementing pagination for all filter methods. This would provide a consistent interface for handling large result sets across all search types.
- Evaluate the performance impact of the current filtering approach, especially for large datasets. You might want to consider using database-specific optimizations or full-text search capabilities for better performance.
- Think about caching frequently accessed search results to reduce database load.
To ensure that these suggestions don't conflict with existing architecture or requirements, please run the following checks:
#!/bin/bash # Check for any existing pagination implementations grep -r "paginate" . # Look for any performance-related comments or TODOs grep -ri "todo.*performance" . grep -ri "optimize" . # Check for any caching mechanisms already in place grep -r "cache" .These checks will help identify any existing work or considerations related to the suggested improvements.
93-93: 🛠️ Refactor suggestionConsider refining the result limitation approach
The addition of
[:100]to limit the results aligns with the PR objective of optimizing the search-issues API. This change can improve performance by reducing the amount of data returned. However, there are some considerations:
- The hardcoded limit of 100 might not be flexible for different use cases. Consider making this a configurable parameter.
- There's no indication to the client whether there are more results beyond this limit. It might be helpful to include a flag in the response indicating if the results were truncated.
- Applying the limit after all filtering and distinct operations might not be the most efficient approach for very large datasets. Consider using database-level limitations (e.g.,
LIMITin SQL) earlier in the query chain.Here's a suggested refactor to address these points:
from django.conf import settings class GlobalSearchEndpoint(BaseAPIView): # ... (other methods) def filter_issues(self, query, slug, project_id, workspace_search): # ... (existing filtering logic) issues = Issue.issue_objects.filter( q, project__project_projectmember__member=self.request.user, project__project_projectmember__is_active=True, project__archived_at__isnull=True, workspace__slug=slug, ) if workspace_search == "false" and project_id: issues = issues.filter(project_id=project_id) # Apply limit earlier in the query limit = getattr(settings, 'ISSUE_SEARCH_LIMIT', 100) issues = issues.distinct()[:limit] results = list(issues.values( "name", "id", "sequence_id", "project__identifier", "project_id", "workspace__slug", )) return { 'results': results, 'truncated': len(results) == limit }This refactor:
- Uses a configurable limit from settings.
- Applies the limit earlier in the query chain.
- Includes a 'truncated' flag to indicate if there are more results.
To ensure this change doesn't break existing functionality, please run the following verification:
apiserver/plane/app/views/search/issue.py (2)
Line range hint
95-99: Ensure role comparison uses the correct data typeIn the condition checking the user's role:
if ProjectMember.objects.filter( project_id=project_id, member=self.request.user, is_active=True, role=5 ).exists():Ensure that the role comparison matches the data type used for role definitions. If
roleis defined as a string elsewhere, comparing it to an integer might cause unexpected behavior.Please confirm that
roleis indeed an integer field and that5corresponds to the intended role.
Line range hint
86-88: Clarify the exclusion logic formoduleparameterWhen excluding issues related to a specific module:
if module: issues = issues.exclude(issue_module__module=module)Consider whether
moduleshould filter issues belonging to that module instead of excluding them. Excluding might not be the intended behavior if the goal is to search within a specific module.Please verify if the exclusion is intentional. If you want to filter issues within the specified module, modify the code as follows:
- issues = issues.exclude(issue_module__module=module) + issues = issues.filter(issue_module__module=module)
3 participants
Summary by CodeRabbit
New Features
Bug Fixes