Enhanced java parsing support #210
Conversation
|
Warning Rate limit exceeded@dhirenmathur has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes involve multiple files across the application, primarily focusing on enhancing the graph construction and parsing logic. Key modifications include the introduction of new helper methods for relationship creation in the Changes
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 (9)
app/modules/parsing/graph_construction/code_graph_service.py (1)
65-93: Enhance error handling and code clarity in node processing.
- Log or handle cases where
node_typeisUNKNOWNto assist in debugging.- Consolidate labels and node data preparation for better readability.
- Ensure that all node types are accounted for or explicitly ignored.
Consider refactoring the code as follows:
for node_id, node_data in batch_nodes: node_type = node_data.get("type", "UNKNOWN") if node_type == "UNKNOWN": logging.warning(f"Unknown node type for node {node_id}") continue # Initialize labels labels = ["NODE", node_type] if node_type in ["FILE", "CLASS", "FUNCTION", "INTERFACE"] else ["NODE"] # Prepare node data processed_node = { "name": node_data.get("name", node_id), "file_path": node_data.get("file", ""), "start_line": node_data.get("line", -1), "end_line": node_data.get("end_line", -1), "repoId": project_id, "node_id": CodeGraphService.generate_node_id(node_id, user_id), "entityId": user_id, "type": node_type, "text": node_data.get("text", ""), "labels": labels, } # Remove None and empty string values processed_node = {k: v for k, v in processed_node.items() if v not in (None, "")} nodes_to_create.append(processed_node)app/modules/parsing/graph_construction/parsing_repomap.py (5)
165-199: ReplaceUsing
loggingmodule to control the verbosity level.Apply this diff to replace
logging.debug:- print(f"Processing file: {fname}") - print(f"Language detected: {lang}") ... - print(f"Captured node: {node_text} with tag: {tag}") ... - print(f"Found method definition: {node_text}") ... - print(f"Found method reference: {node_text}") + logging.debug(f"Processing file: {fname}") + logging.debug(f"Language detected: {lang}") ... + logging.debug(f"Captured node: {node_text} with tag: {tag}") ... + logging.debug(f"Found method definition: {node_text}") ... + logging.debug(f"Found method reference: {node_text}")
571-584: Simplify nestedifstatements by combining conditions.The nested
ifstatements can be combined using logicaloroperators to improve readability.Apply this diff:
- if relationship_type == "REFERENCES": - if (source_data.get('type') == 'FUNCTION' and - target_data.get('type') == 'FUNCTION' and - 'Impl' in source): # Implementation class - valid_direction = True - elif source_data.get('type') == 'FUNCTION': - valid_direction = True - elif target_data.get('type') == 'CLASS': - valid_direction = True + if relationship_type == "REFERENCES" and ( + (source_data.get('type') == 'FUNCTION' and target_data.get('type') == 'FUNCTION' and 'Impl' in source) or + source_data.get('type') == 'FUNCTION' or + target_data.get('type') == 'CLASS' + ): + valid_direction = True🧰 Tools
🪛 Ruff (0.8.2)
572-579: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
578-583: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
602-602: Rename unused loop variabledirsto_to indicate it's unused.In the loop at line 602, the
dirsvariable isn't used within the loop body.Apply this diff:
- for root, dirs, files in os.walk(repo_dir): + for root, _, files in os.walk(repo_dir):🧰 Tools
🪛 Ruff (0.8.2)
602-602: Loop control variable
dirsnot used within loop bodyRename unused
dirsto_dirs(B007)
704-704: Rename unused loop variablessrc_classandsrc_methodto_to indicate they are unused.In the loop at line 704,
src_classandsrc_methodare not used.Apply this diff:
- for source, line, end_line, src_class, src_method in refs: + for source, line, end_line, _, _ in refs:🧰 Tools
🪛 Ruff (0.8.2)
704-704: Loop control variable
src_classnot used within loop bodyRename unused
src_classto_src_class(B007)
704-704: Loop control variable
src_methodnot used within loop bodyRename unused
src_methodto_src_method(B007)
751-754: Simplifyifstatements infind_node_by_rangemethod.Combine the nested
ifconditions using logicalorfor clarity.Apply this diff:
- if node_type == "FUNCTION" and node.type in ["function_definition", "method","method_declaration", "function"]: - return node - elif node_type in ["CLASS", "INTERFACE"] and node.type in ["class_definition", "interface", "class", "class_declaration", "interface_declaration"]: - return node + if ( + (node_type == "FUNCTION" and node.type in ["function_definition", "method", "method_declaration", "function"]) or + (node_type in ["CLASS", "INTERFACE"] and node.type in ["class_definition", "interface", "class", "class_declaration", "interface_declaration"]) + ): + return node🧰 Tools
🪛 Ruff (0.8.2)
751-754: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
app/modules/auth/auth_service.py (1)
Line range hint
52-61: Ensure mock authentication is securely managed and disabled in production.The introduction of a development mode mock authentication is useful for testing. However, to prevent security risks:
- Make sure the
isDevelopmentModeflag cannot be enabled accidentally in production environments.- Consider using environment variables that are securely managed and distinct between development and production.
- Add checks or safeguards to prevent misuse.
🧰 Tools
🪛 Ruff (0.8.2)
49-49: Do not perform function call
HTTPBearerin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
54-54: Use capitalized environment variable
ISDEVELOPMENTMODEinstead ofisDevelopmentMode(SIM112)
55-55: Use capitalized environment variable
DEFAULTUSERNAMEinstead ofdefaultUsername(SIM112)
app/modules/parsing/graph_construction/parsing_controller.py (2)
Line range hint
1-250: Consider the following architectural improvements:
- Extract duplicate repo_name derivation logic into a helper method
- Move demo repository handling logic into a separate method
- Add error handling for async tasks
Here's a suggested refactor for the repo_name derivation:
@staticmethod def _derive_repo_name(repo_details: ParsingRequest) -> str: return repo_details.repo_name or repo_details.repo_path.split("/")[-1]This would help maintain consistency and make the code more DRY. Would you like me to provide more detailed refactoring suggestions for the other improvements?
Line range hint
206-208: Improve async task error handling.The async task creation for
get_project_structure_asynclacks error handling. Consider wrapping it in a try-catch block and implementing a proper error handling strategy.Here's a suggested improvement:
-asyncio.create_task( - CodeProviderService(db).get_project_structure_async(new_project_id) -) +async def handle_project_structure(): + try: + await CodeProviderService(db).get_project_structure_async(new_project_id) + except Exception as e: + logger.error(f"Failed to get project structure: {e}") + await project_manager.update_project_status( + new_project_id, + ProjectStatusEnum.ERROR + ) + +asyncio.create_task(handle_project_structure())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/modules/auth/auth_service.py(1 hunks)app/modules/parsing/graph_construction/code_graph_service.py(4 hunks)app/modules/parsing/graph_construction/parsing_controller.py(1 hunks)app/modules/parsing/graph_construction/parsing_repomap.py(3 hunks)app/modules/parsing/graph_construction/queries/tree-sitter-java-tags.scm(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
app/modules/parsing/graph_construction/parsing_repomap.py
572-579: Combine if branches using logical or operator
Combine if branches
(SIM114)
578-583: Combine if branches using logical or operator
Combine if branches
(SIM114)
602-602: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
704-704: Loop control variable src_class not used within loop body
Rename unused src_class to _src_class
(B007)
704-704: Loop control variable src_method not used within loop body
Rename unused src_method to _src_method
(B007)
751-754: Combine if branches using logical or operator
Combine if branches
(SIM114)
🔇 Additional comments (4)
app/modules/parsing/graph_construction/code_graph_service.py (2)
4-4: Importing the 'time' module for performance logging is appropriate.
134-135: Logging the elapsed time is helpful for performance monitoring.
app/modules/parsing/graph_construction/queries/tree-sitter-java-tags.scm (1)
1-35: Tree-sitter query additions are correctly structured.
The added queries for methods, classes, interfaces, and variable declarations enhance the parsing capabilities for Java code and are well-defined.
app/modules/parsing/graph_construction/parsing_controller.py (1)
Line range hint 198-204: LGTM! Moving repo_name assignment improves code clarity.
The change to move the repo_name assignment before its usage in register_project is a good improvement. It ensures the variable is defined before use and makes the code flow more logical.
Let's verify that this pattern is consistent across the codebase:
✅ Verification successful
The repo_name assignment pattern is consistent and correctly implemented
The search results confirm that:
- The pattern
repo_name = repo_details.repo_name or repo_details.repo_path.split("/")[-1]is unique to the parsing controller and appropriately handles the fallback case. - Other repo_name assignments in the codebase handle different scenarios (e.g., direct assignments from project details or database records).
- The register_project calls consistently use the repo_name as the first parameter.
The moved assignment improves code clarity without breaking any patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances where repo_name is derived from repo_path
# to ensure consistent pattern usage
ast-grep --pattern 'repo_name = $x.repo_name or $x.repo_path.split("/")[-1]'
Length of output: 78
Script:
#!/bin/bash
# Let's try a broader search for repo_name assignments and repo_path usage patterns
# First, let's look for repo_name assignments
rg "repo_name\s*=" -A 2
# Then, let's look for repo_path splits
rg "repo_path.*split.*\[-1\]" -A 2
# Finally, let's look for register_project calls to verify parameter usage
rg "register_project" -A 5
Length of output: 4830
| # First, clear any existing data for this project | ||
| session.run( | ||
| """ | ||
| MATCH (n {repoId: $project_id}) | ||
| DETACH DELETE n | ||
| """, | ||
| project_id=project_id, | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Neo4j deletion operation needs additional safeguards
The code review reveals several concerns about the deletion operation:
- The deletion operation is not wrapped in an explicit transaction
- There's no error handling around the Neo4j operations
- The deletion query correctly uses
repoIdparameter to scope the deletion - The same deletion pattern is used in both
create_and_store_graphandcleanup_graphmethods
Recommended changes:
- Wrap the deletion operation in a try-catch block to handle potential Neo4j errors
- Use an explicit transaction with
session.begin_transaction()to ensure atomicity - Add logging to track successful deletions and failures
- Consider adding a validation step to confirm the deletion affected only intended nodes
Example implementation:
with self.driver.session() as session:
try:
with session.begin_transaction() as tx:
result = tx.run(
"""
MATCH (n {repoId: $project_id})
DETACH DELETE n
RETURN count(n) as deleted_count
""",
project_id=project_id,
)
deleted_count = result.single()["deleted_count"]
tx.commit()
logging.info(f"Successfully deleted {deleted_count} nodes for project {project_id}")
except Exception as e:
logging.error(f"Error deleting nodes for project {project_id}: {str(e)}")
raise🔗 Analysis chain
Ensure safe deletion of existing project data in Neo4j.
The code deletes all nodes associated with project_id from the Neo4j database. Verify that this operation does not unintentionally affect other data. Consider wrapping this operation in a transaction and adding error handling.
Run the following script to check if nodes outside of the specified project_id are unaffected:
This script uses rg to list all unique repoId values before and after the deletion to ensure no unintended data is removed.
Let me gather more information about the transaction and error handling aspects by examining the code context around this deletion operation.
Let me try one more search to understand the complete context of the code_graph_service class and its error handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that only nodes related to the specific project_id are deleted.
# Find all unique repoIds before deletion
echo "Repo IDs before deletion:"
rg -o --no-filename 'repoId: [^\n]+' | sort | uniq
# After deletion, check again
echo "Repo IDs after deletion:"
rg -o --no-filename 'repoId: [^\n]+' | sort | uniq
Length of output: 959
Script:
#!/bin/bash
# Check the context of the deletion operation and any transaction/error handling
ast-grep --pattern 'def create_graph($$$)' app/modules/parsing/graph_construction/code_graph_service.py -A 60
Length of output: 111
Script:
#!/bin/bash
# Look for transaction handling patterns in Neo4j operations
rg -B 2 -A 2 "session\.(begin_transaction|run)" app/modules/parsing/graph_construction/code_graph_service.py
Length of output: 1238
Script:
#!/bin/bash
# Check for error handling patterns around Neo4j operations
rg -B 2 -A 2 "try:.*(session|transaction)" app/modules/parsing/graph_construction/code_graph_service.py
Length of output: 105
Script:
#!/bin/bash
# Get the complete content of the code_graph_service.py file
cat app/modules/parsing/graph_construction/code_graph_service.py
Length of output: 7247
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
app/modules/code_provider/github/github_router.py (2)
Line range hint
582-704: Consider optimizing the graph creation process.While the implementation is functionally correct, there are opportunities for improvement:
- The unused variables in the loop can be renamed with underscore prefix
- The relationship creation calls could be simplified
Apply these changes:
- for root, dirs, files in os.walk(repo_dir): + for root, _dirs, files in os.walk(repo_dir):- for source, line, end_line, src_class, src_method in refs: + for source, line, end_line, _src_class, _src_method in refs:
Line range hint
739-752: Simplify the node type conditions using logical operators.The nested if conditions for node type checking can be simplified.
Apply this change:
- if node_type == "FUNCTION" and node.type in [ - "function_definition", - "method", - "method_declaration", - "function", - ]: - return node - elif node_type in ["CLASS", "INTERFACE"] and node.type in [ - "class_definition", - "interface", - "class", - "class_declaration", - "interface_declaration", - ]: + function_types = ["function_definition", "method", "method_declaration", "function"] + class_types = ["class_definition", "interface", "class", "class_declaration", "interface_declaration"] + if (node_type == "FUNCTION" and node.type in function_types) or \ + (node_type in ["CLASS", "INTERFACE"] and node.type in class_types): return nodeapp/modules/usage/usage_service.py (1)
Line range hint
18-28: Consider indexing strategy for query performanceThe query involves multiple joins, filters, and aggregations. Ensure proper indexing exists for:
Conversation.user_idMessage.created_atMessage.conversation_idMessage.typeThis will help maintain good performance as the data grows.
app/modules/parsing/graph_construction/parsing_repomap.py (5)
181-189: LGTM! Consider adding null check for decoded textThe Java method call handling enhancement is well-implemented. However, consider adding a null check when decoding the object reference text to handle potential encoding issues gracefully.
if object_node: - node_text = f"{object_node.text.decode('utf-8')}.{node_text}" + try: + object_text = object_node.text.decode('utf-8') + node_text = f"{object_text}.{node_text}" + except UnicodeDecodeError: + logging.warning(f"Failed to decode object reference in {rel_fname}")
534-580: LGTM! Consider adding type enums for better type safetyThe relationship creation helper is well-implemented with clear validation rules. To enhance type safety and maintainability, consider using enums for relationship and node types.
+from enum import Enum + +class NodeType(Enum): + FUNCTION = "FUNCTION" + CLASS = "CLASS" + INTERFACE = "INTERFACE" + +class RelationType(Enum): + REFERENCES = "REFERENCES" + CONTAINS = "CONTAINS" def create_relationship( G, source, target, relationship_type, seen_relationships, extra_data=None ): # ... - if relationship_type == "REFERENCES": + if relationship_type == RelationType.REFERENCES: # Implementation -> Interface if ( - source_data.get("type") == "FUNCTION" - and target_data.get("type") == "FUNCTION" + source_data.get("type") == NodeType.FUNCTION.value + and target_data.get("type") == NodeType.FUNCTION.value🧰 Tools
🪛 Ruff (0.8.2)
568-573: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
588-588: Fix unused loop variableRename unused loop variable
dirsto_dirsas per static analysis suggestion.-for root, dirs, files in os.walk(repo_dir): +for root, _dirs, files in os.walk(repo_dir):🧰 Tools
🪛 Ruff (0.8.2)
588-588: Loop control variable
dirsnot used within loop bodyRename unused
dirsto_dirs(B007)
582-704: Consider refactoring for better modularityThe graph creation logic is well-implemented but could benefit from better modularity. Consider extracting the file processing logic into a separate method and creating a helper for node creation to reduce code duplication.
def create_graph(self, repo_dir): G = nx.MultiDiGraph() defines = defaultdict(set) references = defaultdict(set) seen_relationships = set() + + def process_file(file_path, rel_path): + if not self.parse_helper.is_text_file(file_path): + return + + logging.info(f"\nProcessing file: {rel_path}") + add_file_node(G, rel_path, self.io.read_text(file_path)) + process_tags(file_path, rel_path) + + def add_file_node(G, rel_path, content): + if not G.has_node(rel_path): + G.add_node( + rel_path, + file=rel_path, + type="FILE", + text=content or "", + line=0, + end_line=0, + name=rel_path.split("/")[-1], + ) for root, _dirs, files in os.walk(repo_dir): if any(part.startswith(".") for part in root.split(os.sep)): continue for file in files: file_path = os.path.join(root, file) rel_path = os.path.relpath(file_path, repo_dir) - # ... rest of the file processing logic + process_file(file_path, rel_path)🧰 Tools
🪛 Ruff (0.8.2)
588-588: Loop control variable
dirsnot used within loop bodyRename unused
dirsto_dirs(B007)
688-688: Loop control variable
src_classnot used within loop bodyRename unused
src_classto_src_class(B007)
688-688: Loop control variable
src_methodnot used within loop bodyRename unused
src_methodto_src_method(B007)
739-752: Consider using constants for node typesWhile the node type checking is correct, consider defining constants for the node types to improve maintainability and prevent typos.
+# At the top of the file +FUNCTION_NODE_TYPES = frozenset([ + "function_definition", + "method", + "method_declaration", + "function", +]) + +CLASS_NODE_TYPES = frozenset([ + "class_definition", + "interface", + "class", + "class_declaration", + "interface_declaration", +]) def find_node_by_range(root_node, start_line, node_type): def traverse(node): if node.start_point[0] <= start_line and node.end_point[0] >= start_line: - if node_type == "FUNCTION" and node.type in [ - "function_definition", - "method", - "method_declaration", - "function", - ]: + if node_type == "FUNCTION" and node.type in FUNCTION_NODE_TYPES: return node - elif node_type in ["CLASS", "INTERFACE"] and node.type in [ - "class_definition", - "interface", - "class", - "class_declaration", - "interface_declaration", - ]: + elif node_type in ["CLASS", "INTERFACE"] and node.type in CLASS_NODE_TYPES: return node🧰 Tools
🪛 Ruff (0.8.2)
739-753: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/modules/code_provider/github/github_router.py(1 hunks)app/modules/parsing/graph_construction/code_graph_service.py(4 hunks)app/modules/parsing/graph_construction/parsing_controller.py(4 hunks)app/modules/parsing/graph_construction/parsing_repomap.py(4 hunks)app/modules/usage/usage_controller.py(1 hunks)app/modules/usage/usage_router.py(1 hunks)app/modules/usage/usage_schema.py(1 hunks)app/modules/usage/usage_service.py(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/modules/usage/usage_schema.py
- app/modules/usage/usage_router.py
🚧 Files skipped from review as they are similar to previous changes (2)
- app/modules/parsing/graph_construction/code_graph_service.py
- app/modules/parsing/graph_construction/parsing_controller.py
🧰 Additional context used
🪛 Ruff (0.8.2)
app/modules/parsing/graph_construction/parsing_repomap.py
568-573: Combine if branches using logical or operator
Combine if branches
(SIM114)
588-588: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
688-688: Loop control variable src_class not used within loop body
Rename unused src_class to _src_class
(B007)
688-688: Loop control variable src_method not used within loop body
Rename unused src_method to _src_method
(B007)
739-753: Combine if branches using logical or operator
Combine if branches
(SIM114)
🔇 Additional comments (5)
app/modules/code_provider/github/github_router.py (2)
19-31: LGTM! Clean implementation of repository deduplication.
The implementation efficiently removes duplicate repositories while preserving order, using the repository's full name as a unique identifier.
Line range hint 534-580: Well-structured relationship creation logic with proper validation.
The create_relationship helper method effectively:
- Prevents self-referential relationships
- Avoids duplicate bidirectional relationships
- Enforces valid relationship directions based on node types
app/modules/usage/usage_controller.py (1)
9-11: LGTM: Improved method signature formatting
The parameter formatting change improves readability by placing each parameter on a separate line.
app/modules/usage/usage_service.py (2)
32-41: LGTM: Clean data transformation
The conversion of query results to a dictionary and the calculation of total messages is clean and efficient.
Line range hint 43-46: LGTM: Proper error handling
The error handling appropriately catches SQLAlchemy errors, logs them, and raises a generic exception to avoid exposing internal details.
|
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Chores