Obey validation rules on: :save condition #10287

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Member

neerajdotname commented Apr 21, 2013

class User < ActiveRecord::Base
validates :name, presence: true, on: :save
end

In at the above case User.new.valid? returns true. Also while
updating user can set name value to nil and no validation
error is found while saving the record.

Active Model understands validations in either :create or
:update context. Otherwise it is best to not set any context.
And that's what this fix is attempting to do.

It removes context if context is :save.

fixes #10248

Neeraj Singh Obey validation rules on: :save condition
    class User < ActiveRecord::Base
      validates :name, presence: true, on: :save
    end

In at the above case `User.new.valid?` returns true. Also while
updating user can set `name` value to `nil` and no validation
error is found while saving the record.

Active Model understands validations in either `:create` or
`:update` context. Otherwise it is best to not set any context.
And that's what this fix is attempting to do.

It removes context if context is `:save`.

fixes #10248
ac8c164
Member

josevalim commented Apr 21, 2013

I would argue that it is not part of Active Model responsibility to set a default context. It doesn't know anything about save and friends. For example, a search model is never saved, but it still can be valid? or not.

Member

neerajdotname commented Apr 21, 2013

@josevalim makes sense that Active Model should not know about save and friends.

Do you have any suggestion in how to go about fixing this issue ?

Active Model sets validation callbacks with condition :save. However Active Record only works with :create and :update.

One solution could be that Active Model provides a hook for Active Record to override :context value before it is used in the condition . Not sure how complex it would be implementation wise.

Member

josevalim commented Apr 21, 2013

Deleting is not an option either because then it would be triggered on all contexts, no? I can only think of making validation_context an array which AR will always set to [:save, :create] or [:save, :update] and which we check the inclusion against.

Member

neerajdotname commented Apr 21, 2013

Yes by deleting it would execute for all contexts. But that's the goal here . on: :save does not mean anything in Active Record. It's as good as no option for :on.

Now that I think about it, fixing by deleting feels like a hack.

As you suggested there should be a way to say in case of :save do both :create and :update. Will study the code a bit more. Thanks for the feedback @josevalim .

@neerajdotname, Check out https://github.com/rails/rails/blob/master/activerecord/lib/active_record/validations.rb#L68

It is not getting context at all and that is what causing the problem. So even if you have custom method for on like

validates :name, :presence => true, :on => :foo

Then also just Bar.foo will not trigger the callback.

Bar.foo.valid?(:foo) will trigger it.

@josevalim Setting if validation_context is set to array like [:save, :create] or [:save, :update], then :save callbacks will be executed with both :create and :update right?

Member

neerajdotname commented Apr 22, 2013

Here is what I did. Changed Active Model validations to support more than one context.

    def valid?(context = nil)
      current_context = validation_context
      errors.clear
      ctxs = Array.wrap(context)

      ctxs.each do |ctx|
        self.validation_context = ctx
        run_validations!
      end
    ensure
      self.validation_context = current_context
    end

And then chagned Active Record validations to pass two contexts.

      #context ||= (new_record? ? :create : :update)
      context ||= (new_record? ? [:create, :save] : [:update, :save])

The issue now is that for User.new.valid? the error messages are happening twice because validates :name, presence: true does not put any if condition and thus both :create and :save fires the validation logic.

One way to fix that would be to assume that validates :name, presence: true is same as validates :name, presence: true, on: :save . Now all the validation logics are bound to save explicitly. But that makes Active Modal aware of :save .

Contributor

aditya-kapoor commented Apr 22, 2013

Now, if I only change the ActiveRecord valid? method and let the validations run as it is, then also I would get my problem solved...
Instead of following line,
output = super(context)
if I write
output = super(context) && super(:save)
then all the cases would be covered and the validations would still run for on: :save option also.

If the context is :update or :create then the second call to super(:save) would not run because the value returned by the first super function in both cases is true.
If no option is given then also we would be getting true because if first super call is false then the second call for :save would definitely be true.
Now, if I give the on: :save option, then the first super call would be false but the second call to super would be true because we have explicitly created a context for save. In this way, we can control the behavior of validations without changing a line of code for validations...

@neerajdotname Any update?

Member

neerajdotname commented Apr 24, 2013

@prathamesh-sonpatki As I mentioned in my previous comment the fix requires structural change. First the design of the fix needs to be blessed by someone from core team before I can move ahead.

@neerajdotname How about changing this in ActiveRecord than ActiveModel?

rails/activerecord/lib/active_record/validations.rb

def valid?(context = nil)
  context ||= new_record? ? [:create, :save] : [:update, :save]
  output = Array.wrap(context).map { |c| super(c) }.inject(:&)
  errors.empty? && output
end
Member

neerajdotname commented Apr 24, 2013

@prathamesh-sonpatki In that case you are invoking the validation twice. Not good from performance point of view and running it twice could have side-effects too.

Contributor

aditya-kapoor commented Apr 24, 2013

@neerajdotname Any word on my solution????

Member

neerajdotname commented Apr 24, 2013

@aditya-kapoor if you do

output = super(context) && super(:save)

then you are hardcoding code for :save . In the case of :update output will not have any value.

Contributor

aditya-kapoor commented Apr 30, 2013

The code is running correctly at my end...Sorry for late reply..

Member

neerajdotname commented May 9, 2013

Closing this PR as the solution proposed by me is a no go .

Contributor

aditya-kapoor commented May 9, 2013

Can you explain a bit more on this???

Contributor

aditya-kapoor commented May 9, 2013

Why are you closing your request??

Member

neerajdotname commented May 9, 2013

@aditya-kapoor Please refer to the very first comment on this thread by @josevalim to see why this PR is a no go.
#10287 (comment)

PRs which are no go should be closed to keep the pull requests list more manageable .

Contributor

aditya-kapoor commented May 9, 2013

So this is a known issue in Rails and we cannot do anything about it???

@take take added a commit to take/rails that referenced this pull request Aug 4, 2013

@take take Fix active_record_validations.md document, `:save` for `on:` validati…
…on helper was never available

According to the guide, ":save" value for the "on:" validation helper
was available like below

    validates :name, presence: true, on: :save

but this was never available according to the implementation of the
valid? method, which is below

    # Runs all the validations within the specified context. Returns
+true+ if
    # no errors are found, +false+ otherwise.
    #
    # If the argument is +false+ (default is +nil+), the context is set
to <tt>:create</tt> if
    # <tt>new_record?</tt> is +true+, and to <tt>:update</tt> if it is
not.
    #
    # Validations with no <tt>:on</tt> option will run no matter the
context. Validations with
    # some <tt>:on</tt> option will only run in the specified context.
    def valid?(context = nil)
      context ||= (new_record? ? :create : :update)
      output = super(context)
      errors.empty? && output
    end

So the documentation was always wrong since the PR proposed by
@neerajdotname  ( #10287 ) was rejected.
740f778
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment