Fix preventing validation hooks when save! is used

[jwkoelewijn]

Currently it is possible to get in a state in which the resource
is dirty and the stack is not empty, resulting in a call to valid?,
even when save! is used.

Added a spec causing the valid? call to be run when save! is used,
to work around it, I've added an extra check to the save_self
chainable in dm-validations.rb that checks for the execute_hooks
parameter, circumventing a call to valid?

Note: I don't think this is an actual fix, because I think it has
to do with the way the validation_context_stack is used,
especially the check for emptyness in the save_self chainable.

[xaviershay]

I don't really understand the context stack so well. The commit seems ok, anyone else have any thoughts? @dkubb @snusu

[xaviershay]

Have just posted to the mailing list for some more feedback on this.

[probablykabari]

@xaviershay - dm-validations doesn't check execute_hooks. So in a relationship, when #save_parents gets called the validations will still get run on the parent (and children).

Actually that may not be the problem, I just assumed that because the following works without this patch:

    describe "#save! on a saved, dirty resource" do
      before :all do
        @resource = DataMapper::Validations::Fixtures::Barcode.new
        @resource.code = 'a' * 10
        @resource.save
      end

      it 'should not call valid?' do
        @resource.valid_hook_call_count.should == 1
        @resource.code = 'b' * 10
        @resource.save!
        @resource.valid_hook_call_count.should == 1
      end
    end

The patch uses a has 1 relationship, so #save_parents definitely gets called even when #save! is used.

[jwkoelewijn]

@RipTheJacker I don't think it is a problem that #save_parents gets called, because execute_hooks is passed to #save_parents as well. If you look at the implementation of #save_parents:

def save_parents(execute_hooks)
  run_once(true) do
    parent_relationships.map do |relationship|
      parent = relationship.get(self)
      if parent.__send__(:save_parents, execute_hooks) && parent.__send__(:save_self, execute_hooks)
        relationship.set(self, parent)  # set the FK values
      end
    end.all?
  end
end

Especially the parent.send(:save_self, execute_hooks) part, which will end up in my patched #save_self again, so parents won't be validated either

[probablykabari]

@jwkoelewijn - Correct, which is why when you call BillCorrection.save, #save_parents gets called with execute_hooks = true

describe '#save!' do
  it "should not call valid when save! is used" do
    bill = DataMapper::Validations::Fixtures::Bill.new()
    bill.save.should be_true

    bill_correction = DataMapper::Validations::Fixtures::BillCorrection.build_from(bill)

    # bill_correction.save will call save! on @bill
    bill_correction.save.should be_true # this also calls #save_parents with execute_hooks = true

    bill.valid_hook_call_count.should == 1
  end
end

So really, this test is incorrect in a way, because #valid? should get called on "bill" twice.

[probablykabari]

@jwkoelewijn - This will make your test pass, though I'm not 100% if it solves your issue:

      class BillCorrection
        include DataMapper::Resource

        property :id,                     DataMapper::Property::Serial

        belongs_to :original_bill,   :model => 'Bill'
        belongs_to :correction_bill, :model => 'Bill'

        def self.build_from(original, intermediary = nil)
          correction = Bill.new
          correction.original = self.new(:original_bill => original, :correction_bill => correction )
          correction
        end

        def save_parents(execute_hooks)
          if saved?
            original_bill.has_correction = true
            # suppose we want to bypass validation for some reason
            original_result = original_bill.save!
            super(false)
          else
            super
          end
        end     
      end

[jwkoelewijn]

@RipTheJacker The problem is not in the fact that I cannot get it to work, but more in the fact that when I use #save! to bypass validations and all callbacks, validations are still run in this case. This is not the behavior one would expect from the semantics of #save!, no matter the context in which it is called.
For your proposal, I will try it to see if it solves my problems for now, if the tests pass it probably will :)

[jwkoelewijn]

@RipTheJacker Hmmm...tried your proposed solution, it did not work however :(

[probablykabari]

@jwkoelewijn - Check the comment I added next to "bill_correction.save.should be_true" in my earlier post :) The validations get run when #save is called (in BillCorrection). So for your code that overloads the #save method

def save
  if save_result = super
    original_bill.has_correction = true
    # suppose we want to bypass validation for some reason
    original_result = original_bill.save!
  end
  save_result && original_result
end

That call to #super calls #save_parents which in turn calls #save_self on the bill. All of that gets run with execute_hooks as true, because it is coming from bill_correction.save. Your call to original_bill.save! happens after all of that.

The code for BIllCorrection I posted definitely makes your test pass though. After I applied your patch I made these changes: https://gist.github.com/968528

[jwkoelewijn]

@RipTheJacker after looking again at your proposed solution, I figured out that I could use the #save_parents like you said, and indeed, it does work. However, maybe my proposed spec isn't complete enough, in the real-world scenario from which I extracted the spec you solution did not work without any adaptation (solution was actually to rename the #save method to #save_parents :)).
Tomorrow I'll probably try to make the spec more extensive to reflect my problem, because I'm still not sure about the behavior...

[emmanuel]

@jwkoelewijn—I did some work on dm-validations (#32), part of which involved removing the override of #save_self and replacing it with a before :save, :validate_or_halt hook.

I don't think that the existing test suite provides coverage of the scenario addressed in this pull request, and I would love any feedback if the approach I've taken does address the situation discussed here.

I could simply merge in the spec example captured here and verify that it works on my fork, but I don't know if the spec example attached to this pull request completely captures the failure described. If you have the time, please let me know if that test case sufficiently exemplifies the described failure.