(MODULES-775) Fix refresh/unless parameter interactions

Interactions between resource refreshes and the 'unless' parameter have been
fixed to follow the behaviour of the 'exec' type.

The 'unless' parameter is now always taken into account, whether in ordinary
operation, during a refresh, or when refreshonly is set to true.  The resource
will not run the SQL command when the 'unless' clause matches a row.

Previously a refresh on a resource would ignore the 'unless' parameter if set
which could cause a failure re-running a command, such as attempting to create
a role that already exists.

The following examples have been fixed:

  * should not run SQL when refreshed and the unless query returns rows
  * with refreshonly should not run SQL when the unless query returns no rows
  * with refreshonly should not run SQL when refreshed and the unless query
    returns rows

This is done by moving the logic for refreshonly and whether to run the SQL
command from the provider into the type, and consolidating it in the
should_run_sql method which is called during 'command' property retrieval
(instead of sync) and during refresh.

Gemfile

@@ -6,7 +6,7 @@ group :development, :test do
   gem 'rspec-puppet', '~> 1.0'
   gem 'puppet-lint', '~> 0.3.2'
   gem 'beaker',                  :require => false
-  gem 'beaker-rspec',           :require => false
+  gem 'beaker-rspec', '> 2.0.0', :require => false
   gem 'serverspec',              :require => false
 end
 

lib/puppet/provider/postgresql_psql/ruby.rb

@@ -1,55 +1,5 @@
 Puppet::Type.type(:postgresql_psql).provide(:ruby) do
 
-  def command()
-    if ((! resource[:unless]) or (resource[:unless].empty?))
-      if (resource.refreshonly?)
-        # So, if there's no 'unless', and we're in "refreshonly" mode,
-        # we need to return the target command here.  If we don't,
-        # then Puppet will generate an event indicating that this
-        # property has changed.
-        return resource[:command]
-      end
-
-      # if we're not in refreshonly mode, then we return nil,
-      # which will cause Puppet to sync this property.  This
-      # is what we want if there is no 'unless' value specified.
-      return nil
-    end
-
-    if Puppet::PUPPETVERSION.to_f < 4
-      output, status = run_unless_sql_command(resource[:unless])
-    else
-      output = run_unless_sql_command(resource[:unless])
-      status = output.exitcode
-    end
-
-    if status != 0
-      puts status
-      self.fail("Error evaluating 'unless' clause: '#{output}'")
-    end
-    result_count = output.strip.to_i
-    if result_count > 0
-      # If the 'unless' query returned rows, then we don't want to execute
-      # the 'command'.  Returning the target 'command' here will cause
-      # Puppet to treat this property as already being 'insync?', so it
-      # won't call the setter to run the 'command' later.
-      return resource[:command]
-    end
-
-    # Returning 'nil' here will cause Puppet to see this property
-    # as out-of-sync, so it will call the setter later.
-    nil
-  end
-
-  def command=(val)
-    output, status = run_sql_command(val)
-
-    if status != 0
-      self.fail("Error executing SQL; psql returned #{status}: '#{output}'")
-    end
-  end
-
-
   def run_unless_sql_command(sql)
     # for the 'unless' queries, we wrap the user's query in a 'SELECT COUNT',
     # which makes it easier to parse and process the output.
@@ -75,6 +25,8 @@ Puppet::Type.type(:postgresql_psql).provide(:ruby) do
     end
   end
 
+  private
+
   def run_command(command, user, group)
     if Puppet::PUPPETVERSION.to_f < 3.4
       Puppet::Util::SUIDManager.run_and_capture(command, user, group)

lib/puppet/type/postgresql_psql.rb

@@ -10,32 +10,20 @@ Puppet::Type.newtype(:postgresql_psql) do
 
     defaultto { @resource[:name] }
 
-    def sync(refreshing = false)
+    # If needing to run the SQL command, return a fake value that will trigger
-      # We're overriding 'sync' here in order to do some magic
+    # a sync, else return the expected SQL command so no sync takes place
-      # in support of providing a 'refreshonly' parameter.  This
+    def retrieve
-      # is kind of hacky because the logic for 'refreshonly' is
+      if @resource.should_run_sql
-      # spread between the type and the provider, but this is
+        return :notrun
-      # the least horrible way that I could determine to accomplish
-      # it.
-      #
-      # Note that our overridden version of 'sync' takes a parameter,
-      # 'refreshing', which the parent version doesn't take.  This
-      # allows us to call the sync method directly from the 'refresh'
-      # method, and then inside of the body of 'sync' we can tell
-      # whether or not we're refreshing.
-
-      if (!@resource.refreshonly? || refreshing)
-        # If we're not in 'refreshonly' mode, or we're not currently
-        # refreshing, then we just call the parent method.
-        super()
       else
-        # If we get here, it means we're in 'refreshonly' mode and
+        return self.should
-        # we're not being called by the 'refresh' method, so we
-        # just no-op.  We'll be called again by the 'refresh'
-        # method momentarily.
-        nil
       end
     end
+
+    def sync
+      output, status = provider.run_sql_command(value)
+      self.fail("Error executing SQL; psql returned #{status}: '#{output}'") unless status == 0
+    end
   end
 
   newparam(:unless) do
@@ -43,6 +31,21 @@ Puppet::Type.newtype(:postgresql_psql) do
         "this is generally intended to be used for idempotency, to check " +
         "for the existence of an object in the database to determine whether " +
         "or not the main SQL command needs to be executed at all."
+
+    # Return true if a matching row is found
+    def matches(value)
+      if Puppet::PUPPETVERSION.to_f < 4
+        output, status = provider.run_unless_sql_command(value)
+      else
+        output = provider.run_unless_sql_command(value)
+        status = output.exitcode
+      end
+      self.fail("Error evaluating 'unless' clause, returned #{status}: '#{output}'") unless status == 0
+
+      result_count = output.strip.to_i
+      self.debug("Found #{result_count} row(s) executing 'unless' clause")
+      result_count > 0
+    end
   end
 
   newparam(:db) do
@@ -84,10 +87,15 @@ Puppet::Type.newtype(:postgresql_psql) do
     newvalues(:true, :false)
   end
 
-  def refresh()
+  def should_run_sql(refreshing = false)
-    # All of the magic for this type is attached to the ':command' property, so
+    unless_param = @parameters[:unless]
-    # we just need to sync it to accomplish a 'refresh'.
+    return false if !unless_param.nil? && !unless_param.value.nil? && unless_param.matches(unless_param.value)
-    self.property(:command).sync(true)
+    return false if !refreshing && @parameters[:refreshonly].value == :true
+    true
+  end
+
+  def refresh
+    self.property(:command).sync if self.should_run_sql(true)
   end
 
 end

spec/acceptance/postgresql_psql_spec.rb

@@ -6,6 +6,22 @@ describe 'postgresql_psql:', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osf
     apply_manifest("class { 'postgresql::server': ensure => absent }", :catch_failures => true)
   end
 
+  it 'should always run SQL' do
+    pp = <<-EOS.unindent
+      class { 'postgresql::server': }
+
+      postgresql_psql { 'foobar':
+        db        => 'postgres',
+        psql_user => 'postgres',
+        command   => 'select 1',
+        require   => Class['postgresql::server'],
+      }
+    EOS
+
+    apply_manifest(pp, :catch_failures => true)
+    apply_manifest(pp, :catch_failures => true)
+  end
+
   it 'should run some SQL when the unless query returns no rows' do
     pp = <<-EOS.unindent
       class { 'postgresql::server': }
@@ -40,4 +56,79 @@ describe 'postgresql_psql:', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osf
     apply_manifest(pp, :catch_changes => true)
   end
 
+  it 'should not run SQL when refreshed and the unless query returns rows' do
+    pp = <<-EOS.unindent
+      class { 'postgresql::server': }
+
+      notify { 'trigger': } ~>
+      postgresql_psql { 'foobar':
+        db        => 'postgres',
+        psql_user => 'postgres',
+        command   => 'invalid sql statement',
+        unless    => 'select 1 where 1=1',
+        require   => Class['postgresql::server'],
+      }
+    EOS
+
+    apply_manifest(pp, :catch_failures => true)
+    apply_manifest(pp, :catch_failures => true)
+  end
+
+  context 'with refreshonly' do
+    it 'should not run SQL when the unless query returns no rows' do
+      pp = <<-EOS.unindent
+        class { 'postgresql::server': }
+
+        postgresql_psql { 'foobar':
+          db          => 'postgres',
+          psql_user   => 'postgres',
+          command     => 'select 1',
+          unless      => 'select 1 where 1=2',
+          refreshonly => true,
+          require     => Class['postgresql::server'],
+        }
+      EOS
+
+      apply_manifest(pp, :catch_failures => true)
+      apply_manifest(pp, :catch_changes => true)
+    end
+
+    it 'should run SQL when refreshed and the unless query returns no rows' do
+      pp = <<-EOS.unindent
+        class { 'postgresql::server': }
+
+        notify { 'trigger': } ~>
+        postgresql_psql { 'foobar':
+          db          => 'postgres',
+          psql_user   => 'postgres',
+          command     => 'select 1',
+          unless      => 'select 1 where 1=2',
+          refreshonly => true,
+          require     => Class['postgresql::server'],
+        }
+      EOS
+
+      apply_manifest(pp, :catch_failures => true)
+      apply_manifest(pp, :catch_failures => true)
+    end
+
+    it 'should not run SQL when refreshed and the unless query returns rows' do
+      pp = <<-EOS.unindent
+        class { 'postgresql::server': }
+
+        notify { 'trigger': } ~>
+        postgresql_psql { 'foobar':
+          db          => 'postgres',
+          psql_user   => 'postgres',
+          command     => 'invalid sql query',
+          unless      => 'select 1 where 1=1',
+          refreshonly => true,
+          require     => Class['postgresql::server'],
+        }
+      EOS
+
+      apply_manifest(pp, :catch_failures => true)
+      apply_manifest(pp, :catch_failures => true)
+    end
+  end
 end

spec/unit/puppet/provider/postgresql_psql/ruby_spec.rb

@@ -66,75 +66,14 @@ describe Puppet::Type.type(:postgresql_psql).provider(:ruby) do
         provider.run_sql_command("SELECT something")
       end
     end
-
   end
 
-  context("#command") do
+  context("#run_unless_sql_command") do
-    context "when unless is specified" do
+    let(:attributes) do { } end
-      [:true, :false, true, false].each do |refresh|
-        context "and refreshonly is #{refresh}" do
-          let(:attributes) { {
-            :command     => 'SELECT something',
-            :db          => 'spec_db',
-            :unless      => 'SELECT something',
-            :refreshonly => refresh
-          } }
 
-          it "does not fail when the status is successful" do
+    it "calls #run_sql_command with SQL" do
-            expect(provider).to receive(:run_unless_sql_command).and_return ["1 row returned", 0]
+      expect(provider).to receive(:run_sql_command).with('SELECT COUNT(*) FROM (SELECT 1) count')
-            provider.command
+      provider.run_unless_sql_command('SELECT 1')
-          end
-
-          it "returns the given command when rows are returned" do
-            expect(provider).to receive(:run_unless_sql_command).and_return ["1 row returned", 0]
-            expect(provider.command).to eq("SELECT something")
-          end
-
-          it "does not return the given command when no rows are returned" do
-            expect(provider).to receive(:run_unless_sql_command).and_return ["0 rows returned", 0]
-            expect(provider.command).to_not eq("SELECT something")
-          end
-
-          it "raises an error when the sql command fails" do
-            allow(provider).to receive(:run_unless_sql_command).and_return ["Something went wrong", 1]
-            expect { provider.command }.to raise_error(Puppet::Error, /Something went wrong/)
-          end
-        end
-      end
-    end
-
-    context "when unless is not specified" do
-      context "and refreshonly is true" do
-        let(:attributes) do {
-          :command     => 'SELECT something',
-          :db          => 'spec_db',
-          :refreshonly => :true
-        } end
-        it "does not run unless sql command" do
-          expect(provider).to_not receive(:run_unless_sql_command)
-          provider.command
-        end
-
-        it "returns the given command do disable sync" do
-          expect(provider.command).to eq("SELECT something")
-        end
-      end
-
-      context "and refreshonly is false" do
-        let(:attributes) do {
-          :command     => 'SELECT something',
-          :db          => 'spec_db',
-          :refreshonly => :false
-        } end
-        it "does not run unless sql command" do
-          expect(provider).to_not receive(:run_unless_sql_command)
-          provider.command
-        end
-
-        it "does not return the command so as to enable sync" do
-          expect(provider.command).to_not eq("SELECT something")
-        end
-      end
     end
   end
 end

spec/unit/puppet/type/postgresql_psql_spec.rb

@@ -44,49 +44,182 @@ describe Puppet::Type.type(:postgresql_psql), :unless => Puppet.features.microso
       its([:psql_user]) { should eq("postgres") }
       its([:psql_group]) { should eq("postgres") }
       its([:cwd]) { should eq("/tmp") }
-      its(:refreshonly?) { should be_false }
+      its(:refreshonly?) { should be_falsey }
     end
   end
 
-  describe "#refreshonly" do
+  describe "#command" do
-    [true, :true].each do |refreshonly|
+    let(:attributes) do {:command => 'SELECT stuff'} end
-      context "=> #{refreshonly.inspect}" do
+
-        let(:attributes) do { :refreshonly => refreshonly } end
+    it "will have the value :notrun if the command should execute" do
-        it "has a value of true" do
+      expect(subject).to receive(:should_run_sql).and_return(true)
-          expect(subject.refreshonly?).to be_true
+      expect(subject.property(:command).retrieve).to eq(:notrun)
     end
-        it "will not enforce command on sync because refresh() will be called" do
+
-          expect(subject.provider).to_not receive(:command=)
+    it "will be the 'should' value if the command should not execute" do
+      expect(subject).to receive(:should_run_sql).and_return(false)
+      expect(subject.property(:command).retrieve).to eq('SELECT stuff')
+    end
+
+    it "will call provider#run_sql_command on sync" do
+      expect(subject.provider).to receive(:run_sql_command).with('SELECT stuff').and_return(["done", 0])
       subject.property(:command).sync
     end
   end
+
+  describe "#unless" do
+    let(:attributes) do {:unless => 'SELECT something'} end
+
+    describe "#matches" do
+      it "does not fail when the status is successful" do
+        expect(subject.provider).to receive(:run_unless_sql_command).and_return ["1 row returned", 0]
+        subject.parameter(:unless).matches('SELECT something')
+      end
+
+      it "returns true when rows are returned" do
+        expect(subject.provider).to receive(:run_unless_sql_command).and_return ["1 row returned", 0]
+        expect(subject.parameter(:unless).matches('SELECT something')).to be_truthy
+      end
+
+      it "returns false when no rows are returned" do
+        expect(subject.provider).to receive(:run_unless_sql_command).and_return ["0 rows returned", 0]
+        expect(subject.parameter(:unless).matches('SELECT something')).to be_falsey
+      end
+
+      it "raises an error when the sql command fails" do
+        allow(subject.provider).to receive(:run_unless_sql_command).and_return ["Something went wrong", 1]
+        expect {
+          subject.parameter(:unless).matches('SELECT something')
+        }.to raise_error(Puppet::Error, /Something went wrong/)
+      end
+    end
+  end
+
+  describe "#should_run_sql" do
+    context "without 'unless'" do
+      [true, :true].each do |refreshonly|
+        context "refreshonly => #{refreshonly.inspect}" do
+          let(:attributes) do {
+            :refreshonly => refreshonly,
+          } end
+
+          context "not refreshing" do
+            it { expect(subject.should_run_sql).to be_falsey }
+          end
+
+          context "refreshing" do
+            it { expect(subject.should_run_sql(true)).to be_truthy }
+          end
+        end
       end
 
       [false, :false].each do |refreshonly|
-      context "=> #{refreshonly.inspect}" do
+        context "refreshonly => #{refreshonly.inspect}" do
-        let(:attributes) do { :refreshonly => refreshonly } end
+          let(:attributes) do {
-        it "has a value of false" do
+            :refreshonly => refreshonly,
-          expect(subject.refreshonly?).to be_false
+          } end
+
+          context "not refreshing" do
+            it { expect(subject.should_run_sql).to be_truthy }
           end
-        it "will enforce command on sync because refresh() will not be called" do
+
-          expect(subject.provider).to receive(:command=)
+          context "refreshing" do
-          subject.property(:command).sync
+            it { expect(subject.should_run_sql(true)).to be_truthy }
           end
         end
       end
     end
 
-  ## If we refresh the resource, the command should always be run regardless of
+    context "with matching 'unless'" do
-  ## refreshonly
+      before { expect(subject.parameter(:unless)).to receive(:matches).with('SELECT something').and_return(true) }
-  describe "when responding to refresh" do
+
-    [true, :true, false, :false].each do |refreshonly|
+      [true, :true].each do |refreshonly|
-      context "with refreshonly => #{refreshonly.inspect}" do
+        context "refreshonly => #{refreshonly.inspect}" do
-        let(:attributes) do { :refreshonly => refreshonly } end
+          let(:attributes) do {
-        it "will enforce command on sync" do
+            :refreshonly => refreshonly,
-          expect(subject.provider).to receive(:command=)
+            :unless => 'SELECT something',
+          } end
+
+          context "not refreshing" do
+            it { expect(subject.should_run_sql).to be_falsey }
+          end
+
+          context "refreshing" do
+            it { expect(subject.should_run_sql(true)).to be_falsey }
+          end
+        end
+      end
+
+      [false, :false].each do |refreshonly|
+        context "refreshonly => #{refreshonly.inspect}" do
+          let(:attributes) do {
+            :refreshonly => refreshonly,
+            :unless => 'SELECT something',
+          } end
+
+          context "not refreshing" do
+            it { expect(subject.should_run_sql).to be_falsey }
+          end
+
+          context "refreshing" do
+            it { expect(subject.should_run_sql(true)).to be_falsey }
+          end
+        end
+      end
+    end
+
+    context "when not matching 'unless'" do
+      before { expect(subject.parameter(:unless)).to receive(:matches).with('SELECT something').and_return(false) }
+
+      [true, :true].each do |refreshonly|
+        context "refreshonly => #{refreshonly.inspect}" do
+          let(:attributes) do {
+            :refreshonly => refreshonly,
+            :unless => 'SELECT something',
+          } end
+
+          context "not refreshing" do
+            it { expect(subject.should_run_sql).to be_falsey }
+          end
+
+          context "refreshing" do
+            it { expect(subject.should_run_sql(true)).to be_truthy }
+          end
+        end
+      end
+
+      [false, :false].each do |refreshonly|
+        context "refreshonly => #{refreshonly.inspect}" do
+          let(:attributes) do {
+            :refreshonly => refreshonly,
+            :unless => 'SELECT something',
+          } end
+
+          context "not refreshing" do
+            it { expect(subject.should_run_sql).to be_truthy }
+          end
+
+          context "refreshing" do
+            it { expect(subject.should_run_sql(true)).to be_truthy }
+          end
+        end
+      end
+    end
+  end
+
+  describe "#refresh" do
+    let(:attributes) do {} end
+
+    it "syncs command property when command should run" do
+      expect(subject).to receive(:should_run_sql).with(true).and_return(true)
+      expect(subject.property(:command)).to receive(:sync)
+      subject.refresh
+    end
+
+    it "does not sync command property when command should not run" do
+      expect(subject).to receive(:should_run_sql).with(true).and_return(false)
+      expect(subject.property(:command)).not_to receive(:sync)
       subject.refresh
     end
   end
-    end
-  end
 end