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

Set ansi_null_dflt_on on by default #230

Merged
merged 1 commit into from
Mar 15, 2015
Merged

Set ansi_null_dflt_on on by default #230

merged 1 commit into from
Mar 15, 2015

Conversation

lee-houghton
Copy link
Contributor

I recently switched my application to use tedious for connecting to SQL Server 2012, which mostly went smoothly. I did however notice that some stored procedures produced errors when ran through a tedious connection, when they worked when run via ODBC or SQL Server Management Studio.

By playing with the Query Options in SSMS I was able to find that I could reproduce the problem by unticking SET ANSI_NULL_DFLT_ON. According to MSDN the OLE DB and ODBC drivers set this flag by default. Perhaps tedious should set it too?

At the same time, perhaps a way of customizing what getInitialSql returns would be useful, but I thought that wasn't really for me to decide. For the moment, I have duck-punched it like this:

oldInitialSql = Connection::getInitialSql
Connection::getInitialSql = ->
    sql = oldInitialSql.call(this)
    "#{sql}
    set ansi_null_dflt_on on"

@bretcope
Copy link
Member

I like the consistency with SSMS and the native driver. I would note that this is a minor breaking change though. At the very least, there needs to be a config option to restore previous behavior.

I actually think that you're right that a way to customize the initial sql is the best long-term approach.

@arthurschreiber
Copy link
Collaborator

@lee-houghton I also like this change very much. Would you be open to work on making the initial sql customizable?

@lee-houghton
Copy link
Contributor Author

@arthurschreiber - definitely. I think it would be best to do it in such a way that you don't have to re-specify all the default options, then if the defaults change the user will still receive new values for anything they haven't already customized.

I'd probably lean towards the simplest option of allowing appending to the initial SQL via config.options.extraInitialSql (or some better-named option), and if you want to change an option that's already set, you set it again. The only drawback I can see is a few wasted bytes, is that likely to be an issue?

@bretcope
Copy link
Member

bretcope commented Feb 9, 2015

There are two separate issues here which are getting conflated, and that's a bad thing. This PR should address the original issue by changing the initial sql, and adding a config option which allows you to switch it back to the old behavior. That's how we've done changes to the initial sql in the past, and is the most respectful of our users.

The second issue, which should probably be a separate PR, is how do we want to enable people to make arbitrary changes to the initial sql (stuff we won't support via the config). I think the simplest way to do that is with an optional function, but with a slightly different interface than the one you're proposing.

Here's what I would suggest:

config.options.getInitialSql = function (defaultInitialSql) {
  return defaultInitialSql + "SET SOME OPTION;\n";
};

That would provide the default initial sql as a baseline for you to work with, but allows you to do whatever you want, including replace it entirely.

To revert to the previous default, set config.options.enableAnsiNullDefault
to false.

This emulates the defaults for ODBC and OLE SQL. Also add a test that the
default settings allow adding a null value to a temporary table with
unspecified nullability.
@lee-houghton
Copy link
Contributor Author

That's actually how I initially considered doing it. Makes sense.

I've changed my ansi_null_dflt_on branch to do just the first part: change the default, and add a "enableAnsiDefaultNull" flag which can be set to false to use the old default behaviour.

@bretcope
Copy link
Member

bretcope commented Mar 3, 2015

I'm in favor of merging this. Documentation also needs to be updated. If no one else gets to it in the next few days, I'll take care of it.

bretcope added a commit that referenced this pull request Mar 15, 2015
Set ansi_null_dflt_on on by default
@bretcope bretcope merged commit 9851e01 into tediousjs:master Mar 15, 2015
@bretcope
Copy link
Member

Published as 1.11.0 and documentation is updated.

@lee-houghton lee-houghton deleted the ansi_null_dflt_on branch March 18, 2015 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants