Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

accepts_nested_attributes_for does not rollback parent correctly if child fails to save #5651

Closed
nolamesa opened this Issue · 6 comments

5 participants

@nolamesa

In a new rails project (rails 3.2.2) do the following:

rails g scaffold User name
rails g model Profile skill user_id:integer

Then in the User we add:

 class User < ActiveRecord::Base
   has_one :profile, inverse_of: :user
   accepts_nested_attributes_for :profile
 end

and in the Profile:

class Profile < ActiveRecord::Base
  belongs_to :user, inverse_of: :profile
  accepts_nested_attributes_for :user
  before_create :do_not_create
  def do_not_create
    false
  end
 end

Now in the users controller we add:

def new
  @user = User.new
  @user.build_profile
end

and in the app/views/users/_form.html.erb:

...
<%= f.text_field :name %>
<%= f.fields_for :profile do |g|%>
<%=  g.text_field :skill%>
<% end %>

Now when we submit the user form to create a new record the action is not completed because of the after create callback. The problem is that the @user does not get flushed but has an id (as if it was created) and now the submit button of the form tries to update the record which fails because there is no record.

@joeytheman

This is because you are using the before_create callback which occurs after validations. See http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html .

@nolamesa

Yes indeed. But when before_create returns false the action is cancelled. See http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html#label-Canceling+callbacks

@artem-mindrov

I've got a very similar (but a bit more complicated) issue:

class Tool < ActiveRecord::Base
  has_many :loss_ratios, :inverse_of => :tool, :dependent => :destroy, :order => "loss_ratios.starts_at DESC"

  validates_associated :loss_ratios
  accepts_nested_attributes_for :loss_ratios, :allow_destroy => true

  attr_accessible :loss_ratios_attributes

end

class LossRatio < ActiveRecord::Base
  belongs_to :tool

  validates :rate, :starts_at, :tool, :presence => true
  validates_uniqueness_of :starts_at, :scope => :tool_id
  validates_numericality_of :rate
  validates_inclusion_of :rate, :in => (0..1)

  %w{ create save }.each do |name|
    %W{ #{name} #{name}! }.each do |method|
      define_method(method) do |*args|
        begin
          super(*args)
        rescue ActiveRecord::RecordNotUnique
          self.errors.add(:starts_at, I18n.t('activerecord.errors.models.loss_ratio.attributes.starts_at.not_unique'))
          false
        end
      end
    end
  end

end

So when a user tries to create a new tool and mistakenly specifies two entries with duplicate dates, here's what happens:

  • validation of starts_at does not catch anything, since the Tool record, being new, does not have an id, so there's no scope to validate against
  • when the tool is saved, RecordNotUnique is raised and caught by the code in LossRatio (that's why I can't get rid of this hack), save returns false, so the controller seemingly does everything right (the user is taken back to the new tool form with errors dsplayed)
  • BUT, when the user fills in the form with correct values, I get a routing error (no route to /tools with PUT), because the tool instance is treated as an existing record! (having an id and new_record? set to false). This also leads to weird issues like tool.save returning true on subsequent attempts (although there's no such record in the DB).

I've found an old ticket here https://rails.lighthouseapp.com/projects/8994/tickets/1948-transaction-block-sets-model-id-to-non-existent-row, but seems like it was auto-closed without a resolution.

@artem-mindrov

I've tried to wrap Tool#save in a 'rollback state' block:

  %w{ create save }.each do |name|
    %W{ #{name} #{name}! }.each do |method|
      define_method(method) do |*args|
        self.rollback_active_record_state! do
          super(*args)
        end
      end
    end
  end

but that didn't help either...

@schneems
Collaborator

3 months old with no discussion. I'm not seeing "parents being rolled back if children do not save" being a supported feature or desired behavior in the docs: http://api.rubyonrails.org/classes/ActiveRecord/NestedAttributes/ClassMethods.html . The last section Validating the presence of a parent model implies that the parent and the child are not implicitly bound. If you have documentation that I missed or other information pointing to this being desired functionality let me know.

I believe you can build the behavior you want by adding in a validation to the User model that checks to see if it has a child.

If you believe this behavior to be desired most of the time, you can always submit a pull request with the desired feature and make a case for it. Unless I missed some docs or another discussion with rails core, we should close this issue.

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.