Skip to content
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

Fix validations on child record when record parent has validate: false #18612

Conversation

@eileencodes
Copy link
Member

eileencodes commented Jan 20, 2015

Fixes #17621. This 5 year old (or older) issue causes validations to fire when a parent record has validate: false option and a child record is saved. It's not the responsibility of the model to validate an associated object unless the object was created or modified by the parent.

Also cleaned up tests related to validations
assert_nothing_raised is not benefiting us in these tests
Corrected spelling of "respects"
It's better to use assert_not_operator over assert !r.valid

Originally @tenderlove and I had put the definition of should_validate? in ActiveModel but I found later that persisted?, changed? and marked_for_destruction? are not defined in ActiveModel so those tests were failing. I moved that code completely into AR. If there is another suggestion to a better place for this method I'm open to suggestions.

P.S. I'm on vacation this week so I might be slow to respond if you have questions ☀️ 😄

@sgrif
sgrif reviewed Jan 20, 2015
View changes
activerecord/lib/active_record/validations/uniqueness.rb Outdated
@@ -28,6 +29,10 @@ def validate_each(record, attribute, value)
end
end

def should_validate?(record)
!record.persisted? || record.changed? || record.marked_for_destruction?

This comment has been minimized.

Copy link
@sgrif

sgrif Jan 20, 2015

Contributor

Should this be extracted somewhere, since it's re-used three times?

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Jan 20, 2015

We might want to benchmark this, since calling record.changed? repeatedly will probably cause a major regression.

@tenderlove

This comment has been minimized.

Copy link
Member

tenderlove commented Jan 20, 2015

It looks like changed? is just checking to see if a hash is empty. Is it doing something more? Also this change only impacts specific validations, I have a hard time believing it would cause a major speed regression in an app.

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Jan 20, 2015

The hash calculates in-place changes.

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Jan 20, 2015

Maybe "major" is a bit far, but I would expect it to move the needle for Model#save if it has multiple presence validations.

@eileencodes

This comment has been minimized.

Copy link
Member Author

eileencodes commented Jan 21, 2015

Thanks for the feedback @sgrif - I'll consolidate the code and run benchmarks when I'm back in town this weekend.

@eileencodes eileencodes force-pushed the eileencodes:fix-validates-on-associated-record-if-parent-is-validate-false branch Jan 25, 2015
@eileencodes

This comment has been minimized.

Copy link
Member Author

eileencodes commented Jan 25, 2015

@sgrif - I moved the should_validate? definition into ActiveModel but am not calling it from there so the tests pass. That was the only shared file between the validations - if this is undesirable the only other place I can think to put it would be it's own file. 😸

Edit: I deleted what I previously wrote here because I realized right away that I wasn't thinking when I wrote it 😰 See below for the benchmarks.

@eileencodes

This comment has been minimized.

Copy link
Member Author

eileencodes commented Jan 25, 2015

Ugg I do have vacation brain - forgot I had the #save! on the test scripts 😁

Here are the benchmarks master vs my branch. There is a little bit of a slow down.

Calculating -------------------------------------
master
                        46.000  i/100ms
-------------------------------------------------
master
                        466.578  (± 5.1%) i/s -      2.346k
Calculating -------------------------------------
with should_validate?
                        44.000  i/100ms
-------------------------------------------------
with should_validate?
                        448.450  (± 5.6%) i/s -      2.244k
@eileencodes eileencodes force-pushed the eileencodes:fix-validates-on-associated-record-if-parent-is-validate-false branch Jan 30, 2015
Fixes #17621. This 5 year old (or older) issue causes validations to fire
when a parent record has `validate: false` option and a child record is
saved. It's not the responsibility of the model to validate an
associated object unless the object was created or modified by the
parent.

Clean up tests related to validations

`assert_nothing_raised` is not benefiting us in these tests
Corrected spelling of "respects"
It's better to use `assert_not_operator` over `assert !r.valid`
@eileencodes eileencodes force-pushed the eileencodes:fix-validates-on-associated-record-if-parent-is-validate-false branch to 27aa4dd Feb 2, 2015
@bgott

This comment has been minimized.

Copy link

bgott commented Feb 2, 2015

Yo @tenderlove I ran the benchmarks against master and this branch. The standard deviation shows that there isn't much difference between the two branches.

Calculating -------------------------------------
master
                        39.000  i/100ms
-------------------------------------------------
master
                        407.243  (± 4.4%) i/s -      2.067k


Calculating -------------------------------------
with should_validate?
                        40.000  i/100ms
-------------------------------------------------
with should_validate?
                        415.600  (± 3.4%) i/s -      2.080k
tenderlove added a commit that referenced this pull request Feb 2, 2015
…d-record-if-parent-is-validate-false

Fix validations on child record when record parent has validate: false
@tenderlove tenderlove merged commit 83c1973 into rails:master Feb 2, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Feb 2, 2015

Thanks for running the benchmarks, guys. I just want to confirm, since nobody posted the script they used, this was benchmarking modifying existing records, not creating new ones, right?

@eileencodes

This comment has been minimized.

Copy link
Member Author

eileencodes commented Feb 2, 2015

@sgrif I ran both and the results for the difference were the same

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Feb 2, 2015

👍

On Sun, Feb 1, 2015, 7:24 PM Eileen M. Uchitelle notifications@github.com
wrote:

@sgrif https://github.com/sgrif I ran both and the results for the
difference were the same


Reply to this email directly or view it on GitHub
#18612 (comment).

topic.save!

assert_equal topic.replies.size, 1
assert reply.valid?

This comment has been minimized.

Copy link
@pacoguzman

pacoguzman Feb 7, 2015

Contributor

I have one question about this. Does this mean the reply will be valid from now on? I mean I thought calling valid? must execute the validations again

This comment has been minimized.

Copy link
@hundredwatt

hundredwatt Feb 7, 2015

@pacoguzman It thought this was odd too, took a few minutes to grok! If you look at the new should_validate? method:

   def should_validate?(record) # :nodoc:
      !record.persisted? || record.changed? || record.marked_for_destruction?
    end

When assert reply.valid?, the record is [ ] persisted, [ ] un-changed, [ ] not marked for destruction, so the uniqueness validation isn't performed. Before this PR was merged, the validation would have been run, but now it isn't.

This comment has been minimized.

Copy link
@hundredwatt

hundredwatt Feb 7, 2015

As an example, if you change a different attribute, say title, then the validations will be run:

  def test_does_not_validate_uniqueness_of_if_parent_record_is_validate_false
    ...
    assert_equal topic.replies.size, 1
    assert reply.valid?
    reply.title = "Change An Attribute"
    assert reply.valid? #=> Fails!
    ...
  end

This comment has been minimized.

Copy link
@pacoguzman

pacoguzman Feb 7, 2015

Contributor

Understood thanks for the response. It's a big change for me. Then some
validations will run and others don't when the record has no changes.
Anyway I always add a condition on the uniqueness validations to avoid
extra queries
On Feb 7, 2015 5:59 PM, "Jason Nochlin" notifications@github.com wrote:

In activerecord/test/cases/validations/uniqueness_validation_test.rb
#18612 (comment):

@@ -386,4 +386,21 @@ def test_validate_uniqueness_on_empty_relation
topic = TopicWithUniqEvent.new
assert topic.valid?
end
+

  • def test_does_not_validate_uniqueness_of_if_parent_record_is_validate_false
  • Reply.validates_uniqueness_of(:content)
  • Reply.create!(content: "Topic Title")
  • reply = Reply.new(content: "Topic Title")
  • reply.save!(validate: false)
  • assert reply.persisted?
  • topic = Topic.new(reply_ids: [reply.id])
  • topic.save!
  • assert_equal topic.replies.size, 1
  • assert reply.valid?

As an example, if you change a different attribute, say title, then the
validations will be run:

def test_does_not_validate_uniqueness_of_if_parent_record_is_validate_false
...
assert_equal topic.replies.size, 1
assert reply.valid?
reply.title = "Change An Attribute"
assert reply.valid? #=> Fails!
...
end


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/18612/files#r24290679.

@eileencodes eileencodes deleted the eileencodes:fix-validates-on-associated-record-if-parent-is-validate-false branch Mar 6, 2015
eileencodes added a commit that referenced this pull request Feb 23, 2016
In order to fix issue #17621 we added a check to validations that
determined if a record should be validated. Based on the existing tests
and behavior we wanted we determined the best way to do that was by
checking if `!record.peristed? || record.changed? || record.marked_for_destruction?`

This change didn't make it into a release until now. When #23790 was
opened we realized that `valid?` and `invalid?` were broken and did not
work on persisted records because of the `!record.persisted?`.

While there is still a bug that #17621 brought up, this change was too
drastic and should not be a RC blocker. I will work on fixing this so
that we don't break `valid?` but also aren't validating parent records
through child records if that parent record is validate false. This
change removes the code changes to validate and the corresponding tests.
It adds tests for two of the bugs found since Rails 5 beta2 release.

Fixes #17621
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.