rails-sqlserver / activerecord-sqlserver-adapter

SQL Server Adapter For Rails
MIT License
972 stars 557 forks source link

Ability to set CONCAT_NULL_YIELDS_NULL #108

Closed barnaclebarnes closed 12 years ago

barnaclebarnes commented 13 years ago

I'm getting this error whenever I try to update a table in my DB (reading from the DB is fine)

TinyTds::Error: UPDATE failed because the following SET options have incorrect settings: 'CONCAT_NULL_YIELDS_NULL'. Verify that SET options are correct for use with indexed views and/or indexes on computed columns and/or filtered indexes and/or query notifications and/or XML data type methods and/or spatial index operations.: UPDATE [rnz_listing] SET [suburb_id] = 3689 WHERE [rnz_listing].[listing_id] = 766367

Any ideas on how I can set the value of CONCAT_NULL_YIELDS_NULL in the driver? I'm at a bit of a lose as to where to set this.

metaskills commented 13 years ago

Can you give me more information? For instance would UPDATE [rnz_listing] SET [suburb_id] = 3689 WHERE [rnz_listing].[listing_id] = 766367 work just fine if you ran it via SMS/QueryAnalyzer or something? If not, then you need to further configure the TinyTDS/DBLIB client when it connects. We do quite a few config settings then too to make DBLIB more ANSI default'ish like ODBC. See here.

https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/lib/active_record/connection_adapters/sqlserver_adapter.rb#L360

So basically, it sounds like you need to just execute some SQL when the connection is made, note this page http://msdn.microsoft.com/en-us/library/ms176056.aspx. If that is the case, ActiveRecord does not seem to provide a native callback, your left to freedom-patch™ this per your needs via a config/initializer of some sort, like this.

class MyApp::SQLServerAdapterExtensions

  def self.included(klass)
    klass.class_eval do
      include InstanceMethods
      alias_method_chain :connect, :my_settings
    end
  end

  module InstanceMethods

    def connect_with_my_settings
      connect_without_my_settings.tap do |client|
        client.execute("SET CONCAT_NULL_YIELDS_NULL ...").do
      end
    end

  end

end

ActiveRecord::Base::ConnectionAdapters::SQLServerAdapter.send :include, MyApp::SQLServerAdapterExtensions

If this is something very specific that you have to toggle before and after each update, then your left with implementing model callbacks. Either way, this seems your schema's problem to solve and not something the adapter should be worrying about. Reopen if you disagree or have further thoughts?

barnaclebarnes commented 13 years ago

Doing the query in the MS tools in Windows works. I have got it to work now by doing the following:

index 2bea1b4..79fbcd9 100644
--- a/lib/active_record/connection_adapters/sqlserver_adapter.rb
+++ b/lib/active_record/connection_adapters/sqlserver_adapter.rb
@@ -387,6 +387,7 @@ module ActiveRecord
                             client.execute("SET ANSI_DEFAULTS ON").do
                             client.execute("SET CURSOR_CLOSE_ON_COMMIT OFF").do
                             client.execute("SET IMPLICIT_TRANSACTIONS OFF").do
+                            client.execute("SET CONCAT_NULL_YIELDS_NULL ON").do
                           end
                         end
                       when :odic
ruby-1.9.2-p180 :005 > l.save
  EXECUTE (27.0ms)  BEGIN TRANSACTION
   (484.9ms)  EXEC sp_executesql N'UPDATE [rnz_listing] SET [suburb_id] = 823 WHERE [rnz_listing].[listing_id] = 1083928; SELECT @@ROWCOUNT AS AffectedRows'
  EXECUTE (30.4ms)  COMMIT TRANSACTION
 => true 

Note that as I was in a test app I upgraded to Rails 3.1 rc5 to get this all setup and working. I'm stretching my Rails/ruby knowledge here but maybe you should be able to pass in extra config parameters (could read these from database.yml). Something like:

:driver_options => {:concat_null_yields_null => true, ... }

This would loop through each option and run client.execute on each of the options (uppercasing, setting ON if true, OFF if false)

 client.execute("SET CONCAT_NULL_YIELDS_NULL ON").do

Thoughts on this approach? This would save freedom patching and provide more flexibility in configuring the client.

metaskills commented 13 years ago

Too specific a use case approach. Your best best is to use the monkey patch I described. Minimally invasive and portalable to your application.

barnaclebarnes commented 13 years ago

BTW: I don't have permission to reopen the ticket ;-)

metaskills commented 13 years ago

You shouldn't need too?

barnaclebarnes commented 13 years ago

" Reopen if you disagree or have further thoughts?" I didn't agree so I wanted to reopen the case but you've made it clear that this is to specific a use case. I'm personally not so sure as I think you should be able to configure the driver without resorting to monkey patching but in any case I have it working now and if anyone else comes up against this issue then hopefully they will see this and comment if it is more widespread than just me.

Thanks for getting back to me so quickly on this. I'm OK with having it closed for now.

metaskills commented 13 years ago

Yea, I could be a bit ignorant about the need to have CONCAT_NULL_YIELDS_NULL as a logical base, much like I have done for ANSI defaults. if you have some research to show me and perhaps even test to make sure all the tests still pass with that config on, then I'll reopen and take a look at adding it. Agreed, more could find this ticket and do the same too.

On the same note, I do think that perhaps ActiveRecord could offer up a way to further configure any connection right after the #connection method is called. So rather than the adapter doing things specific to your need, I'd rather there be a patch to ActiveRecord that does this. That way all adapters get the benefit and the end user can have a solid method to just just redefine and get what they need. I'm just opposed to doing something like this in the current context given the lack of details mentioned above. See what I am saying, not trying to be close minded, just scope the issue and solve it in a broader context and only when there is enough to act on.

All this could just be bike shedding too since it is so easy to make a config/initializer/sqlserver.rb file with the contents I provided and method chain the #connect and configure in your own application. Hope that makes sense. Cheers.

parkerl commented 12 years ago

For historical purposes I thought I'd include this here so it might help others. I was able set CONCAT_NULL_YIELDS_NULL using the following.

config/intializers/sqlserver_config.rb

module MyApp::SQLServerAdapterExtensions

  def self.included(klass)
    klass.class_eval do
      include InstanceMethods
      alias_method_chain :connect, :my_settings
    end
  end

  module InstanceMethods

    def connect_with_my_settings
      connect_without_my_settings.tap do |client|
        client.execute("SET CONCAT_NULL_YIELDS_NULL ON").do
      end
    end

  end

end

ActiveRecord::ConnectionAdapters::SQLServerAdapter.send :include, MyApp::SQLServerAdapterExtensions
metaskills commented 12 years ago

I have a note to myself that ActiveRecord's AbstractAdapter should have a common interface so that you can configure the connection after it is made for stuff just like this. Perfect hack till that is done tho!!!

metaskills commented 12 years ago

I am reopening this issue with two solutions that I am going to do soon.

1) I will be changing the adapter to act like other adapters and create a #configure_connection method that is meant to be overridden by others to supply additional configuration for the connection. So you would be able to do something like this.

def configure_connection
  do_execute "SET CONCAT_NULL_YIELDS_NULL ON"
end

2) I will be also making a #configure_application_name method that can be overridden the same way and used in both TinyTDS/ODBC to set the application name at login. This would allow you to dynamically create these names so we can easily track which application down to which connection in the pool is connecting to our databases.

metaskills commented 12 years ago

Closing this ticket out as I will have updated gems shortly after this message for 2-3-stable, 3-0-stable and master for 3.1

metaskills commented 12 years ago

This is done now and I have a spot in the readme about it too. Will publish 3.1.2 today soon.

parkerl commented 12 years ago

Thanks @metaskills this is great.