feat: implement mysql execute #15
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a unified execute(sql, values?, options?) API across client, connection, transaction, operator and types; centralizes connection acquisition/release and hook handling; extends promisified pool wrapper to include execute; and adds unit tests for execute (appearing duplicated). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as RDSClient
participant Operator as Operator
participant Conn as RDSConnection / PoolConn
participant DB as Database
participant Tx as RDSTransaction
Client->>Client: resolve connection (options.conn | AsyncLocalStorage | acquire)
alt in-transaction
Client->>Tx: call _execute(sql, values)
Tx->>Tx: `#check`()
Tx->>Conn: conn._execute(sql, values)
Conn->>DB: execute SQL
DB-->>Conn: result
Conn-->>Tx: result
Tx-->>Client: result
else not in-transaction
Client->>Operator: Operator.execute(sql, values)
Operator->>Operator: run pre-hooks / log start
Operator->>Conn: conn._execute/sql execute executor
Conn->>DB: execute SQL
DB-->>Conn: result
Conn-->>Operator: result
Operator->>Operator: run post-hooks / log end
Operator-->>Client: result
Client->>Client: if acquired -> release connection
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello @killagu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant feature by implementing a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an execute method for MySQL prepared statements across the client, connection, and transaction classes, along with corresponding type definitions and tests. The implementation is functional, but there are a couple of architectural concerns. The new execute method in RDSClient duplicates connection management logic from the existing query method. More critically, the execute methods on RDSConnection and RDSTransaction bypass the existing lifecycle hooks (beforeQuery/afterQuery) provided by the Operator base class, leading to inconsistent behavior for logging and monitoring compared to query calls. These issues should be addressed to improve maintainability and ensure consistent behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client.ts(1 hunks)src/connection.ts(2 hunks)src/transaction.ts(1 hunks)src/types.ts(1 hunks)test/client.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/client.ts (2)
src/transaction.ts (2)
conn(49-53)RDSTransaction(5-54)src/connection.ts (1)
RDSConnection(8-55)
test/client.test.ts (1)
src/transaction.ts (1)
conn(49-53)
🪛 GitHub Actions: CI
test/client.test.ts
[error] 1623-1623: Trailing spaces not allowed. ESLint: no-trailing-spaces. Command: eslint src test --cache.
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 16, mysql@5)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 16, mysql@8)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 16, mysql@9)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18, mysql@5)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18, mysql@8)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18, mysql@9)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20, mysql@5)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20, mysql@8)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20, mysql@9)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22, mysql@5)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22, mysql@8)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22, mysql@9)
test/client.test.ts
[failure] 1661-1661:
Trailing spaces not allowed
[failure] 1660-1660:
Trailing spaces not allowed
[failure] 1659-1659:
Unexpected parentheses around single function argument
[failure] 1653-1653:
Trailing spaces not allowed
[failure] 1644-1644:
Trailing spaces not allowed
[failure] 1643-1643:
Trailing spaces not allowed
[failure] 1633-1633:
Trailing spaces not allowed
[failure] 1632-1632:
Trailing spaces not allowed
[failure] 1630-1630:
Trailing spaces not allowed
[failure] 1623-1623:
Trailing spaces not allowed
🔇 Additional comments (4)
src/transaction.ts (1)
44-47: RDSTransaction.execute matches existing transaction safety pattern
execute()correctly reuses#check()and delegates to the underlying connection, consistent with_query,commit, androllback. Looks good.src/types.ts (1)
15-18: execute added to PoolConnectionPromisify in line with queryIncluding
'execute'in theOmitand defining a Promise-returningexecutemethod keeps the promisified connection type consistent with howqueryis handled. This aligns with the new usage inRDSConnectionandRDSTransaction.Please re-run TypeScript build/tests against the current
mysql2version to confirm thatPoolConnectionstill exposes anexecutemethod with a compatible callback shape.src/connection.ts (1)
19-23: RDSConnection.execute is correctly wired into the promisified wrapperAdding
'execute'to the method list forpromisifyand exposingasync execute(sql, values)mirrors the existingquerybehavior and gives higher layers a consistent Promise API.Please confirm via the existing test matrix (and/or mysql2 docs) that
PoolConnection.executehas the expected callback signature soutil.promisifycontinues to resolve to the result set as intended.Also applies to: 40-42
src/client.ts (1)
140-163: RDSClient.execute cleanly mirrors query’s connection lifecycle
execute()reuses the same connection resolution andshouldReleaseConnpattern asquery(), so it behaves correctly in normal, explicit-connection, and transaction-scope scenarios. Implementation looks solid.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #15 +/- ##
==========================================
- Coverage 98.22% 98.13% -0.09%
==========================================
Files 10 10
Lines 1180 1231 +51
Branches 239 248 +9
==========================================
+ Hits 1159 1208 +49
- Misses 21 23 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Move execute method implementation from RDSConnection to Operator base class - Add _execute abstract method in Operator for consistency with _query - Extract common execution logic into private #executeWithHooks method - Reduce code duplication between query and execute methods - Maintain all existing functionality (hooks, logging, monitoring, events) - Simplify RDSConnection by removing complex execute implementation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/operator.ts (1)
76-93: Improve type safety forqueryOneand reduce boilerplate with a default_executefallbackThe new API looks good. Two refinements would strengthen the implementation:
- Add generics and correct null-coalescing in
queryOneCurrently
queryOnereturnsPromise<any>and usesrows && rows[0] || null, which incorrectly treats falsy row values (e.g.,0,false) as "no row found". Make it generic and use the nullish coalescing operator:- async queryOne(sql: string, values?: object | any[]) { - const rows = await this.query(sql, values); - return rows && rows[0] || null; - } + async queryOne<T = any>(sql: string, values?: object | any[]): Promise<T | null> { + const rows = await this.query<T[]>(sql, values); + return (rows && rows[0]) ?? null; + }
- Provide a default
_executeimplementation that delegates to_queryMany subclasses may only need to implement
_queryand don't have a distinctexecuteoperation. Offering a sensible default reduces duplication:- // eslint-disable-next-line @typescript-eslint/no-unused-vars - protected async _execute(_sql: string, _values?: object | any[]): Promise<any> { - throw new Error('SubClass must impl this'); - } + protected async _execute(sql: string, values?: object | any[]): Promise<any> { + return this._query(sql, values); + }This keeps
_queryas the required implementation while giving subclasses a workingexecuteout of the box—they can still override_executewhen the underlying driver provides a distinct operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/connection.ts(2 hunks)src/operator.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/operator.ts (1)
src/channels.ts (1)
QueryStartMessage(25-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Node.js / Test (ubuntu-latest, 22, mysql@9)
- GitHub Check: Node.js / Test (ubuntu-latest, 18, mysql@8)
- GitHub Check: Node.js / Test (ubuntu-latest, 18, mysql@9)
🔇 Additional comments (2)
src/operator.ts (1)
90-100: Unified#executeWithHookslogic correctly applies lifecycle hooks and logging toexecuteThe new
executeAPI and#executeWithHookshelper look consistent and well‑factored:
- Both
queryandexecutenow go through the same lifecycle:beforeQueryHandlers, debug logging, optionalloggingcallback,channels.queryStart/queryEnd, error decoration, andafterQueryHandlers.- Passing an explicit
operationstring into the debug messages cleanly distinguishesqueryvsexecutein logs without changing the channel contracts.This addresses the earlier concern that
executemight bypass query lifecycle hooks and centralizes cross‑cutting behavior in one place. No changes needed here from my side.Also applies to: 94-155, 110-111, 126-132, 138-148
src/connection.ts (1)
17-25: Theexecute()method is guaranteed by the type contract and does not require guarding.The
PoolConnectionPromisifyinterface explicitly declaresexecuteas a required method (lines 16-17, src/types.ts). The client'sgetConnection()method returnsPromise<PoolConnectionPromisify>(line 22, src/types.ts), soRDSConnectionalways receives connections that satisfy this contract. Sincemysql2.PoolConnectionalways includes theexecute()method as part of the standard MySQL protocol, and the type system enforces this at compile time, adding runtime guards is unnecessary.All five methods in the promisify list—
query,execute,beginTransaction,commit,rollback—are treated identically and no guards exist for any of them elsewhere in the codebase. Ifexecutewere optional, the others would need guards too, but they don't. The concern about "different drivers or older versions" does not apply because this library specifically usesmysql2.PoolConnection.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/transaction.ts (1)
44-47: Addprotectedmodifier for consistency with_query.The
_querymethod on line 39 is declared asprotected, but_executelacks this modifier. Both methods serve the same purpose as internal implementations called by the publicquery()andexecute()methods in theOperatorbase class.- async _execute(sql: string, values?: object | any[]) { + protected async _execute(sql: string, values?: object | any[]) { this.#check(); return await this.conn!._execute(sql, values); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client.ts(2 hunks)src/transaction.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client.ts (3)
src/operator.ts (1)
T(94-155)src/transaction.ts (2)
conn(49-53)RDSTransaction(5-54)src/connection.ts (1)
RDSConnection(8-55)
🔇 Additional comments (2)
src/client.ts (2)
78-85: Good addition ofexecuteto promisified methods.Adding
executeto the promisification list properly enables prepared statement support from the mysql2 pool.
116-154: Well-structured refactor consolidating connection handling.The extraction of
#executeWithConnectionand#getConnectionhelpers addresses the previous review concern about duplication betweenqueryandexecute. The implementation correctly:
- Resolves connections from three sources with proper priority (options → AsyncLocalStorage → new acquisition)
- Tracks connection ownership with
shouldReleaseflag- Ensures connection release in
finallyblock only when appropriate
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.