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

Validates_presence_of associated object marked for destruction #6812

Closed
zephyr-dev opened this Issue Jun 21, 2012 · 16 comments

Comments

Projects
None yet
5 participants
@zephyr-dev

zephyr-dev commented Jun 21, 2012

We've come across an issue with validates_presence_of on an associated object that is marked for destruction. We have model Man, that has one Face, with validates_presence_of :face. If the face is marked for destruction, however, then the man still passes validations. This then allows the face to be destroyed, without invalidating the parent or adding an error until reload. The same principle applies to a has_many relationship.

Here are a couple failing tests (within ActiveRecord):

require "cases/helper"
require 'models/topic'
require 'models/reply'
require 'models/man'
require 'models/face'

class AssociationValidationTest < ActiveRecord::TestCase
  def test_validates_presence_of_has_one_marked_for_destruction
    Man.validates_presence_of(:face)
    m = Man.new
    f = Face.new
    m.face = f
    assert m.valid?

    f.mark_for_destruction
    assert m.invalid?
  end

  def test_validates_presence_of_has_many_marked_for_destruction
    Topic.validates_presence_of(:replies)
    t = Topic.new
    t.replies << [r1 = Reply.new, r2 = Reply.new]
    assert t.valid?

    r1.mark_for_destruction
    assert t.valid?

    r2.mark_for_destruction
    assert t.invalid?
  end
end

Possible solutions for this include:

  1. Change ActiveModel::Validations:PresenceValidator to add errors on blank, and on all objects in relation marked for destruction
  2. Within ActiveRecord::Relation, change the definition of blank? to return true if all objects in the relation are marked for destruction
  3. In ActiveModel::Errors, change add_on_blank to check blank? and for at least on non-marked for destruction object in the relation (if the attribute is a relation).

We think that option 1 is the best fix, but we wanted to get some feedback from a wider audience about the preferred solution.

Thanks!

Nick, Evan and Brent

@acapilleri

This comment has been minimized.

Contributor

acapilleri commented Jun 21, 2012

@zephyr-dev you have to add save, you can look here

@zephyr-dev

This comment has been minimized.

zephyr-dev commented Jun 21, 2012

@acapilleri Where would we put the save to help in this situation? The issue is that the save is actually happening when it shouldn't be.

@acapilleri

This comment has been minimized.

Contributor

acapilleri commented Jun 21, 2012

the comment of mark_for_destructionsays that ....

# This does _not_ actually_not_ destroy the record instantly,
# rather child record will be destroyed when <tt>parent.save</tt> is called.
 def test_validates_presence_of_has_one_marked_for_destruction
    Man.validates_presence_of(:face)
    m = Man.new
    f = Face.new
    m.face = f
    assert m.valid?

    f.mark_for_destruction
    m.save
    assert m.invalid?
  end

let me know if it not fail, thanks!

@zephyr-dev

This comment has been minimized.

zephyr-dev commented Jun 21, 2012

You are misunderstanding our issue. The root of our issue is that the Man object remains valid when it's Face object is marked_for_destruction. This seems like the incorrect behavior.

@acapilleri

This comment has been minimized.

Contributor

acapilleri commented Jun 21, 2012

it's correct behavior, because marked_for_destruction is activated during the save transaction and not before .

@zephyr-dev

This comment has been minimized.

zephyr-dev commented Jun 21, 2012

But that's exactly the problem. The save should fail and the destroy should roll back because it puts Man into an invalid state.

@acapilleri

This comment has been minimized.

Contributor

acapilleri commented Jun 21, 2012

ok I have tested this and it seems has a strange behaviour but maybe is better a test like the following:

def test_validates_presence_of_has_one_marked_for_destruction
    Man.validates_presence_of(:face)
    m = Man.new
    f = Face.new
    m.face = f
    assert m.valid?

    f.mark_for_destruction
    assert !m.save
  end
@zephyr-dev

This comment has been minimized.

zephyr-dev commented Jun 21, 2012

Sorry, we think that testing the validation is the correct approach. Testing the save is testing a side effect.

@carlosantoniodasilva

This comment has been minimized.

Member

carlosantoniodasilva commented Jun 22, 2012

I can see a possible problem with the proposed changes in Active Model, 1 and 3, mainly because validations or errors are completely unaware of Active Record associations, they only test if the given object is actually present or not. This means that they have no context to ask about marked_for_destruction?, as they're completely unrelated to each other.

Option 2 may seem ok, with the exception that there's a great chance that it'd break a lot of existing functionality, so I'd vote against it at first thought.

I can't say if Active Record should have a special kind of validation for that, but I can think of a simple working solution:

class Man
  has_one :face
  validates_presence_of :face_not_marked_for_destruction

  private

  def face_not_marked_for_destruction
    face if face && !face.marked_for_destruction?
  end
end

class Topic
  has_many :replies
  validates_presence_of :replies_not_marked_for_destruction

  private

  def replies_not_marked_for_destruction
    replies.reject(&:marked_for_destruction?)
  end
end

In any case, I'm summoning some more feedback @josevalim @rafaelfranca @drogus.

@drogus

This comment has been minimized.

Member

drogus commented Jun 22, 2012

The 2) approach seems controversial, at least, since objects technically are there, they will be removed on next save.

For me the simplest approach would be to subclass PresenceValidator, add check for marked_for_destruction? and use it in AR instead of Active Model's version. Am I missing something important here? :)

@zephyr-dev

This comment has been minimized.

zephyr-dev commented Jun 22, 2012

@carlosantoniodasilva You make a good point about ActiveModel not knowing about ActiveRecord associations, and about option 2 breaking a lot of things.

@drogus We really like the idea of subclassing PresenceValidator in ActiveRecord. Does this option seem viable/palatable to the masses?

@carlosantoniodasilva

This comment has been minimized.

Member

carlosantoniodasilva commented Jun 22, 2012

@drogus no, I agree with the approach, it's basically encapsulating the code I've sent in a specific PresenceValidator for AR, to work with associations. Do you think it should be part of AR?

@drogus

This comment has been minimized.

Member

drogus commented Jun 22, 2012

@carlosantoniodasilva sure, I would put something like that in AR. AR relies on validatiors, so it can extend them as well.

@zephyr-dev

This comment has been minimized.

zephyr-dev commented Jun 22, 2012

We've implemented this in this pull request: #6827

Thanks for your idea!

@carlosantoniodasilva

This comment has been minimized.

Member

carlosantoniodasilva commented Aug 21, 2012

Pull request was merged, so I'm closing the related issue. Thanks!

sgerrand pushed a commit to sgerrand/rails that referenced this issue Nov 2, 2013

Brent Wheeldon & Nick Monje Ben Moss
AR has a subclass of AM:PresenceValidator.
This allows us to mark the parent object as invalid if all associated objects
in a presence validated association are marked for destruction.

See: rails#6812
@monfresh

This comment has been minimized.

monfresh commented Apr 15, 2014

Maybe I'm not understanding this issue correctly, but to me it means that when you validate the presence of an associated object, it should not be possible to destroy that associated object. If that's the case, then it seems like this is still an issue in Rails 4.0.4.

If I have these 2 models associated like so:

class Location < ActiveRecord::Base
  has_one :office
  validates_presence_of :office
end

class Office < ActiveRecord::Base
  belongs_to :location
end

Now let's say a location loc with an office exists in the DB, if I try to destroy the location's office, like so in the console:

loc.office.destroy

I would expect an error to be raised because a Location must have an Office.
Instead, the office is destroyed and the location is left in an invalid state without an office.

What am I missing here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment