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

(MODULES-6281) Return Errors from T-SQL #263

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a
- During acceptance testing, only execute master provisioning steps if there is
a master in the hosts array.
- Stop running ```gem update bundler``` during Travis runs. ([MODULES-6339](https://tickets.puppetlabs.com/browse/MODULES6339))
- The `sqlserver_tsql` resource now returns errors from sql queries properly. ([MODULES-6281](https://tickets.puppetlabs.com/browse/MODULES-6281))

## [2.1.0] - 2017-12-8

Expand Down
29 changes: 15 additions & 14 deletions lib/puppet_x/sqlserver/sql_connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ def open_and_run_command(query, config)
open(config)
execute(query)
rescue win32_exception => e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is => e still necessary if you're not going to do anything with the specific exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not. I'll get rid of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, turns out I do still need it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General rubyism is to prefix with underscore for required, but unused variables _e

return ResultOutput.new(true, e.message)
return ResultOutput.new(true, e.message, @connection)
ensure
close
end

ResultOutput.new(false, nil)
ResultOutput.new(false, nil, @connection)
end

private
Expand Down Expand Up @@ -90,24 +90,25 @@ def win32_exception
end

class ResultOutput
attr_reader :exitstatus, :error_message, :raw_error_message

def initialize(has_errors, error_message)
attr_reader :exitstatus, :error_message

def initialize(has_errors, error_message, connection)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning, this variable has the same name as a method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does ole_connection sound?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RandomNoun7 its the @has_errors bit that should be changed (which predates your code)... connection is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh. I can change it now, or we can call it out of scope for the ticket. I'm fine either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derp.....I should've read more carefully. I was meaning the has_errors but you didn't change that.

Need more ☕️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change the name of the method to errors? and it doesn't seem to break anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will rename or remove in a maint commit.

@exitstatus = has_errors ? 1 : 0
if error_message
@raw_error_message = error_message
@error_message = parse_for_error(error_message)
end

@error_message = extract_messages(connection) || error_message
end

def has_errors
@exitstatus != 0
def extract_messages(connection)
return nil if connection.nil? || connection.Errors.count == 0

error_count = connection.Errors.count - 1

((0..error_count).map { |i| connection.Errors(i).Description.to_s}).join("\n")
end

private
def parse_for_error(result)
match = result.match(/SQL Server\n\s*(.*)/i)
match[1] unless match == nil
def has_errors
@exitstatus != 0
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions spec/acceptance/sqlserver_tsql_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ def ensure_sqlserver_database(db_name, ensure_val = 'present')
}
MANIFEST
execute_manifest(pp, {:acceptable_exit_codes => [0,1]}) do |r|
expect(r.stderr).to match(/Error/i)
expect(r.stderr).to match(/Incorrect syntax/i)
end
end

Expand All @@ -210,7 +210,7 @@ def ensure_sqlserver_database(db_name, ensure_val = 'present')
}
MANIFEST
execute_manifest(pp, {:acceptable_exit_codes => [0,1]}) do |r|
expect(r.stderr).to match(/Error/i)
expect(r.stderr).to match(/Non-Existing-Database/i)
end
end
end
Expand Down
23 changes: 15 additions & 8 deletions spec/unit/puppet_x/sql_connection_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@

def stub_connection
@connection = mock()
error_mock = mock()
error_mock.stubs(:count).returns(0)

subject.stubs(:create_connection).returns(@connection)
@connection.stubs(:State).returns(PuppetX::Sqlserver::CONNECTION_CLOSED)
@connection.stubs(:Errors).returns(error_mock)
subject.stubs(:win32_exception).returns(Exception)
end

Expand All @@ -20,11 +24,12 @@ def stub_connection
@connection.stubs(:Open).with('Provider=SQLNCLI11;Initial Catalog=master;Application Name=Puppet;Data Source=.;DataTypeComptibility=80;User ID=sa;Password=Pupp3t1@')
end
it 'should not raise an error but populate has_errors with message' do
subject.stubs(:execute).raises(Exception.new("SQL Server\n error has happened"))
@connection.Errors.stubs(:count).returns(1)
Copy link
Contributor

@glennsarti glennsarti Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems an odd stub to add. Should instead it be stubbing a an entire Errors construct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to have it there, or it never attempts to examine the @connection object for errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stub is fine... he has to simulate a populated ADODB Errors connection object, as the existence of errors in this collection changes the behavior of the error reporting.

It may be possible to create an instance of the real thing, but that would require Windows...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need to create and test the real thing is why I modified the acceptance tests to ensure the proper object is examined and just the win32 exception message passed into the rescue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it "feels odd" to me that your stubbing the length of an array, but not setting the content of the Array too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's a strange object. Calling count on the Errors object isn't just asking for the length of the array as Errors isn't a plain array object. count is a real method on the object that has other properties and methods. So in this case I'm stubbing a method call. The content that gets returned is set higher up in the stub_connection helper at the top of the file. It defaults to zero, but in the case of tests where I actually want to take a look at the error text in the object I have to override that and return 1 as the count.

Copy link
Contributor

@Iristyle Iristyle Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glennsarti yeah, higher up he sets error_mock.stubs(:Description).returns("SQL Error in Connection") ... but since the default is 0 items, I agree it should probably be done where the length is stubbed to 1 to make things clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two test cases where this was used have been modified to set the non default count value and the Description value inside the test case.

@connection.Errors.stubs(:Description).returns("SQL Error in Connection")
expect {
result = subject.open_and_run_command('whacka whacka whacka', config)
expect(result.exitstatus).to eq(1)
expect(result.error_message).to eq('error has happened')
expect(result.error_message).to eq('SQL Error in Connection')
}.to_not raise_error(Exception)

end
Expand Down Expand Up @@ -114,20 +119,22 @@ def stub_connection
end
context 'return result with errors' do
it {
subject.stubs(:win32_exception).returns(Exception)
stub_connection
@connection.Errors.stubs(:count).returns(1)
@connection.Errors.stubs(:Description).returns("SQL Error in Connection")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mocking library makes this magically work... but I read this as supplying a value for:

@connection.Errors.Description

When I think we're interested in:

@connection.Errors(0).Description

@connection.stubs(:Execute).raises(Exception)
subject.expects(:open).with({:admin_user => 'sa', :admin_pass => 'Pupp3t1@', :instance_name => 'MSSQLSERVER'})
subject.expects(:execute).with('SELECT * FROM sys.databases').raises(Exception.new("SQL Server\ninvalid syntax provider"))
subject.expects(:close).once
result =
subject.open_and_run_command('SELECT * FROM sys.databases', config)

result = subject.open_and_run_command('SELECT * FROM sys.databases', config)
expect(result.exitstatus).to eq(1)
expect(result.error_message).to eq('invalid syntax provider')
expect(result.error_message).to eq('SQL Error in Connection')
}
end
context 'open connection failure' do
it {
stub_connection
err_message = "SQL Server\n ConnectionFailed"
err_message = "ConnectionFailed"
@connection.stubs(:Open).raises(Exception.new(err_message))
expect {
result = subject.open_and_run_command('whacka whacka whacka', config)
Expand Down