Skip to content

feat: use sql params #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
killagu merged 4 commits into from
Nov 25, 2025
Merged

feat: use sql params #14

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8fa81bf
feat: use sql params
killagu Nov 25, 2025
df0ba71
Update src/types.ts
killagu Nov 25, 2025
45ce6ac
f
killagu Nov 25, 2025
e820e8a
f
killagu Nov 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ export class RDSConnection extends Operator {
return this.conn.release();
}

async _query(sql: string) {
return await this.conn.query(sql);
async _query(sql: string, values?: object | any[]) {
return await this.conn.query(sql, values);
}

async beginTransaction() {
Expand Down
7 changes: 2 additions & 5 deletions src/operator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,6 @@ export abstract class Operator {

async query<T = any>(sql: string, values?: object | any[]): Promise<T> {
// query(sql, values)
if (values) {
sql = this.format(sql, values);
}
if (this.beforeQueryHandlers.length > 0) {
for (const beforeQueryHandler of this.beforeQueryHandlers) {
const newSql = beforeQueryHandler(sql);
Expand All @@ -98,7 +95,7 @@ export abstract class Operator {
connection: this.#connection,
} as QueryStartMessage);
try {
rows = await this._query(sql);
rows = await this._query(sql, values);
if (Array.isArray(rows)) {
debug('[connection#%s] query get %o rows', this.threadId, rows.length);
} else {
Expand Down Expand Up @@ -132,7 +129,7 @@ export abstract class Operator {
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
protected async _query(_sql: string): Promise<any> {
protected async _query(_sql: string, _values?: object | any[]): Promise<any> {
throw new Error('SubClass must impl this');
}

Expand Down
4 changes: 2 additions & 2 deletions src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ export class RDSTransaction extends Operator {
}
}

protected async _query(sql: string) {
protected async _query(sql: string, values?: object | any[]) {
this.#check();
return await this.conn!._query(sql);
return await this.conn!._query(sql, values);
}

#check() {
Expand Down
2 changes: 1 addition & 1 deletion src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export interface RDSClientOptions extends PoolOptions {
}

export interface PoolConnectionPromisify extends Omit<PoolConnection, 'query'> {
query(sql: string): Promise<any>;
query(sql: string, values: any | any[] | { [param: string]: any }): Promise<any>;
Comment thread
killagu marked this conversation as resolved.
Outdated

@coderabbitai coderabbitai Bot Nov 25, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

beginTransaction(): Promise<void>;
commit(): Promise<void>;
rollback(): Promise<void>;
Expand Down
16 changes: 8 additions & 8 deletions test/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ describe('test/client.test.ts', () => {
[ table, prefix + 'm@fengmk2.com' ]);
assert.deepEqual(mockLogs, [
'show tables',
`select * from \`${table}\` where email = '${prefix + 'm@fengmk2.com'}' limit 1`,
'select * from ?? where email = ? limit 1',
]);
});
});
Expand Down Expand Up @@ -1478,7 +1478,7 @@ describe('test/client.test.ts', () => {
counter2After++;
});
await db.query('select * from ?? limit 10', [ table ]);
assert.equal(lastSql, 'select * from `myrds-test-user` limit 10');
assert.equal(lastSql, 'select * from ?? limit 10');
assert.equal(lastArgs[0], lastSql);
assert.equal(Array.isArray(lastArgs[1]), true);
assert.equal(count, 1);
Expand All @@ -1491,8 +1491,8 @@ describe('test/client.test.ts', () => {
values(?, ?, now(), now())`,
[ table, prefix + 'beginTransactionScope1', prefix + 'm@beginTransactionScope1.com' ]);
});
assert.equal(lastSql, 'insert into `myrds-test-user`(name, email, gmt_create, gmt_modified)\n' +
` values('${prefix}beginTransactionScope1', '${prefix}m@beginTransactionScope1.com', now(), now())`);
assert.equal(lastSql, 'insert into ??(name, email, gmt_create, gmt_modified)\n' +
' values(?, ?, now(), now())');
assert.equal(lastArgs[0], lastSql);
assert.equal(lastArgs[1].affectedRows, 1);
assert.equal(count, 2);
Expand All @@ -1502,8 +1502,8 @@ describe('test/client.test.ts', () => {
values(?, ?, now(), now())`,
[ table, prefix + 'beginDoomedTransactionScope1', prefix + 'm@beginDoomedTransactionScope1.com' ]);
});
assert.equal(lastSql, 'insert into `myrds-test-user`(name, email, gmt_create, gmt_modified)\n' +
` values('${prefix}beginDoomedTransactionScope1', '${prefix}m@beginDoomedTransactionScope1.com', now(), now())`);
assert.equal(lastSql, 'insert into ??(name, email, gmt_create, gmt_modified)\n' +
' values(?, ?, now(), now())');
assert.equal(lastArgs[0], lastSql);
assert.equal(lastArgs[1].affectedRows, 1);
assert.equal(count, 3);
Expand All @@ -1514,8 +1514,8 @@ describe('test/client.test.ts', () => {
values(?, ?, now(), now())`,
[ table, prefix + 'transaction1', prefix + 'm@transaction1.com' ]);
await conn.commit();
assert.equal(lastSql, 'insert into `myrds-test-user`(name, email, gmt_create, gmt_modified)\n' +
` values('${prefix}transaction1', '${prefix}m@transaction1.com', now(), now())`);
assert.equal(lastSql, 'insert into ??(name, email, gmt_create, gmt_modified)\n' +
' values(?, ?, now(), now())');
assert.equal(lastArgs[0], lastSql);
assert.equal(lastArgs[1].affectedRows, 1);
assert.equal(count, 4);
Expand Down