Skip to content
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

Jump to bottom

Implement mapBinding mssql dialect option #5292

Merged
merged 2 commits into from Aug 31, 2022
Merged

Implement mapBinding mssql dialect option #5292

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
205851e
add mapBinding to mssql dialect; including tests & types
kellyrbourg Aug 6, 2022
0a98d0f
remove commented out code
kellyrbourg Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 20 additions & 10 deletions lib/dialects/mssql/index.js
View file
Open in desktop
Expand Up @@ -333,14 +333,24 @@ class Client_MSSQL extends Client {
_typeForBinding(binding) {
const Driver = this._driver();

if (
this.connectionSettings.options &&
this.connectionSettings.options.mapBinding
) {
const result = this.connectionSettings.options.mapBinding(binding);
if (result) {
return [result.value, result.type];
}
}

switch (typeof binding) {
case 'string':
return Driver.TYPES.NVarChar;
return [binding, Driver.TYPES.NVarChar];
case 'boolean':
return Driver.TYPES.Bit;
return [binding, Driver.TYPES.Bit];
case 'number': {
if (binding % 1 !== 0) {
return Driver.TYPES.Float;
return [binding, Driver.TYPES.Float];
}

if (binding < SQL_INT4.MIN || binding > SQL_INT4.MAX) {
Expand All @@ -350,25 +360,25 @@ class Client_MSSQL extends Client {
);
}

return Driver.TYPES.BigInt;
return [binding, Driver.TYPES.BigInt];
}

return Driver.TYPES.Int;
return [binding, Driver.TYPES.Int];
}
default: {
// if (binding === null || typeof binding === 'undefined') {
// return tedious.TYPES.Null;
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove this comment (not in your PR, but can be good to clean up)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why only MsSQL dialect ?

Each dialect within knex has it's own sauce for how to go from nodejs types to database specific types. Some even defer to the external library for handling the mapping itself (AFAIK). Executing this PR for all dialects would require each knex dialect/client to own this mapping itself. Would need someone that knows how to do that mapping for the other dialects to help take that on. I'm not sure this is as big of an issue with other dialects either.

can we remove this comment (not in your PR, but can be good to clean up)

Will do.


if (binding instanceof Date) {
return Driver.TYPES.DateTime;
return [binding, Driver.TYPES.DateTime];
}

if (binding instanceof Buffer) {
return Driver.TYPES.VarBinary;
return [binding, Driver.TYPES.VarBinary];
}

return Driver.TYPES.NVarChar;
return [binding, Driver.TYPES.NVarChar];
}
}
}
Expand Down Expand Up @@ -401,8 +411,8 @@ class Client_MSSQL extends Client {
}

// sets a request input parameter. Detects bigints and decimals and sets type appropriately.
_setReqInput(req, i, binding) {
const tediousType = this._typeForBinding(binding);
_setReqInput(req, i, inputBinding) {
const [binding, tediousType] = this._typeForBinding(inputBinding);
const bindingName = 'p'.concat(i);
let options;

Expand Down
28 changes: 28 additions & 0 deletions test/integration2/dialects/mssql.spec.js
View file
Open in desktop
@@ -1,4 +1,5 @@
const { expect } = require('chai');
const { TYPES } = require('tedious');
const { getAllDbs, getKnexForDb } = require('../util/knex-instance-provider');

async function fetchDefaultConstraint(knex, table, column) {
Expand Down Expand Up @@ -31,6 +32,8 @@ describe('MSSQL dialect', () => {
beforeEach(async () => {
await knex.schema.createTable('test', function () {
this.increments('id').primary();
this.specificType('varchar', 'varchar(100)');
this.string('nvarchar');
});
});

Expand Down Expand Up @@ -362,6 +365,31 @@ describe('MSSQL dialect', () => {
return result ? result.comment : undefined;
}
});

describe('supports mapBinding config', async () => {
it('can remap types', async () => {
const query = knex('test')
.where('varchar', { value: 'testing', type: TYPES.VarChar })
.select('id');
const { bindings } = query.toSQL().toNative();
expect(bindings[0].type, TYPES.VarChar);
expect(bindings[0].value, 'testing');

// verify the query runs successfully
await query;
});
it('undefined mapBinding result falls back to default implementation', async () => {
const query = knex('test')
.where('nvarchar', 'testing')
.select('id');

const { bindings } = query.toSQL().toNative();
expect(bindings[0], 'testing');

// verify the query runs successfully
await query;
});
});
});
});
});
Expand Down
7 changes: 7 additions & 0 deletions test/integration2/util/knex-instance-provider.js
View file
Open in desktop
Expand Up @@ -173,6 +173,13 @@ const testConfigs = {
server: 'localhost',
port: 21433,
database: 'knex_test',
options: {
mapBinding(value) {
if (value && value.type) {
return { value: value.value, type: value.type };
}
},
},
},
pool: pool,
migrations,
Expand Down
1 change: 1 addition & 0 deletions types/index.d.ts
View file
Open in desktop
Expand Up @@ -2887,6 +2887,7 @@ export declare namespace Knex {
multiSubnetFailover?: boolean;
packetSize?: number;
trustServerCertificate?: boolean;
mapBinding?: (value: any) => ({ value: any, type: any } | undefined);
}>;
pool?: Readonly<{
min?: number;
Expand Down