edges are not bound-able #498
Conversation
WalkthroughThe recent changes focus on enhancing the validation logic for Abstract Syntax Tree (AST) nodes in query language operations, particularly for Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 9
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (6)
- src/ast/ast_validations.c (5 hunks)
- src/errors/error_msgs.h (1 hunks)
- tests/flow/test_bound_variables.py (7 hunks)
- tests/flow/test_create_clause.py (2 hunks)
- tests/flow/test_graph_create.py (1 hunks)
- tests/flow/test_null_handling.py (1 hunks)
Additional comments not posted (2)
src/errors/error_msgs.h (1)
64-64: Confirm the addition of the new error messageEMSG_EDGE_REDECLARATIONfor when an edge is declared more than once.
- This change aligns with the PR's objective to improve error handling and messaging for edge variable redeclaration.
- Ensure that the error message is consistently used across the codebase where applicable.
src/ast/ast_validations.c (1)
1670-1670: EnsureCALL {}clauses are non-returning when they are the last clause in a query.The validation for
CALL {}clauses being non-returning when they are the last clause in a query is crucial for query correctness. This shell script usesast-grepto search for anyCALL {}clauses that improperly include aRETURNstatement, which should be manually reviewed to ensure compliance with the validation rules.Verification successful
This script aims to find references to the handling or validation of
CALL {}clauses within the C codebase, which could provide insights into how these clauses are ensured to be non-returning when they are the last clause in a query.
The review comment is supported by the evidence found in the script output, particularly within
src/ast/ast_validations.c, which includes comments and logic related to ensuringCALL {}clauses are non-returning when they are the last clause in a query. This confirms the importance of the validation for query correctness as mentioned in the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for CALL {} clauses that are the last clause and ensure they are non-returning. ast-grep --lang cypher --pattern $'CALL { $$$ RETURN($_) }'Length of output: 177
Script:
#!/bin/bash # Search for references to 'CALL' clause handling or validation in C files. rg "CALL.*\{.*\}" --type cLength of output: 1931
tests/flow/test_create_clause.py
Outdated
| q = "CREATE ()-[e:R]->()-[e:R]->()" | ||
| try: | ||
| self.g.query(q) | ||
| # should not reach this point | ||
| self.env.assertTrue(False) | ||
| except Exception as e: | ||
| self.env.assertTrue("Variable `e` already declared" in str(e)) | ||
| self.env.assertTrue("Edge `e` can only be declared once" in str(e)) |
There was a problem hiding this comment.
Refine the error message check to ensure it precisely matches the new standard error message for edge redeclaration.
- self.env.assertTrue("Edge `e` can only be declared once" in str(e))
+ self.env.assertTrue("Edge 'e' can only be declared once" in str(e))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| self.env.assertTrue("Edge `e` can only be declared once" in str(e)) | |
| self.env.assertTrue("Edge 'e' can only be declared once" in str(e)) |
tests/flow/test_create_clause.py
Outdated
| @@ -68,5 +67,5 @@ def test02_edge_reuse(self): | |||
| # should not reach this point | |||
| self.env.assertTrue(False) | |||
| except Exception as e: | |||
| self.env.assertTrue("The bound variable 'e' can't be redeclared in a CREATE clause" in str(e)) | |||
| self.env.assertTrue("Edge `e` can only be declared once" in str(e)) | |||
There was a problem hiding this comment.
Refine the error message check to ensure it precisely matches the new standard error message for edge redeclaration.
- self.env.assertTrue("Edge `e` can only be declared once" in str(e))
+ self.env.assertTrue("Edge 'e' can only be declared once" in str(e))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| self.env.assertTrue("Edge `e` can only be declared once" in str(e)) | |
| self.env.assertTrue("Edge 'e' can only be declared once" in str(e)) |
tests/flow/test_bound_variables.py
Outdated
| def test07_bound_edges(self): | ||
| # edges can only be declared once | ||
| # re-declaring a variable as an edge is forbidden | ||
|
|
||
| queries = ["MATCH ()-[e]->()-[e]->() RETURN *", | ||
| "MATCH ()-[e]->() MATCH ()<-[e]-() RETURN *", | ||
| "WITH NULL AS e MATCH ()-[e]->() RETURN *", | ||
| "MATCH ()-[e]->() WITH e MATCH ()-[e]->() RETURN *", | ||
| "MATCH ()-[e]->() MERGE ()-[e:R]->() RETURN *", | ||
| "WITH NULL AS e MERGE ()-[e:R]->() RETURN *", | ||
| "MERGE ()-[e:R]->() MERGE ()-[e:R]->()", | ||
| "MATCH ()-[e]->() WHERE ()-[e]->() RETURN *"] | ||
|
|
||
| for q in queries: | ||
| try: | ||
| res = self.g.query(q) | ||
| # should not reach this point | ||
| self.env.assertFalse(True) | ||
| except Exception as e: | ||
| self.env.assertIn("Edge `e` can only be declared once", str(e)) | ||
|
|
There was a problem hiding this comment.
Ensure the new test method test07_bound_edges correctly tests the constraints on edge declaration.
- The test method is well-structured and covers various scenarios where edge redeclaration might occur.
- The error message check within the exception handling should precisely match the new standard error message for edge redeclaration.
Consider refining the error message check to align with the new error message format.
- self.env.assertIn("Edge `e` can only be declared once", str(e))
+ self.env.assertIn("Edge 'e' can only be declared once", str(e))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def test07_bound_edges(self): | |
| # edges can only be declared once | |
| # re-declaring a variable as an edge is forbidden | |
| queries = ["MATCH ()-[e]->()-[e]->() RETURN *", | |
| "MATCH ()-[e]->() MATCH ()<-[e]-() RETURN *", | |
| "WITH NULL AS e MATCH ()-[e]->() RETURN *", | |
| "MATCH ()-[e]->() WITH e MATCH ()-[e]->() RETURN *", | |
| "MATCH ()-[e]->() MERGE ()-[e:R]->() RETURN *", | |
| "WITH NULL AS e MERGE ()-[e:R]->() RETURN *", | |
| "MERGE ()-[e:R]->() MERGE ()-[e:R]->()", | |
| "MATCH ()-[e]->() WHERE ()-[e]->() RETURN *"] | |
| for q in queries: | |
| try: | |
| res = self.g.query(q) | |
| # should not reach this point | |
| self.env.assertFalse(True) | |
| except Exception as e: | |
| self.env.assertIn("Edge `e` can only be declared once", str(e)) | |
| def test07_bound_edges(self): | |
| # edges can only be declared once | |
| # re-declaring a variable as an edge is forbidden | |
| queries = ["MATCH ()-[e]->()-[e]->() RETURN *", | |
| "MATCH ()-[e]->() MATCH ()<-[e]-() RETURN *", | |
| "WITH NULL AS e MATCH ()-[e]->() RETURN *", | |
| "MATCH ()-[e]->() WITH e MATCH ()-[e]->() RETURN *", | |
| "MATCH ()-[e]->() MERGE ()-[e:R]->() RETURN *", | |
| "WITH NULL AS e MERGE ()-[e:R]->() RETURN *", | |
| "MERGE ()-[e:R]->() MERGE ()-[e:R]->()", | |
| "MATCH ()-[e]->() WHERE ()-[e]->() RETURN *"] | |
| for q in queries: | |
| try: | |
| res = self.g.query(q) | |
| # should not reach this point | |
| self.env.assertFalse(True) | |
| except Exception as e: | |
| self.env.assertIn("Edge 'e' can only be declared once", str(e)) | |
tests/flow/test_null_handling.py
Outdated
| try: | ||
| query = """WITH NULL AS e MATCH (a:L)-[e]->(b) RETURN a, e, b""" | ||
| plan = self.graph.explain(query) | ||
| # not expecting to reach this point | ||
| self.env.assertTrue(False) | ||
| except redis.exceptions.ResponseError as e: | ||
| self.env.assertContains("Edge `e` can only be declared once", str(e)) |
There was a problem hiding this comment.
Refine the error message check to ensure it precisely matches the new standard error message for edge redeclaration.
- self.env.assertContains("Edge `e` can only be declared once", str(e))
+ self.env.assertContains("Edge 'e' can only be declared once", str(e))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| try: | |
| query = """WITH NULL AS e MATCH (a:L)-[e]->(b) RETURN a, e, b""" | |
| plan = self.graph.explain(query) | |
| # not expecting to reach this point | |
| self.env.assertTrue(False) | |
| except redis.exceptions.ResponseError as e: | |
| self.env.assertContains("Edge `e` can only be declared once", str(e)) | |
| try: | |
| query = """WITH NULL AS e MATCH (a:L)-[e]->(b) RETURN a, e, b""" | |
| plan = self.graph.explain(query) | |
| # not expecting to reach this point | |
| self.env.assertTrue(False) | |
| except redis.exceptions.ResponseError as e: | |
| self.env.assertContains("Edge 'e' can only be declared once", str(e)) |
tests/flow/test_graph_create.py
Outdated
| "CREATE (n1)-[r:Rel1]->(n2), (n2)-[r:Rel1]->(n1)", | ||
| "CREATE (n1)-[r:Rel1]->(n2) CREATE (n2)-[r2:Rel1]->(n3), (n3)-[r:Rel1]->(n2)"] | ||
|
|
||
| for query in queries: | ||
| try: | ||
| self.graph.query(query) | ||
| self.env.assertTrue(False) | ||
| except redis.exceptions.ResponseError as e: | ||
| self.env.assertContains("The bound variable 'r' can't be redeclared in a CREATE clause", str(e)) | ||
| self.env.assertContains("Edge `r` can only be declared once", str(e)) |
There was a problem hiding this comment.
Refine the error message checks to ensure they precisely match the new standard error message for edge redeclaration.
- self.env.assertContains("Edge `r` can only be declared once", str(e))
+ self.env.assertContains("Edge 'r' can only be declared once", str(e))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "CREATE (n1)-[r:Rel1]->(n2), (n2)-[r:Rel1]->(n1)", | |
| "CREATE (n1)-[r:Rel1]->(n2) CREATE (n2)-[r2:Rel1]->(n3), (n3)-[r:Rel1]->(n2)"] | |
| for query in queries: | |
| try: | |
| self.graph.query(query) | |
| self.env.assertTrue(False) | |
| except redis.exceptions.ResponseError as e: | |
| self.env.assertContains("The bound variable 'r' can't be redeclared in a CREATE clause", str(e)) | |
| self.env.assertContains("Edge `r` can only be declared once", str(e)) | |
| "CREATE (n1)-[r:Rel1]->(n2), (n2)-[r:Rel1]->(n1)", | |
| "CREATE (n1)-[r:Rel1]->(n2) CREATE (n2)-[r2:Rel1]->(n3), (n3)-[r:Rel1]->(n2)"] | |
| for query in queries: | |
| try: | |
| self.graph.query(query) | |
| self.env.assertTrue(False) | |
| except redis.exceptions.ResponseError as e: | |
| self.env.assertContains("Edge 'r' can only be declared once", str(e)) |
| try: | ||
| self.graph.query(query) | ||
| self.env.assertTrue(False) | ||
| except redis.exceptions.ResponseError as e: | ||
| self.env.assertContains("Variable `r` already declared", str(e)) | ||
| self.env.assertContains("The bound variable 'r' can't be redeclared in a CREATE clause", str(e)) |
There was a problem hiding this comment.
Refine the error message check to ensure it precisely matches the new standard error message for variable redeclaration.
- self.env.assertContains("The bound variable 'r' can't be redeclared in a CREATE clause", str(e))
+ self.env.assertContains("The bound variable 'r' cannot be redeclared in a CREATE clause", str(e))Note: This suggestion assumes that the error message standardization also applies to other variable redeclaration scenarios. If not, please disregard this comment.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| self.env.assertContains("The bound variable 'r' can't be redeclared in a CREATE clause", str(e)) | |
| self.env.assertContains("The bound variable 'r' cannot be redeclared in a CREATE clause", str(e)) |
src/ast/ast_validations.c
Outdated
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | ||
| enum cypher_rel_direction dir = cypher_ast_rel_pattern_get_direction(n); | ||
| const cypher_astnode_t *range = cypher_ast_rel_pattern_get_varlength(n); | ||
| if(vctx->clause == CYPHER_AST_CREATE) { | ||
| // validate that the relation alias is not bound | ||
| if(_ValidateCreateRelation(n, vctx) == AST_INVALID) { | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| // Validate that each relation has exactly one type | ||
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | ||
| // extra validation when in a CREATE / MERGE clause | ||
| if(vctx->clause == CYPHER_AST_CREATE || vctx->clause == CYPHER_AST_MERGE) { | ||
| // clause name used for error reporting | ||
| const char *clause_name = (vctx->clause == CYPHER_AST_CREATE) ? | ||
| "CREATE" : | ||
| "MERGE"; | ||
|
|
||
| // validate that each relation has exactly one type | ||
| if(reltype_count != 1) { | ||
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, "CREATE"); | ||
| // CREATE ()-[:A:B]->() | ||
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name); | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| // Validate that each relation being created is directed | ||
| if(cypher_ast_rel_pattern_get_direction(n) == CYPHER_REL_BIDIRECTIONAL) { | ||
| // validate that each relation being created is directed | ||
| // CREATE ()-[]-() | ||
| if(dir == CYPHER_REL_BIDIRECTIONAL) { | ||
| ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP); | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| // Validate that each relation being created is not variable length relationship | ||
| // validate that each relation being created is not of variable length | ||
| // CREATE ()-[*2..4]->() | ||
| if(range) { | ||
| ErrorCtx_SetError(EMSG_VAR_LEN, "CREATE"); | ||
| ErrorCtx_SetError(EMSG_VAR_LEN, clause_name); | ||
| return VISITOR_BREAK; | ||
| } | ||
| } | ||
|
|
||
| if(_ValidateInlinedProperties(cypher_ast_rel_pattern_get_properties(n)) == AST_INVALID) { | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| if(vctx->clause == CYPHER_AST_MERGE && | ||
| _ValidateMergeRelation(n, vctx) == AST_INVALID) { | ||
| const cypher_astnode_t *props = cypher_ast_rel_pattern_get_properties(n); | ||
| if(_ValidateInlinedProperties(props) == AST_INVALID) { | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| const cypher_astnode_t *alias_node = cypher_ast_rel_pattern_get_identifier(n); | ||
| if(!alias_node && !range) { | ||
| return VISITOR_RECURSE; // Skip unaliased, single-hop entities. | ||
| return VISITOR_RECURSE; // skip unaliased, single-hop entities | ||
| } | ||
|
|
||
| // If this is a multi-hop traversal, validate it accordingly | ||
| // if this is a multi-hop traversal, validate it accordingly | ||
| if(range && _ValidateMultiHopTraversal(n, range) == AST_INVALID) { | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| if(alias_node) { | ||
| const char *alias = cypher_ast_identifier_get_name(alias_node); | ||
| void *alias_type = _IdentifiersFind(vctx, alias); | ||
| if(alias_type == raxNotFound) { | ||
| _IdentifierAdd(vctx, alias, (void*)T_EDGE); | ||
| return VISITOR_RECURSE; | ||
| } | ||
|
|
||
| if(alias_type != (void *)T_EDGE && alias_type != NULL) { | ||
| ErrorCtx_SetError(EMSG_SAME_ALIAS_NODE_RELATIONSHIP, alias); | ||
| return VISITOR_BREAK; | ||
| } | ||
| void *alias_type = _IdentifiersFind(vctx, alias); | ||
|
|
||
| if(vctx->clause == CYPHER_AST_MATCH && alias_type != NULL) { | ||
| ErrorCtx_SetError(EMSG_SAME_ALIAS_MULTIPLE_PATTERNS, alias); | ||
| // edge can not be redeclared | ||
| if(_IdentifierAdd(vctx, alias, (void*)T_EDGE) == 0) { | ||
| ErrorCtx_SetError(EMSG_EDGE_REDECLARATION, alias); |
There was a problem hiding this comment.
Consider refactoring _Validate_rel_pattern to improve readability and maintainability.
- if(vctx->clause == CYPHER_AST_CREATE || vctx->clause == CYPHER_AST_MERGE) {
+ // Refactor into a separate function for clarity
+ if(_isCreateOrMergeClause(vctx->clause)) {The function _Validate_rel_pattern is complex and handles multiple validation checks. Refactoring it by extracting some of its logic into smaller, more focused functions can improve readability and maintainability. For example, the checks specific to CREATE and MERGE clauses could be moved into a separate function. This would make the main function shorter and its purpose clearer.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | |
| enum cypher_rel_direction dir = cypher_ast_rel_pattern_get_direction(n); | |
| const cypher_astnode_t *range = cypher_ast_rel_pattern_get_varlength(n); | |
| if(vctx->clause == CYPHER_AST_CREATE) { | |
| // validate that the relation alias is not bound | |
| if(_ValidateCreateRelation(n, vctx) == AST_INVALID) { | |
| return VISITOR_BREAK; | |
| } | |
| // Validate that each relation has exactly one type | |
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | |
| // extra validation when in a CREATE / MERGE clause | |
| if(vctx->clause == CYPHER_AST_CREATE || vctx->clause == CYPHER_AST_MERGE) { | |
| // clause name used for error reporting | |
| const char *clause_name = (vctx->clause == CYPHER_AST_CREATE) ? | |
| "CREATE" : | |
| "MERGE"; | |
| // validate that each relation has exactly one type | |
| if(reltype_count != 1) { | |
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, "CREATE"); | |
| // CREATE ()-[:A:B]->() | |
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name); | |
| return VISITOR_BREAK; | |
| } | |
| // Validate that each relation being created is directed | |
| if(cypher_ast_rel_pattern_get_direction(n) == CYPHER_REL_BIDIRECTIONAL) { | |
| // validate that each relation being created is directed | |
| // CREATE ()-[]-() | |
| if(dir == CYPHER_REL_BIDIRECTIONAL) { | |
| ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP); | |
| return VISITOR_BREAK; | |
| } | |
| // Validate that each relation being created is not variable length relationship | |
| // validate that each relation being created is not of variable length | |
| // CREATE ()-[*2..4]->() | |
| if(range) { | |
| ErrorCtx_SetError(EMSG_VAR_LEN, "CREATE"); | |
| ErrorCtx_SetError(EMSG_VAR_LEN, clause_name); | |
| return VISITOR_BREAK; | |
| } | |
| } | |
| if(_ValidateInlinedProperties(cypher_ast_rel_pattern_get_properties(n)) == AST_INVALID) { | |
| return VISITOR_BREAK; | |
| } | |
| if(vctx->clause == CYPHER_AST_MERGE && | |
| _ValidateMergeRelation(n, vctx) == AST_INVALID) { | |
| const cypher_astnode_t *props = cypher_ast_rel_pattern_get_properties(n); | |
| if(_ValidateInlinedProperties(props) == AST_INVALID) { | |
| return VISITOR_BREAK; | |
| } | |
| const cypher_astnode_t *alias_node = cypher_ast_rel_pattern_get_identifier(n); | |
| if(!alias_node && !range) { | |
| return VISITOR_RECURSE; // Skip unaliased, single-hop entities. | |
| return VISITOR_RECURSE; // skip unaliased, single-hop entities | |
| } | |
| // If this is a multi-hop traversal, validate it accordingly | |
| // if this is a multi-hop traversal, validate it accordingly | |
| if(range && _ValidateMultiHopTraversal(n, range) == AST_INVALID) { | |
| return VISITOR_BREAK; | |
| } | |
| if(alias_node) { | |
| const char *alias = cypher_ast_identifier_get_name(alias_node); | |
| void *alias_type = _IdentifiersFind(vctx, alias); | |
| if(alias_type == raxNotFound) { | |
| _IdentifierAdd(vctx, alias, (void*)T_EDGE); | |
| return VISITOR_RECURSE; | |
| } | |
| if(alias_type != (void *)T_EDGE && alias_type != NULL) { | |
| ErrorCtx_SetError(EMSG_SAME_ALIAS_NODE_RELATIONSHIP, alias); | |
| return VISITOR_BREAK; | |
| } | |
| void *alias_type = _IdentifiersFind(vctx, alias); | |
| if(vctx->clause == CYPHER_AST_MATCH && alias_type != NULL) { | |
| ErrorCtx_SetError(EMSG_SAME_ALIAS_MULTIPLE_PATTERNS, alias); | |
| // edge can not be redeclared | |
| if(_IdentifierAdd(vctx, alias, (void*)T_EDGE) == 0) { | |
| ErrorCtx_SetError(EMSG_EDGE_REDECLARATION, alias); | |
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | |
| enum cypher_rel_direction dir = cypher_ast_rel_pattern_get_direction(n); | |
| const cypher_astnode_t *range = cypher_ast_rel_pattern_get_varlength(n); | |
| // extra validation when in a CREATE / MERGE clause | |
| if(_isCreateOrMergeClause(vctx->clause)) { | |
| // clause name used for error reporting | |
| const char *clause_name = (vctx->clause == CYPHER_AST_CREATE) ? | |
| "CREATE" : | |
| "MERGE"; | |
| // validate that each relation has exactly one type | |
| if(reltype_count != 1) { | |
| // CREATE ()-[:A:B]->() | |
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name); | |
| return VISITOR_BREAK; | |
| } | |
| // validate that each relation being created is directed | |
| // CREATE ()-[]-() | |
| if(dir == CYPHER_REL_BIDIRECTIONAL) { | |
| ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP); | |
| return VISITOR_BREAK; | |
| } | |
| // validate that each relation being created is not of variable length | |
| // CREATE ()-[*2..4]->() | |
| if(range) { | |
| ErrorCtx_SetError(EMSG_VAR_LEN, clause_name); | |
| return VISITOR_BREAK; | |
| } | |
| } | |
| const cypher_astnode_t *props = cypher_ast_rel_pattern_get_properties(n); | |
| if(_ValidateInlinedProperties(props) == AST_INVALID) { | |
| return VISITOR_BREAK; | |
| } | |
| const cypher_astnode_t *alias_node = cypher_ast_rel_pattern_get_identifier(n); | |
| if(!alias_node && !range) { | |
| return VISITOR_RECURSE; // skip unaliased, single-hop entities | |
| } | |
| // if this is a multi-hop traversal, validate it accordingly | |
| if(range && _ValidateMultiHopTraversal(n, range) == AST_INVALID) { | |
| return VISITOR_BREAK; | |
| } | |
| if(alias_node) { | |
| const char *alias = cypher_ast_identifier_get_name(alias_node); | |
| void *alias_type = _IdentifiersFind(vctx, alias); | |
| // edge can not be redeclared | |
| if(_IdentifierAdd(vctx, alias, (void*)T_EDGE) == 0) { | |
| ErrorCtx_SetError(EMSG_EDGE_REDECLARATION, alias); |
| @@ -250,7 +250,7 @@ static AST_Validation _ValidateMultiHopTraversal | |||
| end = AST_ParseIntegerNode(range_end); | |||
| } | |||
|
|
|||
| // Validate specified range | |||
| // validate specified range | |||
There was a problem hiding this comment.
Ensure proper handling of negative range values in _ValidateMultiHopTraversal.
+ // Ensure start and end are non-negative
+ if(start < 0 || end < 0) {
+ ErrorCtx_SetError(EMSG_VAR_LEN_INVALID_RANGE);
+ return AST_INVALID;
+ }The function _ValidateMultiHopTraversal currently checks if the start of the range is greater than the end, but it does not explicitly check for negative values. In Cypher, ranges should be non-negative. Adding a check for negative values would prevent potential issues with range specifications.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // validate specified range | |
| // validate specified range | |
| + // Ensure start and end are non-negative | |
| + if(start < 0 || end < 0) { | |
| + ErrorCtx_SetError(EMSG_VAR_LEN_INVALID_RANGE); | |
| + return AST_INVALID; | |
| + } |
src/ast/ast_validations.c
Outdated
| @@ -1729,7 +1667,7 @@ static VISITOR_STRATEGY _Validate_CREATE_Clause | |||
|
|
|||
| // fail on duplicate identifier | |||
| if(_IdentifierAdd(vctx, alias, (void*)t) == 0 && t == T_EDGE) { | |||
| ErrorCtx_SetError(EMSG_VAIABLE_ALREADY_DECLARED, alias); | |||
| ErrorCtx_SetError(EMSG_EDGE_REDECLARATION, alias); | |||
There was a problem hiding this comment.
Avoid redeclaration of edges in CREATE clauses.
- ErrorCtx_SetError(EMSG_EDGE_REDECLARATION, alias);The error handling for edge redeclaration within CREATE clauses is correctly placed. However, ensure that the logic preceding this error handling accurately identifies redeclarations and does not falsely trigger this error in valid scenarios.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ErrorCtx_SetError(EMSG_EDGE_REDECLARATION, alias); |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/ast/ast_validations.c (5 hunks)
Additional comments not posted (3)
src/ast/ast_validations.c (3)
739-793: Consider refactoring_Validate_rel_patternto improve readability and maintainability.
1678-1678: Avoid redeclaration of edges inCREATEclauses.
253-253: Ensure proper handling of negative range values in_ValidateMultiHopTraversal.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #498 +/- ##
==========================================
- Coverage 81.04% 81.01% -0.03%
==========================================
Files 313 313
Lines 31856 31826 -30
Branches 5113 5105 -8
==========================================
- Hits 25818 25785 -33
- Misses 5207 5210 +3
Partials 831 831 ☔ View full report in Codecov by Sentry. |
src/ast/ast_validations.c
Outdated
| ErrorCtx_SetError(EMSG_SAME_ALIAS_NODE_RELATIONSHIP, alias); | ||
| return VISITOR_BREAK; | ||
| } | ||
| void *alias_type = _IdentifiersFind(vctx, alias); |
swilly22
force-pushed
the
edge-reuse-validation
branch
from
cd750f2
to
c7c198a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
tests/flow/test_bound_variables.py (1)
Line range hint
1-1: Avoid using star imports as they can lead to undefined names.- from common import * - from index_utils import * + from common import Env, FlowTestsBase + from index_utils import create_node_fulltext_indexAlso applies to: 2-2, 4-4, 8-8, 11-11
Tools
Ruff
8-8:
FlowTestsBasemay be undefined, or defined from star imports (F405)
11-11:
Envmay be undefined, or defined from star imports (F405)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/ast/ast_validations.c (5 hunks)
- tests/flow/test_bound_variables.py (7 hunks)
Additional context used
Ruff
tests/flow/test_bound_variables.py
1-1:
from common import *used; unable to detect undefined names (F403)
2-2:
from index_utils import *used; unable to detect undefined names (F403)
4-4:
sysmay be undefined, or defined from star imports (F405)
4-4:
osmay be undefined, or defined from star imports (F405)
4-4:
osmay be undefined, or defined from star imports (F405)
8-8:
FlowTestsBasemay be undefined, or defined from star imports (F405)
11-11:
Envmay be undefined, or defined from star imports (F405)
22-22:
Nodemay be undefined, or defined from star imports (F405)
27-27:
Edgemay be undefined, or defined from star imports (F405)
28-28:
Edgemay be undefined, or defined from star imports (F405)
58-58:
create_node_fulltext_indexmay be undefined, or defined from star imports (F405)
130-130: Local variable
resis assigned to but never used (F841)
Additional comments not posted (7)
tests/flow/test_bound_variables.py (6)
38-38: The check for Cartesian products is correctly implemented.
50-50: The checks for created nodes and relationships are correctly implemented.
60-65: The verification of the execution plan's order is correctly implemented.
79-84: The count of scan operations in the execution plan is correctly verified.
110-114: The verification of overriding a bound alias with a new scan is correctly implemented.
134-134: Refine the error message check to align with the new error message format.- self.env.assertIn("Edge `e` can only be declared once", str(e)) + self.env.assertIn("Edge 'e' can only be declared once", str(e))src/ast/ast_validations.c (1)
790-791: Avoid redeclaration of edges inCREATEclauses.The error handling for edge redeclaration within
CREATEclauses is correctly placed. However, ensure that the logic preceding this error handling accurately identifies redeclarations and does not falsely trigger this error in valid scenarios.
src/ast/ast_validations.c
Outdated
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | ||
| enum cypher_rel_direction dir = cypher_ast_rel_pattern_get_direction(n); | ||
| const cypher_astnode_t *range = cypher_ast_rel_pattern_get_varlength(n); | ||
| if(vctx->clause == CYPHER_AST_CREATE) { | ||
| // validate that the relation alias is not bound | ||
| if(_ValidateCreateRelation(n, vctx) == AST_INVALID) { | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| // Validate that each relation has exactly one type | ||
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | ||
| // extra validation when in a CREATE / MERGE clause | ||
| if(vctx->clause == CYPHER_AST_CREATE || vctx->clause == CYPHER_AST_MERGE) { | ||
| // clause name used for error reporting | ||
| const char *clause_name = (vctx->clause == CYPHER_AST_CREATE) ? | ||
| "CREATE" : | ||
| "MERGE"; | ||
|
|
||
| // validate that each relation has exactly one type | ||
| if(reltype_count != 1) { | ||
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, "CREATE"); | ||
| // CREATE ()-[:A:B]->() | ||
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name); | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| // Validate that each relation being created is directed | ||
| if(cypher_ast_rel_pattern_get_direction(n) == CYPHER_REL_BIDIRECTIONAL) { | ||
| // validate that each relation being created is directed | ||
| // CREATE ()-[]-() | ||
| if(dir == CYPHER_REL_BIDIRECTIONAL) { | ||
| ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP); | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| // Validate that each relation being created is not variable length relationship | ||
| // validate that each relation being created is not of variable length | ||
| // CREATE ()-[*2..4]->() | ||
| if(range) { | ||
| ErrorCtx_SetError(EMSG_VAR_LEN, "CREATE"); | ||
| ErrorCtx_SetError(EMSG_VAR_LEN, clause_name); |
There was a problem hiding this comment.
Consider simplifying the validation logic for relation patterns.
The validation logic for relation patterns in CREATE and MERGE clauses is quite verbose and repetitive. Consider refactoring this into a helper function to check the conditions and set the error context. This would make the code cleaner and more maintainable.
- if(reltype_count != 1) {
- ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name);
- return VISITOR_BREAK;
- }
- if(dir == CYPHER_REL_BIDIRECTIONAL) {
- ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP);
- return VISITOR_BREAK;
- }
- if(range) {
- ErrorCtx_SetError(EMSG_VAR_LEN, clause_name);
- return VISITOR_BREAK;
- }
+ if (!validateRelationPattern(reltype_count, dir, range, clause_name)) {
+ return VISITOR_BREAK;
+ }Committable suggestion was skipped due to low confidence.
| @@ -20,6 +21,7 @@ def populate_graph(self): | |||
| for idx, v in enumerate(node_props): | |||
| node = Node(alias=f"n_{idx}", labels="L", properties={"val": v}) | |||
There was a problem hiding this comment.
Ensure that Node and Edge are defined or imported explicitly.
+ from graph_entities import Node, EdgeAlso applies to: 27-28
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| node = Node(alias=f"n_{idx}", labels="L", properties={"val": v}) | |
| + from graph_entities import Node, Edge | |
| node = Node(alias=f"n_{idx}", labels="L", properties={"val": v}) |
Tools
Ruff
22-22:
Nodemay be undefined, or defined from star imports (F405)
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
tests/flow/test_bound_variables.py (1)
Line range hint
1-1: Refactor to use explicit imports instead of star imports.- from common import * - from index_utils import * + from common import Env, Node, Edge + from index_utils import create_node_fulltext_indexThis change will help avoid potential issues with undefined names and improve code clarity.
Also applies to: 2-2, 4-4, 8-8, 11-11, 22-22, 27-27, 28-28, 58-58
Tools
Ruff
8-8:
FlowTestsBasemay be undefined, or defined from star imports (F405)
11-11:
Envmay be undefined, or defined from star imports (F405)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/ast/ast_validations.c (5 hunks)
- tests/flow/test_bound_variables.py (7 hunks)
Additional context used
Ruff
tests/flow/test_bound_variables.py
1-1:
from common import *used; unable to detect undefined names (F403)
2-2:
from index_utils import *used; unable to detect undefined names (F403)
4-4:
sysmay be undefined, or defined from star imports (F405)
4-4:
osmay be undefined, or defined from star imports (F405)
4-4:
osmay be undefined, or defined from star imports (F405)
8-8:
FlowTestsBasemay be undefined, or defined from star imports (F405)
11-11:
Envmay be undefined, or defined from star imports (F405)
22-22:
Nodemay be undefined, or defined from star imports (F405)
27-27:
Edgemay be undefined, or defined from star imports (F405)
28-28:
Edgemay be undefined, or defined from star imports (F405)
58-58:
create_node_fulltext_indexmay be undefined, or defined from star imports (F405)
130-130: Local variable
resis assigned to but never used (F841)
Additional comments not posted (7)
tests/flow/test_bound_variables.py (6)
Line range hint
10-27: Setup inpopulate_graphmethod looks correct.Tools
Ruff
22-22:
Nodemay be undefined, or defined from star imports (F405)
27-27:
Edgemay be undefined, or defined from star imports (F405)
38-38: Check for Cartesian products is correctly implemented.
50-50: Creation and validation intest02_match_create_bound_variableare correctly implemented.
60-65: Integration of full-text index with MATCH pattern is correctly implemented.
79-84: Scanning and validation intest04_projected_scanned_entityare correctly implemented.
110-114: Alias override behavior intest06_override_bound_with_labelis correctly implemented.src/ast/ast_validations.c (1)
1676-1676: Ensure that the error handling for edge redeclaration is robust and covers all necessary scenarios.Verification successful
Ensure that the error handling for edge redeclaration is robust and covers all necessary scenarios.
- The definition of
EMSG_EDGE_REDECLARATIONis found insrc/errors/error_msgs.h.- The usage of
ErrorCtx_SetError(EMSG_EDGE_REDECLARATION, alias);is found only insrc/ast/ast_validations.c.Given that the error handling for edge redeclaration is localized to
src/ast/ast_validations.c, it appears to be correctly implemented within the context of this file. However, there are no other instances of this specific error handling across the codebase, which suggests that edge redeclaration errors are only managed within this specific validation function.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that edge redeclaration is handled correctly across all scenarios. # Test: Search for edge redeclaration handling in all relevant files. rg --type c $'ErrorCtx_SetError(EMSG_EDGE_REDECLARATION, alias);'Length of output: 66
Script:
#!/bin/bash # Description: Verify the definition and usage of EMSG_EDGE_REDECLARATION and ErrorCtx_SetError. # Search for the definition of EMSG_EDGE_REDECLARATION rg --type c 'EMSG_EDGE_REDECLARATION' # Search for all usages of ErrorCtx_SetError rg --type c 'ErrorCtx_SetError'Length of output: 16195
src/ast/ast_validations.c
Outdated
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | ||
| enum cypher_rel_direction dir = cypher_ast_rel_pattern_get_direction(n); | ||
| const cypher_astnode_t *range = cypher_ast_rel_pattern_get_varlength(n); | ||
| if(vctx->clause == CYPHER_AST_CREATE) { | ||
| // validate that the relation alias is not bound | ||
| if(_ValidateCreateRelation(n, vctx) == AST_INVALID) { | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| // Validate that each relation has exactly one type | ||
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | ||
| // extra validation when in a CREATE / MERGE clause | ||
| if(vctx->clause == CYPHER_AST_CREATE || vctx->clause == CYPHER_AST_MERGE) { | ||
| // clause name used for error reporting | ||
| const char *clause_name = (vctx->clause == CYPHER_AST_CREATE) ? | ||
| "CREATE" : | ||
| "MERGE"; | ||
|
|
||
| // validate that each relation has exactly one type | ||
| if(reltype_count != 1) { | ||
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, "CREATE"); | ||
| // CREATE ()-[:A:B]->() | ||
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name); | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| // Validate that each relation being created is directed | ||
| if(cypher_ast_rel_pattern_get_direction(n) == CYPHER_REL_BIDIRECTIONAL) { | ||
| // validate that each relation being created is directed | ||
| // CREATE ()-[]-() | ||
| if(dir == CYPHER_REL_BIDIRECTIONAL) { | ||
| ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP); | ||
| return VISITOR_BREAK; | ||
| } | ||
|
|
||
| // Validate that each relation being created is not variable length relationship | ||
| // validate that each relation being created is not of variable length | ||
| // CREATE ()-[*2..4]->() | ||
| if(range) { | ||
| ErrorCtx_SetError(EMSG_VAR_LEN, "CREATE"); | ||
| ErrorCtx_SetError(EMSG_VAR_LEN, clause_name); |
There was a problem hiding this comment.
Refactor the validation logic for relation patterns to simplify and enhance readability.
- if(vctx->clause == CYPHER_AST_CREATE || vctx->clause == CYPHER_AST_MERGE) {
- const char *clause_name = (vctx->clause == CYPHER_AST_CREATE) ? "CREATE" : "MERGE";
- if(reltype_count != 1) {
- ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name);
- return VISITOR_BREAK;
- }
- if(dir == CYPHER_REL_BIDIRECTIONAL) {
- ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP);
- return VISITOR_BREAK;
- }
- if(range) {
- ErrorCtx_SetError(EMSG_VAR_LEN, clause_name);
- return VISITOR_BREAK;
- }
- }
+ // Simplify the conditional structure and reduce nesting
+ const char *clause_name = (vctx->clause == CYPHER_AST_CREATE || vctx->clause == CYPHER_AST_MERGE) ?
+ (vctx->clause == CYPHER_AST_CREATE ? "CREATE" : "MERGE") : NULL;
+ if(clause_name) {
+ if(reltype_count != 1) ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name);
+ else if(dir == CYPHER_REL_BIDIRECTIONAL) ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP);
+ else if(range) ErrorCtx_SetError(EMSG_VAR_LEN, clause_name);
+ if(ErrorCtx_EncounteredError()) return VISITOR_BREAK;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | |
| enum cypher_rel_direction dir = cypher_ast_rel_pattern_get_direction(n); | |
| const cypher_astnode_t *range = cypher_ast_rel_pattern_get_varlength(n); | |
| if(vctx->clause == CYPHER_AST_CREATE) { | |
| // validate that the relation alias is not bound | |
| if(_ValidateCreateRelation(n, vctx) == AST_INVALID) { | |
| return VISITOR_BREAK; | |
| } | |
| // Validate that each relation has exactly one type | |
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | |
| // extra validation when in a CREATE / MERGE clause | |
| if(vctx->clause == CYPHER_AST_CREATE || vctx->clause == CYPHER_AST_MERGE) { | |
| // clause name used for error reporting | |
| const char *clause_name = (vctx->clause == CYPHER_AST_CREATE) ? | |
| "CREATE" : | |
| "MERGE"; | |
| // validate that each relation has exactly one type | |
| if(reltype_count != 1) { | |
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, "CREATE"); | |
| // CREATE ()-[:A:B]->() | |
| ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name); | |
| return VISITOR_BREAK; | |
| } | |
| // Validate that each relation being created is directed | |
| if(cypher_ast_rel_pattern_get_direction(n) == CYPHER_REL_BIDIRECTIONAL) { | |
| // validate that each relation being created is directed | |
| // CREATE ()-[]-() | |
| if(dir == CYPHER_REL_BIDIRECTIONAL) { | |
| ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP); | |
| return VISITOR_BREAK; | |
| } | |
| // Validate that each relation being created is not variable length relationship | |
| // validate that each relation being created is not of variable length | |
| // CREATE ()-[*2..4]->() | |
| if(range) { | |
| ErrorCtx_SetError(EMSG_VAR_LEN, "CREATE"); | |
| ErrorCtx_SetError(EMSG_VAR_LEN, clause_name); | |
| uint reltype_count = cypher_ast_rel_pattern_nreltypes(n); | |
| enum cypher_rel_direction dir = cypher_ast_rel_pattern_get_direction(n); | |
| const cypher_astnode_t *range = cypher_ast_rel_pattern_get_varlength(n); | |
| // extra validation when in a CREATE / MERGE clause | |
| const char *clause_name = (vctx->clause == CYPHER_AST_CREATE || vctx->clause == CYPHER_AST_MERGE) ? | |
| (vctx->clause == CYPHER_AST_CREATE ? "CREATE" : "MERGE") : NULL; | |
| if(clause_name) { | |
| if(reltype_count != 1) ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name); | |
| else if(dir == CYPHER_REL_BIDIRECTIONAL) ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP); | |
| else if(range) ErrorCtx_SetError(EMSG_VAR_LEN, clause_name); | |
| if(ErrorCtx_EncounteredError()) return VISITOR_BREAK; | |
| } |
tests/flow/test_bound_variables.py
Outdated
| # should not reach this point | ||
| self.env.assertFalse(True) | ||
| except Exception as e: | ||
| self.env.assertIn("Edge `e` can only be declared once", str(e)) |
There was a problem hiding this comment.
Refine the error message check to align with the new standard error message format.
- self.env.assertIn("Edge `e` can only be declared once", str(e))
+ self.env.assertIn("Edge 'e' can only be declared once", str(e))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| self.env.assertIn("Edge `e` can only be declared once", str(e)) | |
| self.env.assertIn("Edge 'e' can only be declared once", str(e)) |
swilly22
force-pushed
the
edge-reuse-validation
branch
from
eff8d1f
to
bb22b30
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/ast/ast_validations.c (5 hunks)
- tests/flow/test_bound_variables.py (7 hunks)
- tests/flow/test_create_clause.py (2 hunks)
- tests/flow/test_graph_create.py (1 hunks)
- tests/flow/test_null_handling.py (1 hunks)
Additional context used
Ruff
tests/flow/test_create_clause.py
1-1:
from common import *used; unable to detect undefined names (F403)
2-2:
from index_utils import *used; unable to detect undefined names (F403)
8-8:
Envmay be undefined, or defined from star imports (F405)tests/flow/test_bound_variables.py
1-1:
from common import *used; unable to detect undefined names (F403)
2-2:
from index_utils import *used; unable to detect undefined names (F403)
4-4:
sysmay be undefined, or defined from star imports (F405)
4-4:
osmay be undefined, or defined from star imports (F405)
4-4:
osmay be undefined, or defined from star imports (F405)
8-8:
FlowTestsBasemay be undefined, or defined from star imports (F405)
11-11:
Envmay be undefined, or defined from star imports (F405)
22-22:
Nodemay be undefined, or defined from star imports (F405)
27-27:
Edgemay be undefined, or defined from star imports (F405)
28-28:
Edgemay be undefined, or defined from star imports (F405)
58-58:
create_node_fulltext_indexmay be undefined, or defined from star imports (F405)
130-130: Local variable
resis assigned to but never used (F841)tests/flow/test_null_handling.py
1-1:
from common import *used; unable to detect undefined names (F403)
5-5:
FlowTestsBasemay be undefined, or defined from star imports (F405)
7-7:
Envmay be undefined, or defined from star imports (F405)
21-21:
redismay be undefined, or defined from star imports (F405)
29-29:
redismay be undefined, or defined from star imports (F405)
39-39:
redismay be undefined, or defined from star imports (F405)
47-47:
redismay be undefined, or defined from star imports (F405)
104-104:
redismay be undefined, or defined from star imports (F405)tests/flow/test_graph_create.py
1-1:
from common import *used; unable to detect undefined names (F403)
5-5:
FlowTestsBasemay be undefined, or defined from star imports (F405)
7-7:
Envmay be undefined, or defined from star imports (F405)
50-50:
Nodemay be undefined, or defined from star imports (F405)
74-74:
redismay be undefined, or defined from star imports (F405)
105-105:
redismay be undefined, or defined from star imports (F405)
144-144:
redismay be undefined, or defined from star imports (F405)
151-151:
redismay be undefined, or defined from star imports (F405)
Additional comments not posted (4)
tests/flow/test_create_clause.py (1)
59-59: Refine the error message check to ensure it precisely matches the new standard error message for edge redeclaration.- self.env.assertTrue("The bound edge 'e' can't be redeclared in a CREATE clause" in str(e)) + self.env.assertTrue("Edge 'e' can only be declared once" in str(e))Also applies to: 70-70
tests/flow/test_bound_variables.py (1)
134-134: Refine the error message check to align with the new standard error message format.- self.env.assertIn("The bound edge 'e' can't be redeclared in a MERGE clause", str(e)) + self.env.assertIn("Edge 'e' can only be declared once", str(e))src/ast/ast_validations.c (2)
739-768: Refined validation logic for relation patterns in CREATE and MERGE clauses.The changes correctly implement additional checks for relation types, ensuring each relation has exactly one type and is directed, and that variable-length relations are not allowed in CREATE and MERGE clauses. This aligns with the PR objectives to prevent incorrect usage of edge variables in multiple MATCH clauses.
1677-1677: Ensure proper error handling when redeclaring an edge in a CREATE clause.The addition of an error message when an edge is redeclared in a CREATE clause is a good practice. It prevents potential conflicts or logical errors in graph database operations, aligning with the overall goal of enhancing query validation.
Resolves #465
Unlike nodes, edges always declare alias
Example:
Here
ais declared by the firstMATCHclause andais considered bounded by the secondMATCHclause.This behavior isn't applicable for edges
In this case
eis declared by BOTHMATCHclauses, which is not allowed.Summary by CodeRabbit
Bug Fixes
CREATEandMERGEclauses to handle relation types, directions, and properties accurately.Tests
test07_null_graph_entity_inputsto manage specific error scenarios during query execution.