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

RandomNoun7 · 2018-02-23T20:58:34Z

The sqlserver_tsql resource does not handle errors returned from T-SQL
statements properly. The errors come back from the query but unless the
phrase 'SQL Server' was included in the error text, the error was not
then passed back to the user/logs. This change ensure that errors
returned are passed back properly by inspecting the Errors property of
the ADO object used to run the query. Note that this may not behave as
expected if the error that occurrs is of sufficently low severity that a
rowset, even if empty, is also returned. If a rowset is returned by the
ADO object, the object will not populate the Errors property, and there
is no way to tell a low severity error occurred. For instance 'Select
1/0' returns a low severity division by zero error, and an empty rowset.
Because the empty rowset exists, the error cannot be seen. It is up to
the user to ensure that the errors they want to catch are of sufficient
severity that execution of the query is halted.

michaeltlombardi · 2018-02-23T21:04:29Z

lib/puppet_x/sqlserver/sql_connection.rb

@@ -9,7 +9,7 @@ def open_and_run_command(query, config)
          open(config)
          execute(query)
        rescue win32_exception => e


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

Probably not. I'll get rid of it.

Hey, turns out I do still need it.

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

michaeltlombardi · 2018-02-23T21:05:07Z

lib/puppet_x/sqlserver/sql_connection.rb

@@ -9,7 +9,7 @@ def open_and_run_command(query, config)
          open(config)
          execute(query)
        rescue win32_exception => e
-          return ResultOutput.new(true, e.message)
+          return ResultOutput.new(true, @connection.Errors(0).Description)


Am I correct in thinking this will only send back the first error? Is it possible/probable to get 2+ errors, and if so, do I care to retrieve the others?

Yes, it will only send back the first error. It seems unlikely enough to receive multiple errors back from a script that it doesn't seem worth the increase in complexity to handle the edge case.

To play devils advocate, also not too much to collect all the descriptions:

messages = (0..@connection.Errors.Count { |i| @connection.Errors(i).Description }).join("\n")

RandomNoun7

@glennsarti Interested in your input, because @jpogran said you might have an opinion.
Technically what I've done here could be viewed by some as a change to the behavior of the module. Previously when errors were encountered in this module, unless the error text started with "SQL Server \n", then the exit status would get set, but not error message would be returned, and if it did start that way, only the portion of the string after that phrase was returned. This change got rid of that and started passing back either the full win32 error message, or the error message from the SQL Query. If a user was depending on the previous behavior for some reason, this could be viewed as a breaking change. I don't personally view it as a breaking change, I think this is a bug fix in error messaging and we should go ahead and merge this PR as it stands once I'm done with adhoc and manual testing.

RandomNoun7 · 2018-02-26T19:01:19Z

lib/puppet_x/sqlserver/sql_connection.rb

@@ -9,7 +9,7 @@ def open_and_run_command(query, config)
          open(config)
          execute(query)
        rescue win32_exception => e


Hey, turns out I do still need it.

glennsarti · 2018-02-27T02:22:46Z

lib/puppet_x/sqlserver/sql_connection.rb

@@ -9,7 +9,7 @@ def open_and_run_command(query, config)
          open(config)
          execute(query)
        rescue win32_exception => e


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

glennsarti · 2018-02-27T02:24:25Z

lib/puppet_x/sqlserver/sql_connection.rb

+        end
+
+        if (@error_message.nil?) && (!error_message.nil?)
+          @error_message = error_message


THis is being set twice. Perhaps move it above and add a return statement.

glennsarti · 2018-02-27T02:25:17Z

spec/unit/puppet_x/sql_connection_spec.rb

@@ -8,8 +8,11 @@

  def stub_connection
    @connection = mock()
+    error = mock()


Now is not the time, but should really move away from using mocha for stubs and just using core rspec doubles.

It took me a while to figure out why all of my reading on how to do mocking in rspec was coming up with a different syntax.

glennsarti · 2018-02-27T02:28:13Z

In this case I don't view it as a change in behvaiour. For example, on non-English version of windows the error message would be different. (English v Japanese for example). It would probably warrant a minor version bump 0.y.0. One thing that will change will be the acceptance tests (assuming there are any for that class)

Iristyle · 2018-02-27T22:10:59Z

lib/puppet_x/sqlserver/sql_connection.rb

+          end
+        end
+
+        unless (!@error_message.nil?) && (error_message.nil?)


The unless + negation is a bit confusing to read, especially when also combined with the &&.

I think your intent here is to set @error_message ONLY when it's not already set?

If that's correct, I think this can this be simplified like:

@error_message ||= error_message

I was looking for a more elegant want to do this. Changing now.

Iristyle · 2018-02-27T22:12:48Z

lib/puppet_x/sqlserver/sql_connection.rb

+          unless connection.Errors.count == 0
+            @error_message = connection.Errors(0).Description
+          end
+        end


Could this be simpler as:

@error_message = connection.Errors(0).Description if connection && !connection.Errors.empty?

The Errors collection is never a null property, it just throws an error if you try to call an index if it's not storing any errors, which is why I'm calling the count property first. I could probably get away with @error_message = connection.Errors(0).Description if connection && !connection.Errors.count == 0

The .empty? I suggested should generally be the same as .count == 0 ... unless this is a COM enumerable? In which case using .count == 0 is probably the correct thing to do.

Worse than I thought. This object just doesn't respond to .empty?. It's a OLE object so I'm guess that's why.

It's probably not an Enumerable object.

That's what I was saying @glennsarti ;0

But not only is it not a COM enumerable like IEnumVariant ... which Ruby cannot enumerate ... but the interface doesn't have enumeration methods anyhow.

https://docs.microsoft.com/en-us/sql/ado/reference/ado-api/errors-collection-properties-methods-and-events

Iristyle · 2018-02-27T22:15:42Z

lib/puppet_x/sqlserver/sql_connection.rb

@@ -90,25 +90,26 @@ def win32_exception
    end

    class ResultOutput
+
      attr_reader :exitstatus, :error_message, :raw_error_message


So this class still has a raw_error_message attribute... but it's not set anymore? Do we still want to keep the concept of what this previously represented?

No, I'll get rid of it. That was previously in place because the larger error message was being parsed for the sql error, but even in the older version it was never called.

Iristyle · 2018-02-27T22:16:54Z

spec/spec_helper.rb

@@ -10,4 +10,3 @@
    e.syntax = [:should, :expect]
  end
 end
-


Accidental newline deletion

Iristyle · 2018-02-28T23:27:31Z

lib/puppet_x/sqlserver/sql_connection.rb

-          @error_message = parse_for_error(error_message)
-        end
+
+        @error_message = connection.Errors(0).Description if connection && !connection.Errors.count == 0


One minor nit here... my initial suggestion could have been better:

@error_message = connection.Errors(0).Description if connection && connection.Errors.count > 0

Iristyle · 2018-03-01T02:02:47Z

lib/puppet_x/sqlserver/sql_connection.rb

+      end
+
+      def extract_messages(connection)
+        error_count = connection.nil? ? -1 : (connection.Errors.count - 1)


How about an early return like this to avoid the nesting?

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")

RandomNoun7 · 2018-03-01T21:23:28Z

Ok, this version incorporates a short circuit in the extract_messages function, it updates the unit tests to work properly with errors contained in the connection object, and it updates the acceptance tests to reflect that the error message returned from T-SQL errors is no longer being parsed and chopped up like it was.

glennsarti · 2018-03-01T23:29:06Z

lib/puppet_x/sqlserver/sql_connection.rb

@@ -9,12 +9,15 @@ def open_and_run_command(query, config)
          open(config)
          execute(query)
        rescue win32_exception => e
-          return ResultOutput.new(true, e.message)
+          # require 'pry'; binding.pry;


glennsarti · 2018-03-01T23:29:12Z

lib/puppet_x/sqlserver/sql_connection.rb

        ensure
          close
        end

-        ResultOutput.new(false, nil)
+        # require 'pry'; binding.pry;


glennsarti · 2018-03-01T23:29:58Z

lib/puppet_x/sqlserver/sql_connection.rb

-          @error_message = parse_for_error(error_message)
-        end
+
+        @error_message = (extract_messages(connection) || error_message)


nit - extra brackets not required.

glennsarti · 2018-03-01T23:31:39Z

lib/puppet_x/sqlserver/sql_connection.rb

-      def initialize(has_errors, error_message)
+      attr_reader :exitstatus, :error_message
+
+      def initialize(has_errors, error_message, connection)


Warning, this variable has the same name as a method

How does ole_connection sound?

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

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

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

Need more ☕️

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

Will rename or remove in a maint commit.

glennsarti · 2018-03-01T23:34:13Z

spec/unit/puppet_x/sql_connection_spec.rb

@@ -20,11 +25,11 @@ 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)


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

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

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...

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.

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

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.

@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.

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.

The sqlserver_tsql resource does not handle errors returned from T-SQL statements properly. The errors come back from the query but unless the phrase 'SQL Server' was included in the error text, the error was not then passed back to the user/logs. This change ensure that errors returned are passed back properly by inspecting the Errors property of the ADO object used to run the query. Note that this may not behave as expected if the error that occurrs is of sufficently low severity that a rowset, even if empty, is also returned. If a rowset is returned by the ADO object, the object will not populate the Errors property, and there is no way to tell a low severity error occurred. For instance 'Select 1/0' returns a low severity division by zero error, and an empty rowset. Because the empty rowset exists, the error cannot be seen. It is up to the user to ensure that the errors they want to catch are of sufficienty severity that execution of the query is halted.

Superceded by updated changes and verbal agreements

Iristyle · 2018-03-05T19:03:24Z

spec/unit/puppet_x/sql_connection_spec.rb

-        subject.stubs(:win32_exception).returns(Exception)
+        stub_connection
+        @connection.Errors.stubs(:count).returns(1)
+        @connection.Errors.stubs(:Description).returns("SQL Error in Connection")


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

michaeltlombardi reviewed Feb 23, 2018

View reviewed changes

RandomNoun7 force-pushed the tickets/main/MODULES-6281-module_does_not_return_errors branch 2 times, most recently from 6397e6f to 63abc02 Compare February 23, 2018 22:29

RandomNoun7 closed this Feb 23, 2018

RandomNoun7 force-pushed the tickets/main/MODULES-6281-module_does_not_return_errors branch from 63abc02 to 45d9721 Compare February 23, 2018 22:35

RandomNoun7 reopened this Feb 23, 2018

RandomNoun7 force-pushed the tickets/main/MODULES-6281-module_does_not_return_errors branch 5 times, most recently from 288a437 to 4ebc3e2 Compare February 26, 2018 18:52

RandomNoun7 requested a review from glennsarti February 26, 2018 19:08

RandomNoun7 commented Feb 26, 2018

View reviewed changes

RandomNoun7 added the do not merge label Feb 26, 2018

glennsarti reviewed Feb 27, 2018

View reviewed changes

RandomNoun7 force-pushed the tickets/main/MODULES-6281-module_does_not_return_errors branch 3 times, most recently from e8bae9b to d89351a Compare February 27, 2018 16:48

RandomNoun7 removed the do not merge label Feb 27, 2018

Iristyle reviewed Feb 27, 2018

View reviewed changes

Iristyle suggested changes Feb 27, 2018

View reviewed changes

Iristyle previously requested changes Feb 27, 2018

View reviewed changes

spec/spec_helper.rb Outdated

@@ -10,4 +10,3 @@

e.syntax = [:should, :expect]

end

end

Copy link

Contributor

Iristyle Feb 27, 2018

Choose a reason for hiding this comment

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

Accidental newline deletion

RandomNoun7 force-pushed the tickets/main/MODULES-6281-module_does_not_return_errors branch 5 times, most recently from bbc77d7 to 9de69b7 Compare February 28, 2018 16:36

Iristyle reviewed Feb 28, 2018

View reviewed changes

RandomNoun7 force-pushed the tickets/main/MODULES-6281-module_does_not_return_errors branch from 9de69b7 to 75e7e46 Compare March 1, 2018 01:24

RandomNoun7 added the do not merge label Mar 1, 2018

RandomNoun7 changed the title ~~(MODULES-6281) Return Errors from T-SQL~~ (WIP) (MODULES-6281) Return Errors from T-SQL Mar 1, 2018

Iristyle reviewed Mar 1, 2018

View reviewed changes

RandomNoun7 force-pushed the tickets/main/MODULES-6281-module_does_not_return_errors branch 2 times, most recently from 9fda054 to 5923ba0 Compare March 1, 2018 21:20

RandomNoun7 force-pushed the tickets/main/MODULES-6281-module_does_not_return_errors branch from 5923ba0 to 6de16ff Compare March 1, 2018 23:31

glennsarti reviewed Mar 1, 2018

View reviewed changes

RandomNoun7 force-pushed the tickets/main/MODULES-6281-module_does_not_return_errors branch from 6de16ff to 78ff9f3 Compare March 1, 2018 23:35

RandomNoun7 force-pushed the tickets/main/MODULES-6281-module_does_not_return_errors branch from 78ff9f3 to 9b8a0fe Compare March 2, 2018 00:09

RandomNoun7 removed the do not merge label Mar 2, 2018

RandomNoun7 changed the title ~~(WIP) (MODULES-6281) Return Errors from T-SQL~~ (MODULES-6281) Return Errors from T-SQL Mar 2, 2018

RandomNoun7 referenced this pull request in ram-devsecops/puppetlabs-iis Mar 5, 2018

(MODULES-6637) Fixing port conflicts if hostname is diff in bindings

597dc8b

michaeltlombardi approved these changes Mar 5, 2018

View reviewed changes

michaeltlombardi merged commit 53162b6 into puppetlabs:master Mar 5, 2018

Iristyle reviewed Mar 5, 2018

View reviewed changes

RandomNoun7 deleted the tickets/main/MODULES-6281-module_does_not_return_errors branch March 5, 2018 19:48

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

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

Conversation

RandomNoun7 commented Feb 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomNoun7 Feb 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomNoun7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti commented Feb 27, 2018

Iristyle Feb 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomNoun7 commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomNoun7 Mar 1, 2018 • edited

Choose a reason for hiding this comment

Iristyle Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomNoun7 commented Feb 23, 2018 •

edited

RandomNoun7 Feb 23, 2018 •

edited

Iristyle Feb 27, 2018 •

edited

glennsarti Mar 1, 2018 •

edited

RandomNoun7 Mar 1, 2018 •

edited

Iristyle Mar 1, 2018 •

edited