feat: Advanced Update and Delete support (CTEs, Order, Limit)#94
Conversation
This change enhances UpdateQuery and DeleteQuery to support advanced SQL clauses, bringing them closer to parity with Drizzle ORM. - Added new dialect features for Update/Delete Order and Limit. - Enabled CTE support for MySQL and SQLite. - Refactored shared SQL rendering logic into pkg/rain/query_common.go. - Implemented With(), OrderBy(), and Limit() on UpdateQuery and DeleteQuery. - Added comprehensive unit tests for all new functionality. 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 extends
Confidence Score: 4/5Safe to merge for UPDATE/DELETE paths; the SELECT refactor carries a silent API change that could cause existing callers to get empty result sets instead of full ones. The write-query additions (CTEs, ORDER BY, LIMIT on UPDATE/DELETE) are well-tested and the dialect feature gating is correct. The risky part is the SELECT refactor: migrating pkg/rain/query_select.go — the
|
| Filename | Overview |
|---|---|
| pkg/rain/query_common.go | Adds shared writeCTEs and writeOrderLimit helpers used by all three query types; writeOrderLimit correctly gate-checks dialect features for ORDER BY and LIMIT |
| pkg/rain/query_select.go | Refactors SELECT to use shared helpers and changes limit/offset to *int — Limit(0) now emits LIMIT 0 instead of being a no-op, which is a silent breaking change for callers who relied on 0 meaning "no limit" |
| pkg/rain/query_update.go | Adds CTE (With), OrderBy, and Limit support for UPDATE; dialect feature checks correctly gate MySQL/SQLite-only clauses |
| pkg/rain/query_delete.go | Adds CTE (With), OrderBy, and Limit support for DELETE; ordering relative to WHERE and RETURNING clauses is correct |
| pkg/dialect/mysql.go | Adds CTE/UpdateOrder/UpdateLimit/DeleteOrder/DeleteLimit/Unlimited features; changes LimitOffset guard from limit > 0 to limit >= 0 so explicit LIMIT 0 is now rendered |
| pkg/dialect/feature.go | Adds five new feature flags; FeatureUnlimited is used as a sentinel in writeOrderLimit to skip feature checks for SELECT, and HasFeature now correctly handles a zero feature argument |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ToSQL called] --> B{Query type}
B --> |SELECT| C[writeCTEs featureOrder=FeatureUnlimited featureLimit=FeatureUnlimited]
B --> |UPDATE| D[writeCTEs featureOrder=FeatureUpdateOrder featureLimit=FeatureUpdateLimit]
B --> |DELETE| E[writeCTEs featureOrder=FeatureDeleteOrder featureLimit=FeatureDeleteLimit]
C --> F[writeOrderLimit]
D --> F
E --> F
F --> G{featureOrder == FeatureUnlimited?}
G --> |Yes - SELECT| H[Skip ORDER BY check]
G --> |No - UPDATE/DELETE| I{HasFeature ORDER flag?}
I --> |No - e.g. Postgres| J[Return error]
I --> |Yes - MySQL/SQLite| H
H --> K{featureLimit == FeatureUnlimited?}
K --> |Yes - SELECT| L[Skip LIMIT check]
K --> |No - UPDATE/DELETE| M{HasFeature LIMIT flag?}
M --> |No - e.g. Postgres| N[Return error]
M --> |Yes - MySQL/SQLite| L
L --> O[dialect.LimitOffset limit=-1 if nil else ptr value]
O --> P[Emit clause if non-empty]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
pkg/rain/query_select.go:158-163
**`Limit(0)` on SELECT now emits `LIMIT 0` — silent breaking change**
Before this PR `SelectQuery.limit` was `int` with a zero default, and `LimitOffset(0, 0)` always returned `""` (all three dialect implementations guarded with `limit > 0`). As a result, calling `.Limit(0)` was a no-op: all rows were returned. After this PR the field is `*int`, so `.Limit(0)` stores a non-nil pointer; `writeOrderLimit` then calls `LimitOffset(0, 0)` which now returns `"LIMIT 0"` (zero rows returned).
Any caller that relied on `Limit(0)` to mean "no limit" — for example a config-driven value that defaults to zero — will now silently receive an empty result set on every dialect instead of all rows. There is no test covering this case and no mention of the breaking change in the PR description.
Reviews (2): Last reviewed commit: "refactor(rain): improve ORDER BY/LIMIT s..." | Re-trigger Greptile
|
@jules Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes. Issue 1 of 3pkg/rain/query_common.go:124-128 Issue 2 of 3pkg/rain/query_update.go:73-76
Issue 3 of 3pkg/rain/query_common.go:124-158 The old |
I have addressed all three feedback items:
|
- Treat zero-value dialect features as "deny" in writeOrderLimit for safer defaults. - Introduce FeatureUnlimited for queries that always allow Order/Limit (SELECT). - Change limit and offset to *int to support explicit Limit(0). - Return error for negative limit/offset values. - Update PostgreSQL, MySQL, and SQLite dialects to enable FeatureUnlimited. - Ensure consistent LIMIT syntax across dialects for offset-only and zero-limit cases. Co-authored-by: cungminh2710 <8063319+cungminh2710@users.noreply.github.com>
Enhanced Rain ORM's Update and Delete query builders to support
WITH(CTEs),ORDER BY, andLIMITclauses. This parity improvement allows developers to write more complex data modification queries for MySQL and SQLite, where these features are natively supported. The implementation includes dialect-aware feature gating and refactored common rendering logic to ensure consistency across the query builder.PR created automatically by Jules for task 347139410859395563 started by @cungminh2710