From 302d873a5250c59bd885fdfce99409789f33db26 Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 10:57:36 -0700 Subject: [PATCH 01/11] (maint) Remove unused TSQL provider run_check - This method only appears in tests --- lib/puppet/provider/sqlserver_tsql/mssql.rb | 4 ---- spec/unit/puppet/provider/sqlserver__tsql_spec.rb | 6 +++--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/puppet/provider/sqlserver_tsql/mssql.rb b/lib/puppet/provider/sqlserver_tsql/mssql.rb index a5d6c7b4..e5ec242d 100644 --- a/lib/puppet/provider/sqlserver_tsql/mssql.rb +++ b/lib/puppet/provider/sqlserver_tsql/mssql.rb @@ -25,10 +25,6 @@ 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 diff --git a/spec/unit/puppet/provider/sqlserver__tsql_spec.rb b/spec/unit/puppet/provider/sqlserver__tsql_spec.rb index 687e00ae..7f36b8ac 100644 --- a/spec/unit/puppet/provider/sqlserver__tsql_spec.rb +++ b/spec/unit/puppet/provider/sqlserver__tsql_spec.rb @@ -57,14 +57,14 @@ def gen_query(query) } 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 From 9b9668e0d6e07e94109e765c317f54bd8be721ce Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 11:01:16 -0700 Subject: [PATCH 02/11] (maint) Clean SqlConnection.open_and_run_command - Minor refactor to make the return value more obvious --- lib/puppet_x/sqlserver/sql_connection.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/puppet_x/sqlserver/sql_connection.rb b/lib/puppet_x/sqlserver/sql_connection.rb index 714dc843..b1fd1c68 100644 --- a/lib/puppet_x/sqlserver/sql_connection.rb +++ b/lib/puppet_x/sqlserver/sql_connection.rb @@ -16,7 +16,7 @@ def open_and_run_command(query, config) close end - result + ResultOutput.new(has_errors, error_message) end private @@ -56,10 +56,6 @@ def command(sql) nil end - def result - ResultOutput.new(has_errors, error_message) - end - def has_errors @exception_caught != nil end From 0306d2096b2cc2ae10fd84a36207c1f6f95bea3e Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 11:08:21 -0700 Subject: [PATCH 03/11] (maint) Remove TSQL provider run_update - This method is extraneous / confusing. Better to refactor all code to execute with the providers `run` method --- lib/puppet/provider/sqlserver_tsql/mssql.rb | 4 ---- lib/puppet/type/sqlserver_tsql.rb | 4 ++-- spec/unit/puppet/provider/sqlserver__tsql_spec.rb | 6 +++--- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/puppet/provider/sqlserver_tsql/mssql.rb b/lib/puppet/provider/sqlserver_tsql/mssql.rb index e5ec242d..ae020841 100644 --- a/lib/puppet/provider/sqlserver_tsql/mssql.rb +++ b/lib/puppet/provider/sqlserver_tsql/mssql.rb @@ -24,8 +24,4 @@ def get_config config_resc.original_parameters end - - def run_update - return self.run(resource[:command]) - end end diff --git a/lib/puppet/type/sqlserver_tsql.rb b/lib/puppet/type/sqlserver_tsql.rb index fe648841..c34386e1 100644 --- a/lib/puppet/type/sqlserver_tsql.rb +++ b/lib/puppet/type/sqlserver_tsql.rb @@ -68,7 +68,7 @@ def output def refresh if self.check_all_attributes(true) - result = provider.run_update + result = provider.run(self.resource[:command]) if result.has_errors fail("Unable to apply changes, failed with error message #{result.error_message}") end @@ -110,7 +110,7 @@ def retrieve def sync event = :executed_command begin - @output = provider.run_update + @output = provider.run(self.resource[:command]) end unless @output.exitstatus.to_s == "0" self.fail("#{self.resource[:command]} returned #{@output.exitstatus} instead of one of [#{self.should.join(",")}]") diff --git a/spec/unit/puppet/provider/sqlserver__tsql_spec.rb b/spec/unit/puppet/provider/sqlserver__tsql_spec.rb index 7f36b8ac..0fbad706 100644 --- a/spec/unit/puppet/provider/sqlserver__tsql_spec.rb +++ b/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,7 +53,7 @@ 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 From 7adf09a93156a873c18de5bf74529a32a230ffc2 Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 11:14:39 -0700 Subject: [PATCH 04/11] (FM-5324) Fix TSQL error propagation - Introduce a minor refactor so that the `returns` property captures errors properly from TSQL executions. Previously, if a `refresh` occurred against a sqlserver_tsql the output would cause a failure, but simply executing TSQL would not do the same. Follow the same model as the Puppet exec type (where much of this code comes from), and have `refresh` trigger the `sync` method of the `results` property, so that the code is now shared. --- lib/puppet/type/sqlserver_tsql.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/puppet/type/sqlserver_tsql.rb b/lib/puppet/type/sqlserver_tsql.rb index c34386e1..d61dfe04 100644 --- a/lib/puppet/type/sqlserver_tsql.rb +++ b/lib/puppet/type/sqlserver_tsql.rb @@ -68,10 +68,7 @@ def output def refresh if self.check_all_attributes(true) - result = provider.run(self.resource[:command]) - if result.has_errors - fail("Unable to apply changes, failed with error message #{result.error_message}") - end + self.property(:returns).sync end end @@ -111,6 +108,9 @@ def sync event = :executed_command begin @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(",")}]") From 219183278cee24fd67f4bfefa6426039be53d84f Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 11:50:29 -0700 Subject: [PATCH 05/11] (maint) Simplify SqlConnection#command - The command method for SqlConnection only has a single caller, the block form is never used and the result is not either. Furthermore, the caller open_and_run_command, already catches win32_exception, so there's more code here than necessary. --- lib/puppet_x/sqlserver/sql_connection.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/puppet_x/sqlserver/sql_connection.rb b/lib/puppet_x/sqlserver/sql_connection.rb index b1fd1c68..4319a923 100644 --- a/lib/puppet_x/sqlserver/sql_connection.rb +++ b/lib/puppet_x/sqlserver/sql_connection.rb @@ -47,13 +47,7 @@ def get_connection_string(config) def command(sql) reset_instance - begin - r = execute(sql) - yield(r) if block_given? - rescue win32_exception => e - @exception_caught = e - end - nil + execute(sql) end def has_errors From d61b018b148f8524a19c876a2e5eb9726b226d76 Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 13:18:30 -0700 Subject: [PATCH 06/11] (maint) Remove SqlConnection#command - The existing code only has one code path, so remove the unneeded method. --- lib/puppet_x/sqlserver/sql_connection.rb | 8 ++------ spec/unit/puppet_x/sql_connection_spec.rb | 4 ++-- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/puppet_x/sqlserver/sql_connection.rb b/lib/puppet_x/sqlserver/sql_connection.rb index 4319a923..1917d4c3 100644 --- a/lib/puppet_x/sqlserver/sql_connection.rb +++ b/lib/puppet_x/sqlserver/sql_connection.rb @@ -9,7 +9,8 @@ class SqlConnection def open_and_run_command(query, config) begin open(config) - command(query) + reset_instance + execute(query) rescue win32_exception => e @exception_caught = e ensure @@ -45,11 +46,6 @@ def get_connection_string(config) params.map { |k, v| "#{k}=#{v}" }.join(';') end - def command(sql) - reset_instance - execute(sql) - end - def has_errors @exception_caught != nil end diff --git a/spec/unit/puppet_x/sql_connection_spec.rb b/spec/unit/puppet_x/sql_connection_spec.rb index 3281025a..2054fecf 100644 --- a/spec/unit/puppet_x/sql_connection_spec.rb +++ b/spec/unit/puppet_x/sql_connection_spec.rb @@ -18,7 +18,7 @@ def stub_no_errors end describe 'open_and_run_command' do - context 'command' do + context 'command execution' do before :each do stub_connection @connection.stubs(:State).returns(0) @@ -68,7 +68,7 @@ def stub_no_errors context 'return result with errors' do it { 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') subject.expects(:close).once subject.stubs(:has_errors).returns(:true) subject.stubs(:error_message).returns( From 2bd90426111f22fb1f16cd0efc20f260aa0caff4 Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 14:00:48 -0700 Subject: [PATCH 07/11] (maint) Make SqlConnection state tests explicit - Tests should be better (self) documented --- spec/unit/puppet_x/sql_connection_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/unit/puppet_x/sql_connection_spec.rb b/spec/unit/puppet_x/sql_connection_spec.rb index 2054fecf..36035e56 100644 --- a/spec/unit/puppet_x/sql_connection_spec.rb +++ b/spec/unit/puppet_x/sql_connection_spec.rb @@ -9,6 +9,7 @@ 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 @@ -21,7 +22,6 @@ def stub_no_errors 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 @@ -45,7 +45,6 @@ def stub_no_errors before :each do stub_connection stub_no_errors - @connection.stubs(:State).returns(0) 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') @@ -60,7 +59,7 @@ def stub_no_errors 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 From a3b1d3f41f499749b8bb1f3a3439cd9f949478e1 Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 14:20:22 -0700 Subject: [PATCH 08/11] (maint) Loosen regex restriction on SQL error - Properly parse errors that look like: SQL Server error message instead of those that must have whitespace like: SQL Server error message --- lib/puppet_x/sqlserver/sql_connection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/puppet_x/sqlserver/sql_connection.rb b/lib/puppet_x/sqlserver/sql_connection.rb index 1917d4c3..ab3959d0 100644 --- a/lib/puppet_x/sqlserver/sql_connection.rb +++ b/lib/puppet_x/sqlserver/sql_connection.rb @@ -105,7 +105,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 From 99975630ab7c0aa972734a93c0e4317f95015bed Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 14:26:12 -0700 Subject: [PATCH 09/11] (maint) Simplify SqlConnection error handling - The existing SqlConnection class has a public property exception_caught, which is not used. Further, it uses this value to save off exceptions inside the class, rather than simply returning the parsed errors when open_and_run_command is executed, which is presently the only caller that ever looks at the error values. This provides an opportunity to simplify the code paths, and remove unnecessary caching of errors and the 2 unnecessary methods has_errors and error_message from the SqlConnection class. Instead, directly return a new ResultOutput instance as soon as SQL code is executed. Tests are modified accordingly. --- lib/puppet_x/sqlserver/sql_connection.rb | 19 ++----------------- spec/unit/puppet_x/sql_connection_spec.rb | 17 +++-------------- 2 files changed, 5 insertions(+), 31 deletions(-) diff --git a/lib/puppet_x/sqlserver/sql_connection.rb b/lib/puppet_x/sqlserver/sql_connection.rb index ab3959d0..56ba188c 100644 --- a/lib/puppet_x/sqlserver/sql_connection.rb +++ b/lib/puppet_x/sqlserver/sql_connection.rb @@ -4,20 +4,17 @@ module Sqlserver CONNECTION_CLOSED = 0 class SqlConnection - attr_reader :exception_caught - def open_and_run_command(query, config) begin open(config) - reset_instance execute(query) rescue win32_exception => e - @exception_caught = e + return ResultOutput.new(true, e.message) ensure close end - ResultOutput.new(has_errors, error_message) + ResultOutput.new(false, nil) end private @@ -46,14 +43,6 @@ def get_connection_string(config) params.map { |k, v| "#{k}=#{v}" }.join(';') 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? @@ -61,10 +50,6 @@ def close end end - def reset_instance - @exception_caught = nil - end - def connection_closed? connection.State == CONNECTION_CLOSED end diff --git a/spec/unit/puppet_x/sql_connection_spec.rb b/spec/unit/puppet_x/sql_connection_spec.rb index 36035e56..44d13f9e 100644 --- a/spec/unit/puppet_x/sql_connection_spec.rb +++ b/spec/unit/puppet_x/sql_connection_spec.rb @@ -13,11 +13,6 @@ def stub_connection 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 execution' do before :each do @@ -25,7 +20,6 @@ def stub_no_errors @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,7 +38,7 @@ def stub_no_errors context 'closed connection' do before :each do stub_connection - stub_no_errors + @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') @@ -66,13 +60,10 @@ def stub_no_errors 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(:execute).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) @@ -84,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) From 305e872f587e4b44fbd0ef49ffd09130bc0d60ba Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 15:12:44 -0700 Subject: [PATCH 10/11] (FM-5324) Emit debug output on failed onlyif TSQL - Previously, there was no way of getting the log output from SQL Server when executing TSQL during an onlyif. Only the status code was emitted, which is not helpful when trying to debug. --- lib/puppet/type/sqlserver_tsql.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/puppet/type/sqlserver_tsql.rb b/lib/puppet/type/sqlserver_tsql.rb index d61dfe04..91509d52 100644 --- a/lib/puppet/type/sqlserver_tsql.rb +++ b/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 From 12035863ea2279bb18cd5b3acbbd4fe9bbd19106 Mon Sep 17 00:00:00 2001 From: Iristyle Date: Wed, 8 Jun 2016 15:13:44 -0700 Subject: [PATCH 11/11] (maint) Fix SqlConnection connection string tests - These tests were previously stubbing return values instead of asserting that the code was behaving properly. --- spec/unit/puppet_x/sql_connection_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/unit/puppet_x/sql_connection_spec.rb b/spec/unit/puppet_x/sql_connection_spec.rb index 44d13f9e..cf77bdf9 100644 --- a/spec/unit/puppet_x/sql_connection_spec.rb +++ b/spec/unit/puppet_x/sql_connection_spec.rb @@ -41,11 +41,11 @@ def stub_connection @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