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.

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

Implement mapBinding mssql dialect option #5292

Merged
merged 2 commits into from Aug 31, 2022

Conversation

kellyrbourg
Copy link
Contributor

@kellyrbourg kellyrbourg commented Aug 7, 2022

Purpose (Fix: Issue #5268)

Allow users of knex to define custom mssql parameter value/type mapping through configuration. This is useful when having a mix of varchar/nvarchar types within your database. Or if you are simply looking to extend the db type mapping that exists within knex.

For example, having all strings map to varchar.

knex({
    client: 'mssql',
    connection: {
      host,
      user,
      password,
      database,
      options: {
        mapBinding: value => {
          if (typeof value === 'string') {
            return {
              type: TYPES.VarChar,
              value
            }
          }
        }
      }
    }
  })

Changes

  • added mapBinding type signature to MsSqlConnectionConfigBase
  • added mapBinding config option check and call to mssql dialect.
  • reworked _typeForBinding method to return the potentially mutated binding value to the calling code
  • added integration tests for mapBinding

Documentation PR

@coveralls
Copy link

coveralls commented Aug 8, 2022

Coverage Status

Coverage increased (+0.009%) to 92.28% when pulling 0a98d0f on kellyrbourg:feat/mssql-binding-mapping-config into 2dadde4 on knex:master.

Copy link
Collaborator

@OlivierCavadenti OlivierCavadenti left a comment

Choose a reason for hiding this comment

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

Why only MsSQL dialect ?

}

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.

@kellyrbourg
Copy link
Contributor Author

Documentation PR created. knex/documentation#440

@kibertoad kibertoad merged commit e0c0fa9 into knex:master Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants