Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

validations not called when model updating using nested attributes #7247

Closed
jarl-dk opened this Issue · 23 comments

9 participants

@jarl-dk

This is a reopen of #618 (import of https://rails.lighthouseapp.com/projects/8994/tickets/2646)

I am just upgrading my rails app to rails 3.2 and I have just discovered that the workaround suggested by Matt Jones:

    if value.reject { |v| v.marked_for_destruction? }.size < 1

Is still needed...

There are patches (for tests) that demonstrates the problem...

@exviva

Maybe this is related? I think it was released in 3.2.0.

Edit: updated link.

@jarl-dk

I dont see the relation. This bug is regarding number of associated records, not the values in the associated records. And as I have also mentioned. The problem still exists in 3.2.

Jarl

@jarl-dk

I have made a patch that demonstrates the problem, and similar another patch that demonstrates the workaround proposed by Matt Jones. You can find them here created with git format-patch -o 3-2-stable-ticket_7247 3-2-stable-ticket_7247^^ on my branch (based on 3-2-stable). The problem is also in master.

@jarl-dk

I now see that the original summary seems to be misleading, now the problem seems more to be that the destruction of a nested attribute is not within the transaction that is rolled back on the parent model (in case of failing validations)

Jarl

@steveklabnik
Collaborator

@jarl-dk if you have a patch that solves this, please open up a pull request; gists with patches will end up not being noticed.

@jarl-dk

I don't have a patch that solves the problem. My gist contains a patch that demonstrates the problem as a test (in the rails project)

@steveklabnik
Collaborator

Ah! Sorry, I misread you. Thanks! That's halfway there... I'm sure that will help whoever ends up writing a patch.

@senny
Owner

I investigated a bit:

  • The validations are being called
  • When validation occurs, the records are only marked as deleted but have not been deleted yet
  • The validations calls #length on the association, which just returns the size of the Array (including the deleted records)
  • validation passes and the record is saved and the associated object is deleted, which leaves inconsistent data

I'll write a patch but I'm not sure where we should adress the issue.

patching length to ignore entries, which are marked as deleted

This would be an easy fix but it's a hack and will almost for sure lead to other problems.

lib/active_record/associations/collection_association.rb:269

      def length
        load_target.size
      end

making the validator aware of deleted records

the validator currently only calls #length on the association so it's hard to subtract the deleted records. Also it does not seem to be the job of the validator in activemodel to account for a feature in activerecord

activemodel/lib/active_model/validations/length.rb:38

        value_length = value.respond_to?(:length) ? value.length : value.to_s.length

adding an activerecord specific validator

using a different method than #length if available

this would allow me to create a method on CollectionAssociation which returns the real length (not counting deleted records)

If someone more familiar to the activerecord and activemodel internals could give me a hint what way to go I'll write the patch.

@jarl-dk

I suggest the following people should be involved: Eloy Duran, Michael Koziarski, José Valim, Matt Jones, Adam Ingram-Goble. They have all looked at this earlier on https://rails.lighthouseapp.com/projects/8994/tickets/2646.

Personally I would lean towards your proposal to modify #length. That is also what Elan Duran has come up with. But I do like you and Elan share your worries regarding consequences other places in code. As a positive consequence it could also reveal code that actually should also use the new length (with deleted records uncounted), hence it will fix bugs that was otherwise not discovered yet :-|

@senny
Owner

@jarl-dk I think you should "@mention" said people, so that they get notified.

my primary concern to patching #length is that it will break the Array API because if you access the array, the records are still inside. This means length does no longer say how many records are in the array.

@jarl-dk

OK, I have emails the mentioned people directly... Thanks for the tip...

@jarl-dk

Philosophically: When things are deleted (marked for deletion), is it then reasonable that they are still accessible from Array API? Maybe not, so that should maybe also be "fixed". And then the (Array) API would be consistent again.

@jarl-dk

@alloy, @NZKoz , @josevalim , @adamaig: Could you please participate?

@rafaelfranca

@senny I'd add a specific length validation on ActiveRecord. Want to work on it?

@senny senny was assigned
@senny
Owner

@rafaelfranca I'll take a look at it.

@senny senny referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@matthewford

@senny is this issue not the case with all other validations? I think im seeing the same problem with presence: true on an has_one association

@jarl-dk

For the test of it I tried to merge PR #9917 into 4-0-stable branch and after resolving a few trivial merge conflicts I can verify that the PR resolves the problem. Please merge it.

@jarl-dk

A more in depth investigation: I did tried to add the extra test as mentioned in #9917 (comment) and such test fails which makes it inconsistent between the errors messages (claiming that "pets is too short (minimum is 1)") and owner.pets.size which returns 1

@jarl-dk

Celebrating 5 years anniversary of original bug report: https://rails.lighthouseapp.com/projects/8994/tickets/2646

:cake: 5 years without a fix. :cake:

@jeremy
Owner

:birthday:

@dpehrson

My workaround until this is fixed was just to use validate as such:

has_many :answers # We require a minimum of one answer and a maximum of MAX_ANSWERS = 2

accepts_nested_attributes_for :answers, allow_destroy: true

validate -> do
  answers_not_marked_for_destruction = answers.reject(&:marked_for_destruction?)
  if answers_not_marked_for_destruction.length < 1
    errors.add(:answers, :too_short, count: 1)
  elsif answers_not_marked_for_destruction.length > MAX_ANSWERS
    errors.add(:answers, :too_long, count: MAX_ANSWERS)
  end
end
@Bounga

@jarl-dk I tried your test patch on latest master and the second version passes. The first one still have a bug because on pirate update bird is deleted so pirate becomes invalid and bird is gone from the DB.

Did you expected the bird to be rollback?

@jarl-dk

@Bounga: I expect the whole test to pass. Just as it does with the workaround. With the workaround, the bird is never deleted from the database. That is how all other validations work. If the validation fails the changes are never persisted in the database. That is how it should work for length validation as well.

I have rebased the patch to current master on jarl-dk@d7446e0 and the workarond on jarl-dk@d6a384d
Both the test and the workaround on this branch: https://github.com/jarl-dk/rails/tree/master-ticket_7247

@senny senny closed this issue from a commit
@senny senny AR specific length validator to respect `marked_for_destruction`.
Closes #7247.

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/test/models/owner.rb
2b12288
@senny senny closed this in 2b12288
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.