Implement Arithmetic and Concat Expressions#127
Conversation
Implement arithmetic operators (+, -, *, /, %) and string concatenation in the Rain ORM query builder. - Add BinaryExpr and ConcatExpr to pkg/schema - Add Add, Sub, Mul, Div, Mod methods to Column[T] and AnyColumn - Support expression chaining (e.g., col.Add(1).Mul(2)) - Implement dialect-aware concatenation (|| for Postgres/SQLite, CONCAT() for MySQL) - Support aliasing via .As() on new expression types - Update query compiler to render expressions with correct precedence - Add unit and SQLite integration tests Co-authored-by: cungminh2710 <8063319+cungminh2710@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Greptile SummaryThis PR adds arithmetic (
Confidence Score: 5/5The change is additive and well-scoped; existing query paths are unaffected and new expression types are gated behind operator allowlists in the compiler. All new code paths are covered by both unit and integration tests. The operator allowlists prevent injection, parenthesisation is correct, and dialect branching is explicit. The only issue found is a stale copy-paste comment in one test; the assertion itself is correct so no test coverage is lost. No files require special attention; the minor comment discrepancy in sqlite_integration_test.go does not affect correctness.
|
| Filename | Overview |
|---|---|
| pkg/schema/schema.go | Adds BinaryExpr and ConcatExpr types with full arithmetic/concat helpers on AnyColumn and Column[T], plus cloneExpressionForTable support for the new types. |
| pkg/rain/query_compile.go | Adds compiler cases for BinaryExpr (operator allowlist + parenthesisation) and ConcatExpr (dialect-aware |
| pkg/rain/expressions_test.go | New unit tests for SQL generation of arithmetic and Concat across postgres and mysql dialects; no logic issues. |
| pkg/rain/sqlite_integration_test.go | Integration test for all five arithmetic operations and Concat against live SQLite. One misleading comment (Sub: 10-3=5 should be =7); assertion is correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Column[T].Add/Sub/Mul/Div/Mod(value T)"] --> B[BinaryExpr]
C["AnyColumn.Add/Sub/Mul/Div/Mod(value any)"] --> B
B --> D["BinaryExpr.Add/Sub/Mul/Div/Mod(value any) chaining"]
D --> B
E["schema.Concat(values ...any)"] --> F[ConcatExpr]
B --> G{Compiler}
F --> G
G -->|BinaryExpr| H{"Operator allowlist + - * / %"}
H -->|valid| I["( Left op Right )"]
H -->|invalid| J[return error]
G -->|ConcatExpr| K{Dialect?}
K -->|postgres / sqlite| L["( e1 || e2 || ... )"]
K -->|mysql| M["CONCAT(e1, e2, ...)"]
K -->|other| N[return error]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["Column[T].Add/Sub/Mul/Div/Mod(value T)"] --> B[BinaryExpr]
C["AnyColumn.Add/Sub/Mul/Div/Mod(value any)"] --> B
B --> D["BinaryExpr.Add/Sub/Mul/Div/Mod(value any) chaining"]
D --> B
E["schema.Concat(values ...any)"] --> F[ConcatExpr]
B --> G{Compiler}
F --> G
G -->|BinaryExpr| H{"Operator allowlist + - * / %"}
H -->|valid| I["( Left op Right )"]
H -->|invalid| J[return error]
G -->|ConcatExpr| K{Dialect?}
K -->|postgres / sqlite| L["( e1 || e2 || ... )"]
K -->|mysql| M["CONCAT(e1, e2, ...)"]
K -->|other| N[return error]
Reviews (2): Last reviewed commit: "feat(rain): add support for arithmetic a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func (NullCheckExpr) isExpression() {} | ||
| func (NullCheckExpr) isPredicate() {} | ||
|
|
||
| // BinaryExpr represents an arithmetic operation. | ||
| type BinaryExpr struct { | ||
| Left Expression | ||
| Operator string | ||
| Right Expression | ||
| } | ||
|
|
There was a problem hiding this comment.
Operator field is exported with no validation
BinaryExpr.Operator is a plain string written verbatim into the SQL output in writeExpressionInContext. The built-in factory methods (Add, Sub, etc.) only ever write safe operators, but because the struct and its fields are exported, a caller can construct schema.BinaryExpr{Operator: arbitraryString} and inject arbitrary SQL. The existing ComparisonExpr has the same pattern, so this is consistent — but that means both types carry the same risk. Consider either unexported fields with constructor functions, or a validation allowlist in the compiler.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/schema/schema.go
Line: 1049-1058
Comment:
**`Operator` field is exported with no validation**
`BinaryExpr.Operator` is a plain `string` written verbatim into the SQL output in `writeExpressionInContext`. The built-in factory methods (`Add`, `Sub`, etc.) only ever write safe operators, but because the struct and its fields are exported, a caller can construct `schema.BinaryExpr{Operator: arbitraryString}` and inject arbitrary SQL. The existing `ComparisonExpr` has the same pattern, so this is consistent — but that means both types carry the same risk. Consider either unexported fields with constructor functions, or a validation allowlist in the compiler.
How can I resolve this? If you propose a fix, please make it concise.
|
@jules Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes. Issue 1 of 3pkg/rain/sqlite_integration_test.go:1988-2010 These two subtests ( Issue 2 of 3pkg/schema/schema.go:1049-1058
Issue 3 of 3pkg/rain/sqlite_integration_test.go:1955-1969 The integration test covers |
@jules I've addressed the feedback by making the following changes:
All tests passed successfully. |
Implement arithmetic operators (+, -, *, /, %) and string concatenation in the Rain ORM query builder. - Add BinaryExpr and ConcatExpr to pkg/schema - Add Add, Sub, Mul, Div, Mod methods to Column[T] and AnyColumn - Support expression chaining (e.g., col.Add(1).Mul(2)) - Implement dialect-aware concatenation (|| for Postgres/SQLite, CONCAT() for MySQL) - Support aliasing via .As() on new expression types - Update query compiler to render expressions with correct precedence - Add operator validation allowlist to prevent SQL injection - Add unit and SQLite integration tests covering all operators Co-authored-by: cungminh2710 <8063319+cungminh2710@users.noreply.github.com>
cungminh2710
deleted the
feat/arithmetic-and-concat-expressions-12834576655270581647
branch
1 participant
This change introduces basic SQL arithmetic operations and string concatenation to Rain ORM, improving its parity with Drizzle ORM.
Key features:
.Add(),.Sub(),.Mul(),.Div(), and.Mod(). These can be chained together.schema.Concat()helper provides dialect-aware string concatenation.PR created automatically by Jules for task 12834576655270581647 started by @cungminh2710