Code cleanup for #426 and #427 linked server support.

* Rename `remote_server?` to `database_prefix_remote_server?`
* Make `database_prefix_remote_server?` stronger by checking if #object is left blank.

Misc refactors.

* Remove `@config` init ivar. Not used.
* Use `connection_options` helper in tests for terse win.

cc @jippeholwerda

Author: metaskills

CHANGELOG.md

@@ -3,7 +3,7 @@
 
 #### Fixed
 
-* Allow linked servers for table names. Fixes #426. Thanks @jippeholwerda
+* Allow linked servers for table names. Fixes #426. Fixes #427. Thanks @jippeholwerda
 
 
 ## v4.2.5

lib/active_record/connection_adapters/sqlserver/database_statements.rb

@@ -216,7 +216,7 @@ def select(sql, name = nil, binds = [])
         end
 
         def sql_for_insert(sql, pk, id_value, sequence_name, binds)
-          sql = if pk && self.class.use_output_inserted && !remote_server?
+          sql = if pk && self.class.use_output_inserted && !database_prefix_remote_server?
             quoted_pk = SQLServer::Utils.extract_identifiers(pk).quoted
             sql.insert sql.index(/ (DEFAULT )?VALUES/), " OUTPUT INSERTED.#{quoted_pk}"
           else

lib/active_record/connection_adapters/sqlserver/schema_statements.rb

@@ -220,10 +220,10 @@ def initialize_native_database_types
         end
 
         def column_definitions(table_name)
-          if remote_server?
-            identifier  = SQLServer::Utils.extract_identifiers("#{database_prefix}#{table_name}")
+          identifier = if database_prefix_remote_server?
+            SQLServer::Utils.extract_identifiers("#{database_prefix}#{table_name}")
           else
-            identifier  = SQLServer::Utils.extract_identifiers(table_name)
+            SQLServer::Utils.extract_identifiers(table_name)
           end
           database    = identifier.fully_qualified_database_quoted
           view_exists = schema_cache.view_exists?(table_name)

lib/active_record/connection_adapters/sqlserver_adapter.rb

@@ -54,7 +54,6 @@ def initialize(connection, logger, pool, config)
         @visitor = Arel::Visitors::SQLServer.new self
         @prepared_statements = true
         # Our Responsibility
-        @config = config
         @connection_options = config
         connect
         @sqlserver_azure = !!(select_value('SELECT @@version', 'SCHEMA') =~ /Azure/i)
@@ -181,8 +180,10 @@ def sqlserver_azure?
         @sqlserver_azure
       end
 
-      def remote_server?
-        !!database_prefix and SQLServer::Utils.extract_identifiers(@connection_options[:database_prefix]).fully_qualified?
+      def database_prefix_remote_server?
+        return false if database_prefix.blank?
+        name = SQLServer::Utils.extract_identifiers(database_prefix)
+        name.fully_qualified? && name.object.blank?
       end
 
       def database_prefix

lib/arel/visitors/sqlserver.rb

@@ -74,7 +74,7 @@ def visit_Arel_Nodes_SelectStatement o, collector
       end
 
       def visit_Arel_Table o, collector
-        table_name = if o.engine.connection.remote_server?
+        table_name = if o.engine.connection.database_prefix_remote_server?
           remote_server_table_name(o)
         else
           quote_table_name(o.name)

test/cases/adapter_test_sqlserver.rb

@@ -416,24 +416,28 @@ class AdapterTestSQLServer < ActiveRecord::TestCase
 
   end
 
-  describe 'remote_server?' do
+  describe 'database_prefix_remote_server?' do
+
+    let(:connection_options) { connection.instance_variable_get(:@connection_options) }
+
     after do
-      connection.instance_variable_get(:@connection_options).delete(:database_prefix)
+      connection_options.delete(:database_prefix)
     end
 
-    it 'should return false if database_prefix is not configured' do
-      assert_equal false, connection.remote_server?
+    it 'returns false if database_prefix is not configured' do
+      assert_equal false, connection.database_prefix_remote_server?
     end
 
-    it 'should return true if database_prefix has been set' do
-      connection.instance_variable_get(:@connection_options)[:database_prefix] = "server.database.schema."
-      assert_equal true, connection.remote_server?
+    it 'returns true if database_prefix has been set' do
+      connection_options[:database_prefix] = "server.database.schema."
+      assert_equal true, connection.database_prefix_remote_server?
     end
 
-    it 'should return false if database_prefix has been set incorrectly' do
-      connection.instance_variable_get(:@connection_options)[:database_prefix] = "server.database.schema"
-      assert_equal false, connection.remote_server?
+    it 'returns false if database_prefix has been set incorrectly' do
+      connection_options[:database_prefix] = "server.database.schema"
+      assert_equal false, connection.database_prefix_remote_server?
     end
+
   end
 
 end

test/cases/fully_qualified_identifier_test_sqlserver.rb

@@ -1,21 +1,27 @@
 require 'cases/helper_sqlserver'
 
 class FullyQualifiedIdentifierTestSQLServer < ActiveRecord::TestCase
+
+  let(:connection_options) { connection.instance_variable_get(:@connection_options) }
+
   describe 'local server' do
+
     it 'should use table name in select projections' do
       table = Arel::Table.new(:table)
       expected_sql = "SELECT [table].[name] FROM [table]"
       assert_equal expected_sql, table.project(table[:name]).to_sql
     end
+
   end
 
   describe 'remote server' do
+
     before do
-      connection.instance_variable_get(:@connection_options)[:database_prefix] = "[my.server].db.schema."
+      connection_options[:database_prefix] = "[my.server].db.schema."
     end
 
     after do
-      connection.instance_variable_get(:@connection_options).delete(:database_prefix)
+      connection_options.delete :database_prefix
     end
 
     it 'should use fully qualified table name in select from clause' do
@@ -66,5 +72,7 @@ class FullyQualifiedIdentifierTestSQLServer < ActiveRecord::TestCase
       expected_sql = "DELETE FROM [my.server].[db].[schema].[table] WHERE [table].[id] = 42"
       assert_equal expected_sql, manager.to_sql
     end
+
   end
+
 end

test/cases/utils_test_sqlserver.rb

@@ -42,7 +42,7 @@ class UtilsTestSQLServer < ActiveRecord::TestCase
 
     it 'extracts and returns #object identifier unquoted by default or quoted as needed' do
       valid_names.each do |n|
-        name = SQLServer::Utils.extract_identifiers(n)
+        name = extract_identifiers(n)
         name.object.must_equal 'object', "With #{n.inspect} for #object"
         name.object_quoted.must_equal '[object]', "With #{n.inspect} for #object_quoted"
       end
@@ -53,12 +53,12 @@ class UtilsTestSQLServer < ActiveRecord::TestCase
       it "extracts and returns #{part} identifier unquoted by default or quoted as needed" do
         present, blank = send(:"#{part}_names")
         present.each do |n|
-          name = SQLServer::Utils.extract_identifiers(n)
+          name = extract_identifiers(n)
           name.send(:"#{part}").must_equal "#{part}", "With #{n.inspect} for ##{part} method"
           name.send(:"#{part}_quoted").must_equal "[#{part}]", "With #{n.inspect} for ##{part}_quoted method"
         end
         blank.each do |n|
-          name = SQLServer::Utils.extract_identifiers(n)
+          name = extract_identifiers(n)
           name.send(:"#{part}").must_be_nil "With #{n.inspect} for ##{part} method"
           name.send(:"#{part}_quoted").must_be_nil "With #{n.inspect} for ##{part}_quoted method"
         end
@@ -67,51 +67,59 @@ class UtilsTestSQLServer < ActiveRecord::TestCase
     end
 
     it 'does not blow up on nil or blank string name' do
-      SQLServer::Utils.extract_identifiers(nil).object.must_be_nil
-      SQLServer::Utils.extract_identifiers(' ').object.must_be_nil
+      extract_identifiers(nil).object.must_be_nil
+      extract_identifiers(' ').object.must_be_nil
     end
 
     it 'has a #quoted that returns a fully quoted name with all identifiers as orginially passed in' do
-      SQLServer::Utils.extract_identifiers('object').quoted.must_equal '[object]'
-      SQLServer::Utils.extract_identifiers('server.database..object').quoted.must_equal '[server].[database]..[object]'
-      SQLServer::Utils.extract_identifiers('[server]...[object]').quoted.must_equal '[server]...[object]'
+      extract_identifiers('object').quoted.must_equal '[object]'
+      extract_identifiers('server.database..object').quoted.must_equal '[server].[database]..[object]'
+      extract_identifiers('[server]...[object]').quoted.must_equal '[server]...[object]'
     end
 
     it 'can take a symbol argument' do
-      SQLServer::Utils.extract_identifiers(:object).object.must_equal 'object'
+      extract_identifiers(:object).object.must_equal 'object'
     end
 
     it 'allows identifiers with periods to work' do
-      SQLServer::Utils.extract_identifiers('[obj.name]').quoted.must_equal '[obj.name]'
-      SQLServer::Utils.extract_identifiers('[obj.name].[foo]').quoted.must_equal '[obj.name].[foo]'
+      extract_identifiers('[obj.name]').quoted.must_equal '[obj.name]'
+      extract_identifiers('[obj.name].[foo]').quoted.must_equal '[obj.name].[foo]'
     end
 
     it 'should indicate if a name is fully qualitified' do
-      SQLServer::Utils.extract_identifiers('object').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('schema.object').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('database.schema.object').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('database.object').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('server...object').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('server.database..object').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('server.database.schema.object').fully_qualified?.must_equal true
-      SQLServer::Utils.extract_identifiers('server.database.schema.').fully_qualified?.must_equal true
-      SQLServer::Utils.extract_identifiers('[obj.name]').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('[schema].[obj.name]').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('[database].[schema].[obj.name]').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('[database].[obj.name]').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('[server.name]...[obj.name]').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('[server.name].[database]..[obj.name]').fully_qualified?.must_equal false
-      SQLServer::Utils.extract_identifiers('[server.name].[database].[schema].[obj.name]').fully_qualified?.must_equal true
-      SQLServer::Utils.extract_identifiers('[server.name].[database].[schema].').fully_qualified?.must_equal true
+      extract_identifiers('object').fully_qualified?.must_equal false
+      extract_identifiers('schema.object').fully_qualified?.must_equal false
+      extract_identifiers('database.schema.object').fully_qualified?.must_equal false
+      extract_identifiers('database.object').fully_qualified?.must_equal false
+      extract_identifiers('server...object').fully_qualified?.must_equal false
+      extract_identifiers('server.database..object').fully_qualified?.must_equal false
+      extract_identifiers('server.database.schema.object').fully_qualified?.must_equal true
+      extract_identifiers('server.database.schema.').fully_qualified?.must_equal true
+      extract_identifiers('[obj.name]').fully_qualified?.must_equal false
+      extract_identifiers('[schema].[obj.name]').fully_qualified?.must_equal false
+      extract_identifiers('[database].[schema].[obj.name]').fully_qualified?.must_equal false
+      extract_identifiers('[database].[obj.name]').fully_qualified?.must_equal false
+      extract_identifiers('[server.name]...[obj.name]').fully_qualified?.must_equal false
+      extract_identifiers('[server.name].[database]..[obj.name]').fully_qualified?.must_equal false
+      extract_identifiers('[server.name].[database].[schema].[obj.name]').fully_qualified?.must_equal true
+      extract_identifiers('[server.name].[database].[schema].').fully_qualified?.must_equal true
     end
 
     it 'can return fully qualified quoted table name' do
-      name = SQLServer::Utils.extract_identifiers('[server.name].[database].[schema].[object]')
+      name = extract_identifiers('[my.server].db.schema.')
+      name.fully_qualified_database_quoted.must_equal '[my.server].[db]'
+      name = extract_identifiers('[server.name].[database].[schema].[object]')
       name.fully_qualified_database_quoted.must_equal '[server.name].[database]'
-      name = SQLServer::Utils.extract_identifiers('server.database.schema.object')
+      name = extract_identifiers('server.database.schema.object')
       name.fully_qualified_database_quoted.must_equal '[server].[database]'
     end
 
   end
 
+  private
+
+  def extract_identifiers(name)
+    SQLServer::Utils.extract_identifiers(name)
+  end
+
 end