feat(rain): implement First, TxOptions, and relation column selection#123
feat(rain): implement First, TxOptions, and relation column selection#123cungminh2710 wants to merge 2 commits into
Conversation
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 three feature-parity improvements to the rain ORM:
Confidence Score: 3/5The transaction and relation-column-restriction changes are safe to merge;
pkg/rain/prepared_select.go — the new
|
| Filename | Overview |
|---|---|
| pkg/rain/prepared_select.go | Adds PreparedSelectQuery.First, but it is a direct alias to Scan with no LIMIT 1 applied — unlike SelectQuery.First, which clones and adds LIMIT 1 before executing. All matching rows are fetched from the database on every call. |
| pkg/rain/query_select.go | Adds SelectQuery.First (correctly applies LIMIT 1 on a clone) and adds Columns []schema.Expression to RelationConfig. Both changes are straightforward and correct. |
| pkg/rain/rain.go | Refactors Begin/RunInTx to delegate to new BeginTx/RunInTxOpts methods that accept *sql.TxOptions. The refactoring is clean and backward-compatible. |
| pkg/rain/relation_loading.go | Applies config.Columns column restriction to both loadRelatedRows and loadRelatedManyToManyRows, with a safety check to auto-include TargetColumn. Logic is correct but duplicated in two places. |
Sequence Diagram
sequenceDiagram
participant C as Caller
participant SQ as SelectQuery
participant PSQ as PreparedSelectQuery
participant DB as Database
Note over C,DB: SelectQuery.First (correct)
C->>SQ: First(ctx, dest)
SQ->>SQ: clone().Limit(1)
SQ->>DB: SELECT ... LIMIT 1
DB-->>SQ: ≤1 row
SQ-->>C: first row / ErrNoRows
Note over C,DB: PreparedSelectQuery.First (missing LIMIT 1)
C->>PSQ: First(ctx, args, dest)
PSQ->>PSQ: Scan(ctx, args, dest) [no LIMIT 1]
PSQ->>DB: SELECT ... (no LIMIT 1, compiled at Prepare time)
DB-->>PSQ: ALL matching rows
PSQ-->>C: first row consumed, rest discarded
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
pkg/rain/prepared_select.go:90-94
**`PreparedSelectQuery.First` does not apply LIMIT 1**
`SelectQuery.First` applies `LIMIT 1` at the database level via `q.clone().Limit(1).Scan(...)`, so only one row is ever transferred from the server. `PreparedSelectQuery.First` simply delegates to `p.Scan`, which executes the pre-compiled statement with no `LIMIT 1` in its SQL — the database returns all matching rows, and only the first is consumed by the struct scanner. A prepared query with 100k matching rows will transfer all of them on every `First()` call, silently.
Because the SQL is frozen at `Prepare()` time, the only sound options are: (a) remove `First` from `PreparedSelectQuery` and require callers to build the query with `.Limit(1)` before calling `.Prepare()`, or (b) clearly document that no `LIMIT 1` is applied and that callers must add it themselves when constructing the original `SelectQuery`.
### Issue 2 of 2
pkg/rain/relation_loading.go:298-314
**Duplicated target-column safety check**
The exact same block that checks `config.Columns` and auto-appends `relation.TargetColumn` if missing appears verbatim in both `loadRelatedManyToManyRows` (lines 298–311) and `loadRelatedRows` (lines 393–406). Extracting this into a small helper function (e.g. `ensureTargetColumnSelected(query *SelectQuery, columns []schema.Expression, targetCol *schema.ColumnDef)`) would eliminate the duplication and make future changes (e.g. adding a similar guard for `SourceColumn` on `BelongsTo` relations) a single-line call in each load function.
Reviews (1): Last reviewed commit: "feat(rain): implement First, TxOptions, ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // First executes the prepared SELECT query and scans the first row into dest. | ||
| // Returns sql.ErrNoRows if no result is found. | ||
| func (p *PreparedSelectQuery) First(ctx context.Context, args PreparedArgs, dest any) error { | ||
| return p.Scan(ctx, args, dest) | ||
| } |
There was a problem hiding this comment.
PreparedSelectQuery.First does not apply LIMIT 1
SelectQuery.First applies LIMIT 1 at the database level via q.clone().Limit(1).Scan(...), so only one row is ever transferred from the server. PreparedSelectQuery.First simply delegates to p.Scan, which executes the pre-compiled statement with no LIMIT 1 in its SQL — the database returns all matching rows, and only the first is consumed by the struct scanner. A prepared query with 100k matching rows will transfer all of them on every First() call, silently.
Because the SQL is frozen at Prepare() time, the only sound options are: (a) remove First from PreparedSelectQuery and require callers to build the query with .Limit(1) before calling .Prepare(), or (b) clearly document that no LIMIT 1 is applied and that callers must add it themselves when constructing the original SelectQuery.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/prepared_select.go
Line: 90-94
Comment:
**`PreparedSelectQuery.First` does not apply LIMIT 1**
`SelectQuery.First` applies `LIMIT 1` at the database level via `q.clone().Limit(1).Scan(...)`, so only one row is ever transferred from the server. `PreparedSelectQuery.First` simply delegates to `p.Scan`, which executes the pre-compiled statement with no `LIMIT 1` in its SQL — the database returns all matching rows, and only the first is consumed by the struct scanner. A prepared query with 100k matching rows will transfer all of them on every `First()` call, silently.
Because the SQL is frozen at `Prepare()` time, the only sound options are: (a) remove `First` from `PreparedSelectQuery` and require callers to build the query with `.Limit(1)` before calling `.Prepare()`, or (b) clearly document that no `LIMIT 1` is applied and that callers must add it themselves when constructing the original `SelectQuery`.
How can I resolve this? If you propose a fix, please make it concise.
| if len(config.Columns) > 0 { | ||
| targetQuery.Column(config.Columns...) | ||
| // Ensure mapping target column is selected. | ||
| found := false | ||
| for _, col := range config.Columns { | ||
| if cr, ok := col.(schema.ColumnReference); ok && cr.ColumnDef() == relation.TargetColumn { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| targetQuery.Column(schema.Ref(relation.TargetColumn)) | ||
| } | ||
| } | ||
| if config.Where != nil { | ||
| targetQuery.Where(config.Where) | ||
| } |
There was a problem hiding this comment.
The exact same block that checks config.Columns and auto-appends relation.TargetColumn if missing appears verbatim in both loadRelatedManyToManyRows (lines 298–311) and loadRelatedRows (lines 393–406). Extracting this into a small helper function (e.g. ensureTargetColumnSelected(query *SelectQuery, columns []schema.Expression, targetCol *schema.ColumnDef)) would eliminate the duplication and make future changes (e.g. adding a similar guard for SourceColumn on BelongsTo relations) a single-line call in each load function.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/relation_loading.go
Line: 298-314
Comment:
**Duplicated target-column safety check**
The exact same block that checks `config.Columns` and auto-appends `relation.TargetColumn` if missing appears verbatim in both `loadRelatedManyToManyRows` (lines 298–311) and `loadRelatedRows` (lines 393–406). Extracting this into a small helper function (e.g. `ensureTargetColumnSelected(query *SelectQuery, columns []schema.Expression, targetCol *schema.ColumnDef)`) would eliminate the duplication and make future changes (e.g. adding a similar guard for `SourceColumn` on `BelongsTo` relations) a single-line call in each load function.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@jules Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes. Issue 1 of 2pkg/rain/prepared_select.go:90-94
Because the SQL is frozen at Issue 2 of 2pkg/rain/relation_loading.go:298-314 The exact same block that checks |
Thank you for the feedback! I've made the following changes:
|
- Add .First() to SelectQuery for ergonomic single-row fetching with implicit LIMIT 1. - Add BeginTx and RunInTxOpts to DB for sql.TxOptions support. - Add Columns selection to RelationConfig for restricted relational fetching. - Refactor relation loading to use ensureTargetColumnSelected helper. Co-authored-by: cungminh2710 <8063319+cungminh2710@users.noreply.github.com>
1 participant
This PR adds several feature parity improvements inspired by Drizzle ORM:
PR created automatically by Jules for task 2311615599171583350 started by @cungminh2710