Validate multiple contexts on `valid?` and `invalid?` at once #21069

Merged
merged 1 commit into from Sep 7, 2015

Conversation

Projects
None yet
5 participants
@dmitry
Contributor

dmitry commented Jul 30, 2015

Lets say we have cascade of validations, when we should check more and more validations, after each step. As an example: precreate context should check everything in a create, but less 2 attributes (real word example).

Currently it's only possible to check validations one by one. This patch provides possibility to do all the checks at once.

Example:

class Person
  include ActiveModel::Validations

  attr_reader :name, :title
  validates_presence_of :name, on: :create
  validates_presence_of :title, on: :update
end

person = Person.new
person.valid?([:create, :update])    # => true
person.errors.messages               # => {:name=>["can't be blank"], :title=>["can't be blank"]}
Validate multiple contexts on `valid?` and `invalid?` at once.
Example:

```ruby
class Person
  include ActiveModel::Validations

  attr_reader :name, :title
  validates_presence_of :name, on: :create
  validates_presence_of :title, on: :update
end

person = Person.new
person.valid?([:create, :update])    # => true
person.errors.messages               # => {:name=>["can't be blank"], :title=>["can't be blank"]}
```
@meinac

This comment has been minimized.

Show comment
Hide comment
@meinac

meinac Jul 31, 2015

Contributor

Hi @dmitry. I couldn't realise the real world example. Can you give a code example to show as a developer in which case I need to check the validity of an object in multiple contexts.

Contributor

meinac commented Jul 31, 2015

Hi @dmitry. I couldn't realise the real world example. Can you give a code example to show as a developer in which case I need to check the validity of an object in multiple contexts.

@dmitry

This comment has been minimized.

Show comment
Hide comment
@dmitry

dmitry Jul 31, 2015

Contributor

@meinac hey, thank you for the critics!

For example, imagine you have a multiple steps before you could create an order. You would like to add more and more validations on each steps, so each step will validate all the previous step validations + current one.

Another example, actually what I've got recently (but we are still on rails 3.2.x, hopefully will update soon or later), when you want to validate model for almost everything, except presence of the user, in that case when user isn't logged in yet or not created his profile, which will be created through the nested attributes.

I can think of other possible cases, which can be solved a other way around, but in some cases it's really better to have this feature to make everything easier.

Contributor

dmitry commented Jul 31, 2015

@meinac hey, thank you for the critics!

For example, imagine you have a multiple steps before you could create an order. You would like to add more and more validations on each steps, so each step will validate all the previous step validations + current one.

Another example, actually what I've got recently (but we are still on rails 3.2.x, hopefully will update soon or later), when you want to validate model for almost everything, except presence of the user, in that case when user isn't logged in yet or not created his profile, which will be created through the nested attributes.

I can think of other possible cases, which can be solved a other way around, but in some cases it's really better to have this feature to make everything easier.

@gaurish

This comment has been minimized.

Show comment
Hide comment
@gaurish

gaurish Aug 15, 2015

Contributor

👍 This would enable to remove conditional validations. Much needed feature.

Contributor

gaurish commented Aug 15, 2015

👍 This would enable to remove conditional validations. Much needed feature.

@dmitry

This comment has been minimized.

Show comment
Hide comment
@dmitry

dmitry Aug 15, 2015

Contributor

Through it's 5x slower, it's really helpful feature.

require 'benchmark'

a = [:a, :b]
b = :b
n = 10000000
Benchmark.bm do |x|
  x.report { n.times do !(Array(a) & Array(b)).empty? end }
  x.report { n.times do Array(a).include?(b) end }
end
       user     system      total        real
   5.220000   0.010000   5.230000 (  5.223742)
   1.070000   0.000000   1.070000 (  1.066095)
Contributor

dmitry commented Aug 15, 2015

Through it's 5x slower, it's really helpful feature.

require 'benchmark'

a = [:a, :b]
b = :b
n = 10000000
Benchmark.bm do |x|
  x.report { n.times do !(Array(a) & Array(b)).empty? end }
  x.report { n.times do Array(a).include?(b) end }
end
       user     system      total        real
   5.220000   0.010000   5.230000 (  5.223742)
   1.070000   0.000000   1.070000 (  1.066095)
@dmitry

This comment has been minimized.

Show comment
Hide comment
@dmitry

dmitry Sep 1, 2015

Contributor

I would be glad to rebase this PR if anyone from the core agrees to merge it into the master.

Contributor

dmitry commented Sep 1, 2015

I would be glad to rebase this PR if anyone from the core agrees to merge it into the master.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 1, 2015

Member

How much does valid? being slowed down impact save (which calls valid?)?

I don't think this change is worth it over just doing valid?(:context1) && valid?(:context2).

Member

kaspth commented Sep 1, 2015

How much does valid? being slowed down impact save (which calls valid?)?

I don't think this change is worth it over just doing valid?(:context1) && valid?(:context2).

@dmitry

This comment has been minimized.

Show comment
Hide comment
@dmitry

dmitry Sep 1, 2015

Contributor

@kaspth I've answered on the both questions above :)

With valid?(:context1) && valid?(:context2) you will get an errors of only :context2, because each time valid? resets an errors. Yes, it can be workaround with accumulator, but it's really not nice to maintain + errors state isn't inside the model record, but outside of it.

Slowdown is 5x times, but can be improved a little bit with is_a?(String) || is_a?(Symbol) check.

Contributor

dmitry commented Sep 1, 2015

@kaspth I've answered on the both questions above :)

With valid?(:context1) && valid?(:context2) you will get an errors of only :context2, because each time valid? resets an errors. Yes, it can be workaround with accumulator, but it's really not nice to maintain + errors state isn't inside the model record, but outside of it.

Slowdown is 5x times, but can be improved a little bit with is_a?(String) || is_a?(Symbol) check.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 1, 2015

Member

Duh, sorry! 😄

Slowdown is 5x times,

Is that for save? I don't think we can't accept this if save is slowed down 5x.

Member

kaspth commented Sep 1, 2015

Duh, sorry! 😄

Slowdown is 5x times,

Is that for save? I don't think we can't accept this if save is slowed down 5x.

@dmitry

This comment has been minimized.

Show comment
Hide comment
@dmitry

dmitry Sep 1, 2015

Contributor

Not save but this code:

  x.report { n.times do !(Array(a) & Array(b)).empty? end }
  x.report { n.times do Array(a).include?(b) end }

I believe it will slowdown save by 1.000001. How can I benchmark this part of the code easily, without spending too much time on it?

PS. Looks like it's much easier to close this PR, because such cases might be moved to the if blocks, when required, if you are really think Array#include? much faster than Array#& (in reality it changes only that).

Contributor

dmitry commented Sep 1, 2015

Not save but this code:

  x.report { n.times do !(Array(a) & Array(b)).empty? end }
  x.report { n.times do Array(a).include?(b) end }

I believe it will slowdown save by 1.000001. How can I benchmark this part of the code easily, without spending too much time on it?

PS. Looks like it's much easier to close this PR, because such cases might be moved to the if blocks, when required, if you are really think Array#include? much faster than Array#& (in reality it changes only that).

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 2, 2015

Member

I don't think this slowdown will change anything. This feature is something that I wanted to implement some times so I think it is worth.

Member

rafaelfranca commented Sep 2, 2015

I don't think this slowdown will change anything. This feature is something that I wanted to implement some times so I think it is worth.

+ end
+
+ person = Person.new
+ person.valid?([:create, :update]) # => true

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 2, 2015

Member

How many changes would be necessary to make person.valid?(:create, :update) work?

@rafaelfranca

rafaelfranca Sep 2, 2015

Member

How many changes would be necessary to make person.valid?(:create, :update) work?

This comment has been minimized.

@dmitry

dmitry Sep 2, 2015

Contributor

Not many changes should be made, but the interface of the valid?(context) should be changed to valid?(*context) in activemodel/activerecord implementations (and everywhere where is valid? is used should be changed as well, like save, invalid? and so on). It can brake some gems.

In the beginning I went by this path, but then after changing a lot of places in the code, I found that using an array actually can fit better to whole conception, because it makes sense to pass an array instead of passing an array through a splat; passing a simple array without splat will look more clean in many cases.

@dmitry

dmitry Sep 2, 2015

Contributor

Not many changes should be made, but the interface of the valid?(context) should be changed to valid?(*context) in activemodel/activerecord implementations (and everywhere where is valid? is used should be changed as well, like save, invalid? and so on). It can brake some gems.

In the beginning I went by this path, but then after changing a lot of places in the code, I found that using an array actually can fit better to whole conception, because it makes sense to pass an array instead of passing an array through a splat; passing a simple array without splat will look more clean in many cases.

@rafaelfranca rafaelfranca self-assigned this Sep 2, 2015

@rafaelfranca rafaelfranca merged commit 86e3b04 into rails:master Sep 7, 2015

rafaelfranca added a commit that referenced this pull request Sep 7, 2015

Merge pull request #21069 from dmitry/feature/validate-multiple-conte…
…xts-at-once

Validate multiple contexts on `valid?` and `invalid?` at once

rafaelfranca added a commit that referenced this pull request Sep 7, 2015

Revert "Merge pull request #21069 from dmitry/feature/validate-multip…
…le-contexts-at-once"

This reverts commit 51dd258, reversing
changes made to ecb4e4b.

This broke Active Record tests
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 7, 2015

Member

It broke Active Record tests. I revert it. Could you take a look on these failures and open a new PR?

Member

rafaelfranca commented Sep 7, 2015

It broke Active Record tests. I revert it. Could you take a look on these failures and open a new PR?

@dmitry

This comment has been minimized.

Show comment
Hide comment
@dmitry

dmitry Sep 7, 2015

Contributor

@rafaelfranca fixed at #21535

PS. Broken because some code left when I tried to make it to work as splat params.

Contributor

dmitry commented Sep 7, 2015

@rafaelfranca fixed at #21535

PS. Broken because some code left when I tried to make it to work as splat params.

@dmitry dmitry deleted the dmitry:feature/validate-multiple-contexts-at-once branch Sep 7, 2015

tenderlove added a commit to tenderlove/rails that referenced this pull request Sep 8, 2015

Merge branch 'master' into rack2
* master: (112 commits)
  Use released mysql2
  Fixed Time conversion example for UTC time zone [ci skip]
  Define `SchemaStatements#tables` as interface
  Add view tests for MySQL
  Replace AR with ActiveRecord to make it more readable [ci skip]
  💣
  Memoized reflections accessor
  Reduce allocation in `resolve_column_aliases`.
  Make `config.force_ssl` less dangerous to try and easier to disable
  Support mysql2 0.4.0, first release with prepared statements support
  .gitignore: Ignore .ruby-version in any subdir
  modify to pass the correct argument to the test runner from rake
  Validate multiple contexts on `valid?` and `invalid?` at once.
  Use Hash[] instead of Hash#dup in resolve_column_aliases
  Merge pull request #21534 from Vratislav/clarify-custom-config-guide
  Revert "Merge pull request #21069 from dmitry/feature/validate-multiple-contexts-at-once"
  Correct query for PostgreSQL 8.2
  Typo fix [ci skip]
  Allow global migrations_path configuration with using value from database_tasks instead of Migrator
  changelog, minor formatting changes.
  ...

Conflicts:
	actionpack/lib/action_dispatch/http/request.rb
	actionpack/lib/action_dispatch/middleware/cookies.rb
	actionpack/lib/action_dispatch/middleware/flash.rb
	actionpack/lib/action_dispatch/request/session.rb
	actionpack/lib/action_dispatch/request/utils.rb

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Sep 12, 2015

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Sep 12, 2015

yhirano55 added a commit to yhirano55/rails that referenced this pull request Dec 19, 2017

Backport `Fix to working before/after validation callbacks on multipl…
…e contexts.`

Though the validation have supported multiple context since #21069,
its callbacks don't support multiple context currently.

So I regarded this as the bug and fixed.

Example:

```ruby
class Dog
  include ActiveModel::Validations
  include ActiveModel::Validations::Callbacks

  attr_accessor :history

  def initialize
    @history = []
  end

  before_validation :set_before_validation_on_a, on: :a
  before_validation :set_before_validation_on_b, on: :b
  after_validation :set_after_validation_on_a, on: :a
  after_validation :set_after_validation_on_b, on: :b

  def set_before_validation_on_a; history << "before_validation on a"; end
  def set_before_validation_on_b; history << "before_validation on b"; end
  def set_after_validation_on_a;  history << "after_validation on a" ; end
  def set_after_validation_on_b;  history << "after_validation on b" ; end
end
```

Before:

```
d = Dog.new
d.valid?([:a, :b])
d.history # []
```

After:

```
d = Dog.new
d.valid?([:a, :b])
d.history # ["before_validation on a", "before_validation on b", "after_validation on a", "after_validation on b"]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment