New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: validation callbacks on multiple context #31483

Merged
merged 1 commit into from Dec 19, 2017

Conversation

Projects
None yet
6 participants
@yhirano55
Contributor

yhirano55 commented Dec 16, 2017

Summary

I found a bug that validation callbacks don't fire on multiple context.
So I've fixed it.

Example:

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"]
@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Dec 16, 2017

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

rails-bot commented Dec 16, 2017

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

Show outdated Hide outdated activemodel/CHANGELOG.md Outdated

@yhirano55 yhirano55 changed the title from Fix validation callbacks on multiple context to Bugfix: validation callbacks on multiple context Dec 19, 2017

@yhirano55

This comment has been minimized.

Show comment
Hide comment
@yhirano55

yhirano55 Dec 19, 2017

Contributor

Though the validation have supported multiple context since #21069 (2 years ago), its callbacks don't support multiple context currently. So I regarded as the bug. If it's possible, I want to make backport for v5.1 which I'm using.

Contributor

yhirano55 commented Dec 19, 2017

Though the validation have supported multiple context since #21069 (2 years ago), its callbacks don't support multiple context currently. So I regarded as the bug. If it's possible, I want to make backport for v5.1 which I'm using.

@kamipo

kamipo approved these changes Dec 19, 2017

Some minor comments, but overall looks good.

Show outdated Hide outdated activemodel/CHANGELOG.md Outdated
Show outdated Hide outdated activemodel/CHANGELOG.md Outdated
Fix validation callbacks on multiple context
I found a bug that validation callbacks don't fire on multiple context.
So I've fixed it.

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"]
```

@kamipo kamipo merged commit eb6bacc into rails:master Dec 19, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yhirano55 yhirano55 deleted the yhirano55:fix_validation_callbacks_on_multiple_context_in_active_model branch Dec 19, 2017

kamipo added a commit that referenced this pull request Dec 19, 2017

@plentz

This comment has been minimized.

Show comment
Hide comment
@plentz

plentz Feb 28, 2018

Contributor

I think that this one caused a regression @ 5.1.5

validates :status, inclusion: { in: ["active", "disabled"] }, on: :update, if: ->(t) { t.saved_change_to_status? }

This stopped working.

Contributor

plentz commented Feb 28, 2018

I think that this one caused a regression @ 5.1.5

validates :status, inclusion: { in: ["active", "disabled"] }, on: :update, if: ->(t) { t.saved_change_to_status? }

This stopped working.

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Mar 1, 2018

Member

Can you provide an executable script that demonstrates your issue? You can use this template https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb for active record bugs. Thanks!

Member

kamipo commented Mar 1, 2018

Can you provide an executable script that demonstrates your issue? You can use this template https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb for active record bugs. Thanks!

@plentz

This comment has been minimized.

Show comment
Hide comment
@plentz

plentz Mar 6, 2018

Contributor

@kamipo here it go cc @pmargreff

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", "5.1.4" #works
  #gem "rails", "5.1.5" #fail
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :name
    t.integer :status
  end

end

class Post < ActiveRecord::Base
  enum status: { inactive: 0, active: 1 }
  validates :status, inclusion: { in: ["active"] }, on: :update, if: ->(t) { t.saved_change_to_status? }
  after_create :fail_after_create
  def fail_after_create
    self.update!(name: "yolo again")
  end
end

class BugTest < Minitest::Test
  def test_validation
    post = Post.create(status: "active", name: "yolo")
    assert_equal false, post.update(status: "inactive")
  end
end
Contributor

plentz commented Mar 6, 2018

@kamipo here it go cc @pmargreff

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", "5.1.4" #works
  #gem "rails", "5.1.5" #fail
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :name
    t.integer :status
  end

end

class Post < ActiveRecord::Base
  enum status: { inactive: 0, active: 1 }
  validates :status, inclusion: { in: ["active"] }, on: :update, if: ->(t) { t.saved_change_to_status? }
  after_create :fail_after_create
  def fail_after_create
    self.update!(name: "yolo again")
  end
end

class BugTest < Minitest::Test
  def test_validation
    post = Post.create(status: "active", name: "yolo")
    assert_equal false, post.update(status: "inactive")
  end
end
@plentz

This comment has been minimized.

Show comment
Hide comment
@plentz

plentz Mar 6, 2018

Contributor

I've created #32180 to better track it.

Contributor

plentz commented Mar 6, 2018

I've created #32180 to better track it.

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Mar 18, 2018

Fix "Ruby on Rails 5.2 Release Notes" [ci skip]
- Remove
  ```
  *   Namespace error pages' CSS selectors to stop the styles from bleeding
      into other pages when using Turbolinks.
      ([Pull Request](rails#28814))
  ```
  since it was backported to
  `5-1-stable` by 50d5baf
  and
  `5-0-stable` by d1c4a39.

- Remove
  ```
  *   Allow irb options to be passed from `rails console` command.
      ([Pull Request](rails#29010))
  ```
  since it was backported to
  `5-1-stable` by e91b483.

- Remove
  ```
  *   Load environment file in `dbconsole` command.
      ([Pull Request](rails#29725))
  ```
  since it was backported to
  `5-1-stable` by 7f93428.

- Remove
  ```
  *   Gemfile for new apps: upgrade redis-rb from ~> 3.0 to 4.0.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.

- Remove
  ```
  *   Fix minitest rails plugin.
      The custom reporters are added only if needed.
      This will fix conflicts with others plugins.
      ([Commit](rails@ac99916))
  ```
  since it was backported to
  `5-1-stable` by caa7695.

- Remove
  ```
  *   Add support for compatibility with redis-rb gem for 4.0 version.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.

- Remove
  ```
  *   Add `action_controller_api` and `action_controller_base` load hooks to be
      called in `ActiveSupport.on_load`.
      ([Pull Request](rails#28402))
  ```
  since it was backported to
  `5-1-stable` by b9a5fd7.

- Remove
  ```
  *   `driven_by` now registers poltergeist and capybara-webkit.
      ([Pull Request](rails#29315))
  ```
  since it was backported to
  `5-1-stable` by c5dd451.

- Remove
  ```
  *   Fallback `ActionController::Parameters#to_s` to `Hash#to_s`.
      ([Pull Request](rails#29630))
  ```
  since it was backported to
  `5-1-stable` by c1014e4
  and
  `5-0-stable` by 0e71fc3.

- Remove
  ```
  *   Make `take_failed_screenshot` work within engine.
      ([Pull Request](rails#30421))
  ```
  since it was backported to
  `5-1-stable` by 595a231.

- Remove
  ```
  *   Fix optimized url helpers when using relative url root.
      ([Pull Request](rails#31261))
  ```
  since it was backported to
  `5-1-stable` by e9b7795.

- Remove
  ```
  *   Update `distance_of_time_in_words` helper to display better error messages
      for bad input.
      ([Pull Request](rails#20701))
  ```
  since it was backported to
  `5-1-stable` by 2c97fbf.

- Remove
  ```
  *   Generate field ids in `collection_check_boxes` and
      `collection_radio_buttons`.
      ([Pull Request](rails#29412))
  ```
  since it was backported to
  `5-1-stable` by 2d8c10a.

- Remove
  ```
  *   Fix issues with scopes and engine on `current_page?` method.
      ([Pull Request](rails#29503))
  ```
  since it was backported to
  `5-1-stable` by 2135daf.

- Remove
  ```
  *   Bring back proc with arity of 1 in `ActionMailer::Base.default` proc
      since it was supported in Rails 5.0 but not deprecated.
      ([Pull Request](rails#30391))
  ```
  since it was backported to
  `5-1-stable` by b2bedb1.

- Remove
  ```
  *   Add type caster to `RuntimeReflection#alias_name`.
      ([Pull Request](rails#28961))
  ```
  since it was backported to
  `5-1-stable` by f644e7a.

- Remove
  ```
  *   Loading model schema from database is now thread-safe.
      ([Pull Request](rails#29216))
  ```
  since it was backported to
  `5-1-stable` by 02926cf.
  and
  `5-0-stable` by 84bcfe5

- Remove
  ```
  *   Fix destroying existing object does not work well when optimistic locking
      enabled and `locking_column` is null in the database.
      ([Pull Request](rails#28926))
  ```
  since it was backported to
  `5-1-stable` by e498052.

- Remove
  ```
  *   `ActiveRecord::Persistence#touch` does not work well
      when optimistic locking enabled and `locking_column`,
      without default value, is null in the database.
      ([Pull Request](rails#28914))
  ```
  since it was backported to
  `5-1-stable` by 1e2f63d.

- Remove
  ```
  *   Previously, when building records using a `has_many :through` association,
      if the child records were deleted before the parent was saved,
      they would still be persisted. Now, if child records are deleted
      before the parent is saved on a `has_many :through` association,
      the child records will not be persisted.
      ([Pull Request](rails#29593))
  ```
  since it was backported to
  `5-1-stable` by a22c39e.

- Remove
  ```
  *   Query cache was unavailable when entering the `ActiveRecord::Base.cache`
      block without being connected.
      ([Pull Request](rails#29609))
  ```
  since it was backported to
  `5-1-stable` by fd6c8cd
  and
  `5-0-stable` by 9f2532b.

- Remove
  ```
  *   `Relation#joins` is no longer affected by the target model's
      `current_scope`, with the exception of `unscoped`.
      ([Commit](rails@5c71000))
  ```
  since it was backported to
  `5-1-stable` by 3630d63.

- Remove
  ```
  *   Fix `unscoped(where: [columns])` removing the wrong bind values.
      ([Pull Request](rails#29780))
  ```
  since it was backported to
  `5-1-stable` by d378fcb.

- Remove
  ```
  *   When a `has_one` association is destroyed by `dependent: destroy`,
      `destroyed_by_association` will now be set to the reflection, matching the
      behaviour of `has_many` associations.
      ([Pull Request](rails#29855))
  ```
  since it was backported to
  `5-1-stable` by 8254a8b.

- Remove
  ```
  *   Fix `COUNT(DISTINCT ...)` with `ORDER BY` and `LIMIT`
      to keep the existing select list.
      ([Pull Request](rails#29848))
  ```
  since it was backported to
  `5-1-stable` by 0e8d4ed.

- Remove
  ```
  *   Ensure `sum` honors `distinct` on `has_many :through` associations.
      ([Commit](rails@566f1fd))
  ```
  since it was backported to
  `5-1-stable` by c0a1dc2.

- Remove
  ```
  *   Fix `COUNT(DISTINCT ...)` for `GROUP BY` with `ORDER BY` and `LIMIT`.
      ([Commit](rails@5668dc6))
  ```
  since it was backported to
  `5-1-stable` by 87ca68e.

- Remove
  ```
  *   MySQL: Don't lose `auto_increment: true` in the `db/schema.rb`.
      ([Commit](rails@9493d45))
  ```
  since it was backported to
  `5-1-stable` by 8b6e694.

- Remove
  ```
  *   Fix longer sequence name detection for serial columns.
      ([Pull Request](rails#28339))
  ```
  since it was backported to
  `5-1-stable` by af9c170
  and
  `5-0-stable` by 7025b1d.

- Remove
  ```
  *   Fix `bin/rails db:setup` and `bin/rails db:test:prepare` create wrong
      ar_internal_metadata's data for a test database.
      ([Pull Request](rails#30579))
  ```
  since it was backported to
  `5-1-stable` by bb67b5f
  and
  `5-0-stable` by 60437e6.

- Remove
  ```
  *   Fix conflicts `counter_cache` with `touch: true` by optimistic locking.
      ([Pull Request](rails#31405))
  ```
  since it was backported to
  `5-1-stable` by 5236dda.

- Remove
  ```
  *   Fix `count(:all)` to correctly work `distinct` with custom SELECT list.
      ([Commit](rails@c6cd9a5))
  ```
  since it was backported to
  `5-1-stable` by 6beb4de.

- Remove
  ```
  *   Fix to invoke callbacks when using `update_attribute`.
      ([Commit](rails@732aa34))
  ```
  since it was backported to
  `5-1-stable` by 6346683.

- Remove
  ```
  *   Use `count(:all)` in `HasManyAssociation#count_records` to prevent invalid
      SQL queries for association counting.
      ([Pull Request](rails#27561))
  ```
  since it was backported to
  `5-1-stable` by eef3c89.

- Remove
  ```
  *   Fix `count(:all)` with eager loading and having an order other than
      the driving table.
      ([Commit](rails@ebc09ed))
  ```
  since it was backported to
  `5-1-stable` by 6df9b69.

- Remove
  ```
  *   PostgreSQL: Allow pg-1.0 gem to be used with Active Record.
      ([Pull Request](rails#31671))
  ```
  since it was backported to
  `5-1-stable` by a9c06f6.

- Remove
  ```
  *   Fix that after commit callbacks on update does not triggered
      when optimistic locking is enabled.
      ([Commit](rails@7f9bd03))
  ```
  since it was backported to
  `5-1-stable` by aaee10e.

- Remove
  ```
  *   Fix regression in numericality validator when comparing Decimal and Float
      input values with more scale than the schema.
      ([Pull Request](rails#28584))
  ```
  since it was backported to
  `5-1-stable` by 5b1c3e5.
  Note that there was incorrect link to PR,
  original PR is rails#29249.

- Remove
  ```
  *   Fix to working before/after validation callbacks on multiple contexts.
      ([Pull Request](rails#31483))
  ```
  since it was backported to
  `5-1-stable` by 0f7046a.

- Remove
  ```
  *   Fix implicit coercion calculations with scalars and durations.
      ([Pull Request](rails#29163),
      [Pull Request](rails#29971))
  ```
  since it was backported to
  `5-1-stable` by 51ea27c,
                  4d82e2a.

- Remove
  ```
  *   Fix modulo operations involving durations.
      ([Commit](rails@a54e13b))
  ```
  since it was backported to
  `5-1-stable` by 233fa7e.

- Remove
  ```
  *   Return all mappings for a timezone identifier in `country_zones`.
      ([Commit](rails@cdce6a7))
  ```
  since it was backported to
  `5-1-stable` by 0222ebb.

- Remove
  ```
  *   Add support for compatibility with redis-rb gem for 4.0 version.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.
  Related to rails#32252.

Related to rails#32222, rails#32222 (comment).
Follow up a489cc8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment