Merge pull request #173 from Iristyle/maint/sqlconnection-refactor

(FM-5324) Fix TSQL error propagation

Author: glennsarti

lib/puppet/provider/sqlserver_tsql/mssql.rb

@@ -24,12 +24,4 @@ def get_config
 
     config_resc.original_parameters
   end
-
-  def run_check
-    return self.run(resource[:onlyif])
-  end
-
-  def run_update
-    return self.run(resource[:command])
-  end
 end

lib/puppet/type/sqlserver_tsql.rb

@@ -42,6 +42,7 @@ def self.checks
     def check(value)
       output = provider.run(value)
       debug("OnlyIf returned exitstatus of #{output.exitstatus}")
+      debug("OnlyIf error: #{output.error_message}") if output.has_errors
       output.exitstatus != 0
     end
   end
@@ -68,10 +69,7 @@ def output
 
   def refresh
     if self.check_all_attributes(true)
-      result = provider.run_update
-      if result.has_errors
-        fail("Unable to apply changes, failed with error message #{result.error_message}")
-      end
+      self.property(:returns).sync
     end
   end
 
@@ -110,7 +108,10 @@ def retrieve
     def sync
       event = :executed_command
       begin
-        @output = provider.run_update
+        @output = provider.run(self.resource[:command])
+        if @output.has_errors
+          fail("Unable to apply changes, failed with error message #{@output.error_message}")
+        end
       end
       unless @output.exitstatus.to_s == "0"
         self.fail("#{self.resource[:command]} returned #{@output.exitstatus} instead of one of [#{self.should.join(",")}]")

lib/puppet_x/sqlserver/sql_connection.rb

@@ -4,19 +4,17 @@ module Sqlserver
     CONNECTION_CLOSED = 0
 
     class SqlConnection
-      attr_reader :exception_caught
-
       def open_and_run_command(query, config)
         begin
           open(config)
-          command(query)
+          execute(query)
         rescue win32_exception => e
-          @exception_caught = e
+          return ResultOutput.new(true, e.message)
         ensure
           close
         end
 
-        result
+        ResultOutput.new(false, nil)
       end
 
       private
@@ -45,40 +43,13 @@ def get_connection_string(config)
         params.map { |k, v| "#{k}=#{v}" }.join(';')
       end
 
-      def command(sql)
-        reset_instance
-        begin
-          r = execute(sql)
-          yield(r) if block_given?
-        rescue win32_exception => e
-          @exception_caught = e
-        end
-        nil
-      end
-
-      def result
-        ResultOutput.new(has_errors, error_message)
-      end
-
-      def has_errors
-        @exception_caught != nil
-      end
-
-      def error_message
-        @exception_caught.message unless @exception_caught == nil
-      end
-
       def close
         begin
           connection.Close unless connection_closed?
         rescue win32_exception => e
         end
       end
 
-      def reset_instance
-        @exception_caught = nil
-      end
-
       def connection_closed?
         connection.State == CONNECTION_CLOSED
       end
@@ -119,7 +90,7 @@ def has_errors
 
       private
       def parse_for_error(result)
-        match = result.match(/SQL Server\n\s+(.*)/i)
+        match = result.match(/SQL Server\n\s*(.*)/i)
         match[1] unless match == nil
       end
     end

spec/unit/puppet/provider/sqlserver__tsql_spec.rb

@@ -37,14 +37,14 @@ def gen_query(query)
     PP
   end
 
-  context 'run_update' do
+  context 'run with a command' do
     describe 'against non master database' do
       it {
         create_sqlserver_tsql({:title => 'runme', :command => 'whacka whacka', :instance => 'MSSQLSERVER', :database => 'myDb'})
         stub_get_instance_config(config)
         stub_open_and_run('whacka whacka', config.merge({:database => 'myDb'}))
 
-        @provider.run_update
+        @provider.run(gen_query('whacka whacka'))
       }
     end
     describe 'against default database' do
@@ -53,18 +53,18 @@ def gen_query(query)
         stub_get_instance_config(config)
         stub_open_and_run('whacka whacka', config.merge({:database => 'master'}))
 
-        @provider.run_update
+        @provider.run(gen_query('whacka whacka'))
       }
     end
   end
-  context 'run_check' do
+  context 'run with onlyif' do
     describe 'against default database' do
       it {
         create_sqlserver_tsql({:title => 'runme', :command => 'whacka whacka', :onlyif => 'fozy wozy', :instance => 'MSSQLSERVER'})
         stub_get_instance_config(config)
         stub_open_and_run('fozy wozy', config.merge({:database => 'master'}))
 
-        @provider.run_check
+        @provider.run(gen_query('fozy wozy'))
       }
     end
     describe 'against non master database' do
@@ -78,7 +78,7 @@ def gen_query(query)
         stub_get_instance_config(config)
         stub_open_and_run('fozy wozy', config.merge({:database => 'myDb'}))
 
-        @provider.run_check
+        @provider.run(gen_query('fozy wozy'))
       }
     end
   end

spec/unit/puppet_x/sql_connection_spec.rb

@@ -9,23 +9,17 @@
   def stub_connection
     @connection = mock()
     subject.stubs(:create_connection).returns(@connection)
+    @connection.stubs(:State).returns(PuppetX::Sqlserver::CONNECTION_CLOSED)
     subject.stubs(:win32_exception).returns(Exception)
   end
 
-  def stub_no_errors
-    subject.stubs(:has_errors).returns(false)
-    subject.stubs(:error_message).returns(nil)
-  end
-
   describe 'open_and_run_command' do
-    context 'command' do
+    context 'command execution' do
       before :each do
         stub_connection
-        @connection.stubs(:State).returns(0)
         @connection.stubs(:Open).with('Provider=SQLOLEDB.1;User ID=sa;Password=Pupp3t1@;Initial Catalog=master;Application Name=Puppet;Data Source=localhost')
       end
       it 'should not raise an error but populate has_errors with message' do
-        subject.stubs(:win32_exception).returns(Exception)
         subject.stubs(:execute).raises(Exception.new("SQL Server\n error has happened"))
         expect {
           result = subject.open_and_run_command('whacka whacka whacka', config)
@@ -44,36 +38,32 @@ def stub_no_errors
     context 'closed connection' do
       before :each do
         stub_connection
-        stub_no_errors
-        @connection.stubs(:State).returns(0)
+        @connection.stubs(:Execute)
       end
       it 'should not add MSSQLSERVER to connection string' do
-        @connection.stubs(:Open).with('Provider=SQLOLEDB.1;User ID=sa;Password=Pupp3t1@;Initial Catalog=master;Application Name=Puppet;Data Source=localhost')
+        @connection.expects(:Open).with('Provider=SQLOLEDB.1;User ID=sa;Password=Pupp3t1@;Initial Catalog=master;Application Name=Puppet;Data Source=localhost')
         subject.open_and_run_command('query', config)
       end
       it 'should add a non default instance to connection string' do
-        @connection.stubs(:Open).with('Provider=SQLOLEDB.1;User ID=superuser;Password=puppetTested;Initial Catalog=master;Application Name=Puppet;Data Source=localhost\LOGGING')
+        @connection.expects(:Open).with('Provider=SQLOLEDB.1;User ID=superuser;Password=puppetTested;Initial Catalog=master;Application Name=Puppet;Data Source=localhost\LOGGING')
         subject.open_and_run_command('query', {:admin_user => 'superuser', :admin_pass => 'puppetTested', :instance_name => 'LOGGING'})
       end
     end
     context 'open connection' do
       it 'should not reopen an existing connection' do
         stub_connection
         @connection.expects(:open).never
-        @connection.stubs(:State).returns(1)
+        @connection.stubs(:State).returns(1) # any value other than CONNECTION_CLOSED
         @connection.expects(:Execute).with('query', nil, nil)
         subject.open_and_run_command('query', config)
       end
     end
     context 'return result with errors' do
       it {
+        subject.stubs(:win32_exception).returns(Exception)
         subject.expects(:open).with({:admin_user => 'sa', :admin_pass => 'Pupp3t1@', :instance_name => 'MSSQLSERVER'})
-        subject.expects(:command).with('SELECT * FROM sys.databases')
+        subject.expects(:execute).with('SELECT * FROM sys.databases').raises(Exception.new("SQL Server\ninvalid syntax provider"))
         subject.expects(:close).once
-        subject.stubs(:has_errors).returns(:true)
-        subject.stubs(:error_message).returns(
-          'SQL Server
-    invalid syntax provider')
         result =
           subject.open_and_run_command('SELECT * FROM sys.databases', config)
         expect(result.exitstatus).to eq(1)
@@ -85,8 +75,6 @@ def stub_no_errors
         stub_connection
         err_message = "SQL Server\n ConnectionFailed"
         @connection.stubs(:Open).raises(Exception.new(err_message))
-        subject.stubs(:has_errors).returns(true)
-        subject.stubs(:error_message).returns(err_message)
         expect {
           result = subject.open_and_run_command('whacka whacka whacka', config)
           expect(result.exitstatus).to eq(1)