feat (query/v2): Query Builder#75
Open
zinic wants to merge 25 commits intoSpecterOps:mainfrom
Open
Conversation
… expose shortest path helper functions
WalkthroughIntroduces an experimental fluent Cypher query builder ( ChangesQuery v2 Builder Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
query/v2/backend_test.go (1)
78-86: 💤 Low valueDocument the parameter-remapping boundary.
NamedParameter("props", ...)producespreparedQuery.Parameters["props"]but afterneo4j.NewQueryBuilder.Prepare(), the same value appears asqueryBuilder.Parameters["p0"]. The Neo4j backend re-derives its own parameter map from the AST, dropping the user-supplied symbol names. A short inline comment here would prevent future readers from mistakenly passingpreparedQuery.Parametersto the Neo4j driver instead ofqueryBuilder.Parameters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@query/v2/backend_test.go` around lines 78 - 86, Add a short inline comment in the test case near the "create node" example clarifying that NamedParameter("props", ...) yields preparedQuery.Parameters["props"] but neo4j.NewQueryBuilder.Prepare() (or the Neo4j backend) remaps parameter names (e.g., to "p0") and the actual map passed to the driver is queryBuilder.Parameters; mention NamedParameter, Prepare, preparedQuery.Parameters and queryBuilder.Parameters by name so future readers know not to pass preparedQuery.Parameters directly to the Neo4j driver.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cypher/models/pgsql/test/query_test.go`:
- Around line 31-39: The loop currently continues after queryBuilder.Build()
fails and then dereferences builtQuery on the next lines; change the failure
handling for Build() so the test stops or skips the iteration—either replace
t.Errorf("could not build query: %v", err) with t.Fatalf(...) (or
require.NoError(t, err)) to abort the test, or add a continue after the t.Errorf
to skip using builtQuery; apply the same pattern wherever builtQuery is
dereferenced (e.g., before calling translate.Translate) to ensure no dereference
happens after a failed Build().
In `@query/v2/query.go`:
- Around line 687-695: The Skip and Limit setters currently allow negatives and
callers elsewhere treat zero as “unset”; change builder.Skip and builder.Limit
to validate and preserve zero by (1) rejecting negative values at the setter
(e.g., if skip < 0 or limit < 0, panic or return an explicit error/validation
failure) and (2) always assign the pointer (s.skip = &skip and s.limit = &limit)
even when the value is 0 so presence checks can use != nil; apply the same fix
to the other identical setters around the 1044-1081 region so all zero values
are preserved and negatives are rejected.
- Around line 818-852: Delete() currently allows property lookups via
QualifiedExpression (e.g., Delete(Node().Property("name"))); detect and reject
these by adding a check in the loop that handles QualifiedExpression (the branch
for QualifiedExpression in the deleteItems iteration): after obtaining qualifier
:= typedNextUpdate.qualifier() and before accepting it, identify if qualifier
represents a property access (the form used by property lookups) and if so call
s.trackError with a clear message (e.g., "cannot delete property; use Remove()")
and continue; leave the existing validateExpressionValue,
isDetachDeleteQualifier, s.detachDelete, s.deleteItems and pendingDeleteItems
logic unchanged for non-property qualifiers so only property-access targets are
rejected.
In `@query/v2/util.go`:
- Around line 614-751: CollectFromValue currently doesn't handle raw
*cypher.ProjectionItem values so identifiers inside projection items (e.g.
Return(As(Node(), "alias"))) aren't recorded; add a case for
*cypher.ProjectionItem in CollectFromValue that extracts the inner expression
(the projected expression) and calls s.CollectFromExpression (or
s.CollectFromValue if you prefer) on it so variable identifiers inside
projection items get collected—update the switch in CollectFromValue to mirror
how *cypher.Return is expanded via projectionItemsFromReturn and ensure
ProjectionItem is handled the same way to prevent missing MATCH generation in
Build.
---
Nitpick comments:
In `@query/v2/backend_test.go`:
- Around line 78-86: Add a short inline comment in the test case near the
"create node" example clarifying that NamedParameter("props", ...) yields
preparedQuery.Parameters["props"] but neo4j.NewQueryBuilder.Prepare() (or the
Neo4j backend) remaps parameter names (e.g., to "p0") and the actual map passed
to the driver is queryBuilder.Parameters; mention NamedParameter, Prepare,
preparedQuery.Parameters and queryBuilder.Parameters by name so future readers
know not to pass preparedQuery.Parameters directly to the Neo4j driver.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bf23e1a-4e41-4b27-97dd-3600a9ebc32c
📒 Files selected for processing (10)
cypher/models/cypher/format/format.gocypher/models/pgsql/test/query_test.gocypher/models/pgsql/translate/translator.goquery/neo4j/neo4j.goquery/v2/backend_test.goquery/v2/compat.goquery/v2/doc.goquery/v2/query.goquery/v2/query_test.goquery/v2/util.go
Comment on lines
+31
to
39
| for _, queryBuilder := range queries { | ||
| builtQuery, err := queryBuilder.Build() | ||
| if err != nil { | ||
| t.Errorf("could not build query: %v", err) | ||
| } | ||
|
|
||
| translatedQuery, err := translate.Translate(context.Background(), builtQuery, mapper, nil) | ||
| translatedQuery, err := translate.Translate(context.Background(), builtQuery.Query, mapper, builtQuery.Parameters) | ||
| if err != nil { | ||
| t.Errorf("could not translate query: %#v: %v", builtQuery, err) |
There was a problem hiding this comment.
Stop after Build() fails.
t.Errorf keeps the loop running, but Line 37 dereferences builtQuery even when Build() returned an error. Use t.Fatalf/require.NoError, or continue after the failure paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cypher/models/pgsql/test/query_test.go` around lines 31 - 39, The loop
currently continues after queryBuilder.Build() fails and then dereferences
builtQuery on the next lines; change the failure handling for Build() so the
test stops or skips the iteration—either replace t.Errorf("could not build
query: %v", err) with t.Fatalf(...) (or require.NoError(t, err)) to abort the
test, or add a continue after the t.Errorf to skip using builtQuery; apply the
same pattern wherever builtQuery is dereferenced (e.g., before calling
translate.Translate) to ensure no dereference happens after a failed Build().
Comment on lines
+687
to
+695
| func (s *builder) Skip(skip int) QueryBuilder { | ||
| s.skip = &skip | ||
| return s | ||
| } | ||
|
|
||
| func (s *builder) Limit(limit int) QueryBuilder { | ||
| s.limit = &limit | ||
| return s | ||
| } |
There was a problem hiding this comment.
Preserve explicit Skip(0) / Limit(0) and reject negatives.
These checks treat zero as “unset”, so Limit(0) returns all rows instead of none, and negative values are silently ignored. Presence should be based on != nil, with non-negative validation at the setter.
Suggested fix
func (s *builder) Skip(skip int) QueryBuilder {
+ if skip < 0 {
+ s.trackError(fmt.Errorf("skip must be non-negative"))
+ return s
+ }
+
s.skip = &skip
return s
}
func (s *builder) Limit(limit int) QueryBuilder {
+ if limit < 0 {
+ s.trackError(fmt.Errorf("limit must be non-negative"))
+ return s
+ }
+
s.limit = &limit
return s
}
@@
var (
hasProjectedItems = len(s.projections) > 0
- hasSkip = s.skip != nil && *s.skip > 0
- hasLimit = s.limit != nil && *s.limit > 0
+ hasSkip = s.skip != nil
+ hasLimit = s.limit != nil
requiresProjection = hasProjectedItems || hasSkip || hasLimit
)
@@
- if s.skip != nil && *s.skip > 0 {
+ if s.skip != nil {
projection.Skip = cypher.NewSkip(*s.skip)
}
- if s.limit != nil && *s.limit > 0 {
+ if s.limit != nil {
projection.Limit = cypher.NewLimit(*s.limit)
}Also applies to: 1044-1081
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@query/v2/query.go` around lines 687 - 695, The Skip and Limit setters
currently allow negatives and callers elsewhere treat zero as “unset”; change
builder.Skip and builder.Limit to validate and preserve zero by (1) rejecting
negative values at the setter (e.g., if skip < 0 or limit < 0, panic or return
an explicit error/validation failure) and (2) always assign the pointer (s.skip
= &skip and s.limit = &limit) even when the value is 0 so presence checks can
use != nil; apply the same fix to the other identical setters around the
1044-1081 region so all zero values are preserved and negatives are rejected.
Comment on lines
+818
to
+852
| for _, nextDelete := range deleteItems { | ||
| switch typedNextUpdate := nextDelete.(type) { | ||
| case QualifiedExpression: | ||
| qualifier := typedNextUpdate.qualifier() | ||
| if err := validateExpressionValue(qualifier, "delete expression"); err != nil { | ||
| s.trackError(err) | ||
| continue | ||
| } | ||
|
|
||
| if isDetachDeleteQualifier(qualifier, s.identifiers) { | ||
| s.detachDelete = true | ||
| pendingDetachDelete = true | ||
| } | ||
|
|
||
| s.deleteItems = append(s.deleteItems, qualifier) | ||
| pendingDeleteItems = append(pendingDeleteItems, qualifier) | ||
|
|
||
| case *cypher.Variable: | ||
| if err := validateExpressionValue(typedNextUpdate, "delete expression"); err != nil { | ||
| s.trackError(err) | ||
| continue | ||
| } | ||
|
|
||
| switch typedNextUpdate.Symbol { | ||
| case s.identifiers.node, s.identifiers.start, s.identifiers.end: | ||
| s.detachDelete = true | ||
| pendingDetachDelete = true | ||
| } | ||
|
|
||
| s.deleteItems = append(s.deleteItems, typedNextUpdate) | ||
| pendingDeleteItems = append(pendingDeleteItems, typedNextUpdate) | ||
|
|
||
| default: | ||
| s.trackError(fmt.Errorf("unknown delete type: %T", nextDelete)) | ||
| } |
There was a problem hiding this comment.
Reject property lookups in Delete().
Delete() currently accepts any QualifiedExpression, so Delete(Node().Property("name")) builds a delete target from n.name. That is not a valid Cypher delete target; callers should use Remove() for properties.
Suggested fix
for _, nextDelete := range deleteItems {
switch typedNextUpdate := nextDelete.(type) {
case QualifiedExpression:
qualifier := typedNextUpdate.qualifier()
- if err := validateExpressionValue(qualifier, "delete expression"); err != nil {
+ variable, typeOK := qualifier.(*cypher.Variable)
+ if !typeOK || variable == nil {
+ s.trackError(fmt.Errorf("delete expression must be a variable reference, got %T", qualifier))
+ continue
+ }
+
+ if err := validateExpressionValue(variable, "delete expression"); err != nil {
s.trackError(err)
continue
}
- if isDetachDeleteQualifier(qualifier, s.identifiers) {
+ if isDetachDeleteQualifier(variable, s.identifiers) {
s.detachDelete = true
pendingDetachDelete = true
}
- s.deleteItems = append(s.deleteItems, qualifier)
- pendingDeleteItems = append(pendingDeleteItems, qualifier)
+ s.deleteItems = append(s.deleteItems, variable)
+ pendingDeleteItems = append(pendingDeleteItems, variable)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@query/v2/query.go` around lines 818 - 852, Delete() currently allows property
lookups via QualifiedExpression (e.g., Delete(Node().Property("name"))); detect
and reject these by adding a check in the loop that handles QualifiedExpression
(the branch for QualifiedExpression in the deleteItems iteration): after
obtaining qualifier := typedNextUpdate.qualifier() and before accepting it,
identify if qualifier represents a property access (the form used by property
lookups) and if so call s.trackError with a clear message (e.g., "cannot delete
property; use Remove()") and continue; leave the existing
validateExpressionValue, isDetachDeleteQualifier, s.detachDelete, s.deleteItems
and pendingDeleteItems logic unchanged for non-property qualifiers so only
property-access targets are rejected.
Comment on lines
+614
to
+751
| func (s *identifierSet) CollectFromValue(value any) error { | ||
| switch typedValue := value.(type) { | ||
| case nil: | ||
| return nil | ||
|
|
||
| case QualifiedExpression: | ||
| return s.CollectFromExpression(typedValue.qualifier()) | ||
|
|
||
| case *cypher.Return: | ||
| if projectionItems, err := projectionItemsFromReturn(typedValue); err != nil { | ||
| return err | ||
| } else { | ||
| for _, projectionItem := range projectionItems { | ||
| if err := s.CollectFromExpression(projectionItem); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| case *cypher.Order: | ||
| if sortItems, err := sortItemsFromOrder(typedValue); err != nil { | ||
| return err | ||
| } else { | ||
| for _, sortItem := range sortItems { | ||
| if err := s.CollectFromExpression(sortItem); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| case *cypher.SortItem: | ||
| if sortItem, err := sortItemFromValue(typedValue); err != nil { | ||
| return err | ||
| } else { | ||
| return s.CollectFromExpression(sortItem) | ||
| } | ||
|
|
||
| case *cypher.Set: | ||
| if setItems, err := setItemsFromSet(typedValue); err != nil { | ||
| return err | ||
| } else { | ||
| for _, setItem := range setItems { | ||
| if err := s.CollectFromExpression(setItem); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| case *cypher.SetItem: | ||
| if setItem, err := setItemFromValue(typedValue); err != nil { | ||
| return err | ||
| } else { | ||
| return s.CollectFromExpression(setItem) | ||
| } | ||
|
|
||
| case *cypher.Remove: | ||
| if removeItems, err := removeItemsFromRemove(typedValue); err != nil { | ||
| return err | ||
| } else { | ||
| for _, removeItem := range removeItems { | ||
| if err := s.CollectFromValue(removeItem); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| case *cypher.RemoveItem: | ||
| if removeItem, err := removeItemFromValue(typedValue); err != nil { | ||
| return err | ||
| } else if removeItem.KindMatcher != nil { | ||
| return s.CollectFromExpression(removeItem.KindMatcher) | ||
| } else { | ||
| return s.CollectFromExpression(removeItem) | ||
| } | ||
|
|
||
| case *cypher.NodePattern: | ||
| if err := validateNodePattern(typedValue); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return s.CollectFromExpression(typedValue) | ||
|
|
||
| case *cypher.RelationshipPattern: | ||
| if err := validateRelationshipPattern(typedValue); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return s.CollectFromExpression(typedValue) | ||
|
|
||
| case *cypher.Variable: | ||
| return s.CollectFromExpression(typedValue) | ||
|
|
||
| case *cypher.FunctionInvocation: | ||
| return s.CollectFromExpression(typedValue) | ||
|
|
||
| case *cypher.PropertyLookup: | ||
| return s.CollectFromExpression(typedValue) | ||
|
|
||
| case []any: | ||
| for _, item := range typedValue { | ||
| if err := s.CollectFromValue(item); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| case []cypher.SyntaxNode: | ||
| for _, item := range typedValue { | ||
| if err := s.CollectFromValue(item); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| case []cypher.Expression: | ||
| for _, item := range typedValue { | ||
| if err := s.CollectFromValue(item); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| case []*cypher.SetItem: | ||
| for _, item := range typedValue { | ||
| if err := s.CollectFromValue(item); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| case []*cypher.RemoveItem: | ||
| for _, item := range typedValue { | ||
| if err := s.CollectFromValue(item); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| default: | ||
| return nil | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Track identifiers inside raw *cypher.ProjectionItem values.
CollectFromValue skips *cypher.ProjectionItem, so Return(As(Node(), "alias")) never records n as a read identifier. Build() can then emit return n as alias without generating a MATCH, which leaves n unresolved.
Suggested fix
case *cypher.Return:
if projectionItems, err := projectionItemsFromReturn(typedValue); err != nil {
return err
} else {
for _, projectionItem := range projectionItems {
if err := s.CollectFromExpression(projectionItem); err != nil {
return err
}
}
}
+ case *cypher.ProjectionItem:
+ if projectionItem, err := projectionItemFromValue(typedValue); err != nil {
+ return err
+ } else {
+ return s.CollectFromExpression(projectionItem)
+ }
+
case *cypher.Order:
if sortItems, err := sortItemsFromOrder(typedValue); err != nil {
return err
} else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@query/v2/util.go` around lines 614 - 751, CollectFromValue currently doesn't
handle raw *cypher.ProjectionItem values so identifiers inside projection items
(e.g. Return(As(Node(), "alias"))) aren't recorded; add a case for
*cypher.ProjectionItem in CollectFromValue that extracts the inner expression
(the projected expression) and calls s.CollectFromExpression (or
s.CollectFromValue if you prefer) on it so variable identifiers inside
projection items get collected—update the switch in CollectFromValue to mirror
how *cypher.Return is expanded via projectionItemsFromReturn and ensure
ProjectionItem is handled the same way to prevent missing MATCH generation in
Build.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
v2Cypher query builder with fluent API for constructing queries with match, update, delete, create, and projection clauses.Improvements