feat: use sql params #14
Conversation
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 refactors the SQL query execution mechanism to utilize parameterized queries, moving away from client-side string formatting. This change significantly improves security by mitigating SQL injection risks and potentially boosts performance by allowing the database to optimize query plans. The core 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
|
|
Warning Rate limit exceeded@killagu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
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. WalkthroughAdded optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Operator
participant Transaction
participant Connection
participant DB
Note over App,Operator: Query flow (values forwarded)
App->>Operator: query(sql, values?)
Operator->>Operator: beforeQuery handlers (see sql + values)
alt in transaction
Operator->>Transaction: _query(sql, values?)
Transaction->>Connection: _query(sql, values?)
else not in transaction
Operator->>Connection: _query(sql, values?)
end
Connection->>DB: conn.query(sql, values?)
DB-->>Connection: results
Connection-->>Operator: results
Operator-->>App: results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![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 a significant security improvement by enabling parameterized queries for the query method, which is a great step towards preventing SQL injection vulnerabilities. The changes are logical and the test updates correctly reflect the new behavior.
I've identified a couple of issues: one critical typing issue that would break the build, and one high-severity issue with beforeQuery handlers that could lead to incorrect behavior.
As a suggestion for future work, it would be beneficial to refactor the higher-level helper methods like select, insert, update, and delete to also use parameterized queries. Currently, they still construct SQL strings manually, so they don't benefit from this security enhancement. Unifying the query execution strategy across the library would provide consistent security and is a worthwhile follow-up.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #14 +/- ##
=======================================
Coverage 98.46% 98.46%
=======================================
Files 10 10
Lines 1174 1175 +1
Branches 240 239 -1
=======================================
+ Hits 1156 1157 +1
Misses 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: killa <[email protected]>
![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/connection.ts(1 hunks)src/operator.ts(2 hunks)src/transaction.ts(1 hunks)src/types.ts(1 hunks)test/client.test.ts(5 hunks)
🔇 Additional comments (5)
src/connection.ts (1)
35-36: LGTM! Parameterized query support properly forwarded.The updated
_querymethod correctly accepts optionalvaluesand forwards them to the underlying connection's query method, enabling parameterized queries.src/transaction.ts (1)
39-41: LGTM! Transaction layer correctly forwards parameter values.The change properly extends the transaction's
_querymethod to accept and forward optional values to the underlying connection, maintaining consistency with the parameterized query support added throughout the codebase.test/client.test.ts (1)
360-360: Note the logging behavior change.The test updates reflect that SQL is now logged with placeholders (
??,?) instead of interpolated values. This is a breaking change in observability behavior:Benefits:
- Prevents sensitive data from appearing in logs
- Consistent with parameterized query approach
Trade-offs:
- Debugging becomes harder without seeing actual values in logs
- Monitoring tools expecting formatted SQL may need updates
Consider documenting this behavior change in release notes or migration guides.
Also applies to: 1481-1481, 1494-1495, 1505-1506, 1517-1518
src/operator.ts (2)
98-98: LGTM! Values correctly forwarded for parameterized queries.The change properly passes
valuesto the underlying_queryimplementation, enabling the database driver to handle parameterized queries. This improves security by preventing SQL injection and may improve performance through prepared statements.Note: Error stack traces (line 107) will now contain placeholders instead of actual values, which may impact debugging. Consider whether additional context should be logged separately for troubleshooting purposes.
132-132: LGTM! Protected method signature properly extended.The
_querysignature correctly accepts optionalvalues, maintaining the abstract method contract that subclasses must implement. The parameter is appropriately optional for backward compatibility.
src/types.ts
Outdated
|
|
||
| export interface PoolConnectionPromisify extends Omit<PoolConnection, 'query'> { | ||
| query(sql: string): Promise<any>; | ||
| query(sql: string, values: any | any[] | { [param: string]: any }): Promise<any>; |
There was a problem hiding this comment.
Fix type inconsistency: values should be optional.
The values parameter is defined as required in the type definition, but it's optional in all implementations (src/connection.ts, src/transaction.ts, src/operator.ts). This breaks backward compatibility and causes a type mismatch.
Apply this diff to make the parameter optional:
- query(sql: string, values: any | any[] | { [param: string]: any }): Promise<any>;
+ query(sql: string, values?: any | any[] | { [param: string]: any }): Promise<any>;Also consider simplifying the type to values?: any since any already includes arrays and objects, making the union redundant.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| query(sql: string, values: any | any[] | { [param: string]: any }): Promise<any>; | |
| query(sql: string, values?: any | any[] | { [param: string]: any }): Promise<any>; |
🤖 Prompt for AI Agents
In src/types.ts around line 16, the query signature currently requires the
values parameter which is inconsistent with implementations; change the
signature to make values optional (e.g., values?: any) to restore compatibility
and simplify the type (use any instead of the union of array/object/any). Ensure
the exported type/signature is updated so callers and implementations that omit
values no longer error on typing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/channels.ts (1)
25-29: Consider consistent field ordering.The field order differs between
QueryStartMessage(connection, values, sql) andQueryEndMessage(connection, sql, values, ...). For consistency and readability, consider placingsqlandvaluesin the same order in both interfaces.Apply this diff to align the field order:
export interface QueryStartMessage { connection: PoolConnectionPromisify; - values?: object | any[]; sql: string; + values?: object | any[]; }Also applies to: 31-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/channels.ts(1 hunks)src/operator.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/operator.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/channels.ts (1)
src/types.ts (1)
PoolConnectionPromisify(15-20)
|
|
||
| export interface QueryStartMessage { | ||
| connection: PoolConnectionPromisify; | ||
| values?: object | any[]; |
There was a problem hiding this comment.
🧩 Analysis chain
Type mismatch with PoolConnectionPromisify.query interface.
The values type here is object | any[], but in types.ts the query method accepts any | any[] | { [param: string]: any }, which includes primitives. This inconsistency could cause type errors when forwarding query parameters to these message interfaces.
Consider aligning the types. Apply this diff to match the types.ts interface:
export interface QueryStartMessage {
connection: PoolConnectionPromisify;
- values?: object | any[];
+ values?: any | any[] | { [param: string]: any };
sql: string;
}
export interface QueryEndMessage {
connection: PoolConnectionPromisify;
sql: string;
- values?: object | any[];
+ values?: any | any[] | { [param: string]: any };
duration: number;
error?: Error;
}Alternatively, verify whether MySQL actually accepts primitive values for query parameters, and if not, update types.ts to use the stricter type:
Also applies to: 34-34
🏁 Script executed:
#!/bin/bash
# Search for how values are used with the MySQL connection
rg -n -C3 -t ts 'conn\.query\(' src/Length of output: 567
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for types.ts and look for query method/PoolConnectionPromisify definition
rg -n 'PoolConnectionPromisify|query.*values' src/ --type ts -A2 -B2 | head -100Length of output: 4458
🏁 Script executed:
#!/bin/bash
# Find and examine types.ts
fd 'types\.ts$' src/ -exec cat {} \;Length of output: 146
🏁 Script executed:
#!/bin/bash
# Search for where PoolConnectionPromisify.query is actually defined
rg -n 'query\s*\(' src/ --type ts -B5 | grep -A5 -B5 'PoolConnectionPromisify\|interface.*Connection'Length of output: 440
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# View channels.ts to see the current state
cat src/channels.tsLength of output: 1130
🏁 Script executed:
#!/bin/bash
# Check if values is ever passed as a primitive (non-object/non-array) type in the codebase
rg -n 'values\s*=\s*[^[\{\s]|values\s*:\s*(?!object|any\[|\{)' src/ --type ts | head -20Length of output: 129
Type mismatch confirmed between PoolConnectionPromisify.query interface and usage.
The type inconsistency is real and documented:
- types.ts declares:
query(sql: string, values?: any | any[] | { [param: string]: any }): Promise<any>; - channels.ts (lines 27, 34):
values?: object | any[]; - All implementations (operator.ts, connection.ts, transaction.ts):
values?: object | any[]
The broader type in types.ts (which includes primitives via any) conflicts with the narrower type in channels.ts and all implementations (object | any[]). This inconsistency could cause type errors if a caller passes a primitive value to the query method.
However, note that all implementations consistently use the narrower type, not the broader one declared in types.ts. Before applying the suggested diff, verify which direction to align: either make channels.ts match types.ts (accepting the broader type), or update types.ts to match all implementations and channel messages (using the narrower type).
🤖 Prompt for AI Agents
In src/channels.ts around lines 27 and 34, there's a mismatch between the
narrower values type used by channels and implementations (values?: object |
any[]) and the broader signature in types.ts that allows primitives; update the
signature in types.ts to match the implementations by changing the query
parameter type to values?: object | any[] (or equivalent indexable object type
if preferred), then run TypeScript type-checks and adjust any remaining
declarations so all interfaces and implementations use the same narrower type.
[skip ci] ## 1.4.0 (2025-11-25) * feat: use sql params (#14) ([0d1569d](0d1569d)), closes [#14](#14) [hi#level](https://github.com/hi/issues/level) * fix: add catch for get conn exception (#13) ([314b425](314b425)), closes [#13](#13) * chore: enable auto merge ([a1a801d](a1a801d)) * chore: fix auto release ([8c3e1d4](8c3e1d4))
|
🎉 This PR is included in version 1.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.