CODEWITHSUNDEEP
ruby-on-railsvalidationactiverecord
René Jahn
René Jahn

Reputation: 1213

Rails ActiveRecord custom validator returns false but model.save is accepted without rollback

i have a model which defines size ranges like 100m² to 200m². I wrote a validator:

class SizeRange < ActiveRecord::Base
    validate :non_overlapping

    def non_overlapping
        lower_in_range = SizeRange.where(lower_bound: lower_bound..upper_bound)
        upper_in_range = SizeRange.where(upper_bound: lower_bound..upper_bound)
        if lower_in_range.present?
            errors.add(:lower_bound, 'blablabla')
        end
        if upper_in_range.present?
            errors.add(:upper_bound, 'blablabla')
        end
    end
end

My guess was, that when I try to save a new model which lower or upper bound appears to be in the range of an other SizeRange instance the validator would mark the model as invalid and the save action would be aborted. What really happened is that my model got saved and assigned an id, but when I call model.valid? it returns false (So my validator seems to do what it should).

Is there anything I could have done wrong, or did I not understand how the validators work? Can I force the validator to abort the save action?

Another question: Is there any way to enforce a constraint like that through database constraints? I think I would prefer a solution on database side.

Thanks for your help!

Upvotes: 0

Views: 1732

Answers (4)

NorseGaud
NorseGaud

Reputation: 771

You should be using begin and rescue:

class SizeRange < ActiveRecord::Base
  validate :non_overlapping

  private
    def non_overlapping
      lower_in_range = SizeRange.where(lower_bound: lower_bound..upper_bound)
      upper_in_range = SizeRange.where(upper_bound: lower_bound..upper_bound)

      begin
        raise("Lower Bound Met!") if lower_in_range.present?
      rescue => ex
        errors.add(:lower_bound, "#{ex.message} (#{ex.class})")
      end
      begin
        raise("Lower Bound Met!") if upper_in_range.present?
      rescue => ex
        errors.add(:upper_bound, "#{ex.message} (#{ex.class})")
      end
    end
end

Upvotes: 0

Ren&#233; Jahn
Ren&#233; Jahn

Reputation: 1213

Turns out the problem was my not well-formed validator function.

The case I checked an which led to confusion:

  • SizeRange from 200 to 400 already in the database
  • Creating a new one between 200 and 400 like SizeRange.new({:lower_bound=>250, :upper_bound=>350})

So I expected that to be invalid (model.valid? -> false) but it was, of course, valid. So model.save did no rollback and AFTER that I tested on model.valid? which would NOW return false, as the newly saved instance would violate the constraint, because it was tested against itself.

So there were two problems:

  • model.valid? would also test against the same instance if it already had an id
  • the validator would not validate what I thought it would

So I ended up rewriting my validator function:

def non_overlapping
    sr = id.present? ? SizeRange.where.not(id: id) : SizeRange

    ranges = sr.all
    lower_overlappings = ranges.map { |r| lower_bound.between?(r.lower_bound, r.upper_bound)}
    upper_overlappings = ranges.map { |r| upper_bound.between?(r.lower_bound, r.upper_bound)}

    if lower_overlappings.any?
        errors.add(:lower_bound, 'lower bla bla')
    end
    if upper_overlappings.any?
        errors.add(:lower_bound, 'upper bla bla')
    end
end

The first line handles the first problem and the rest handles the intended validation.

Thanks for your help and sorry for the confusion.

Upvotes: 0

Razvan Pavel
Razvan Pavel

Reputation: 351

You should return false after each errors.add to cancel the record saving

Upvotes: 0

Raj
Raj

Reputation: 22926

model.save

would be accepted silently and return false. It will not throw any Exception.

You should call:

model.save!

to fail with validations

Upvotes: 1

Related Questions