This project is archived and is in readonly mode.

#2298 ✓stale
Peter Kieltyka

ActiveRecord::Rollback does not nil all id attributes

Reported by Peter Kieltyka | March 19th, 2009 @ 05:13 PM | in 3.x

A bug exists in ActiveRecord's transaction mechanism that does not clear the id attribute for nested objects.

I've attached a simple Rails 2.3.2 project that illustrates the problem. Please view the signup_controller.rb for comments on where the bug occurs and what happens.

The application simulates a nested model form of an order where the hierarchy is:

order order.account order.account.contact_info order.account.user

To replicate the bug, run the app, go to /signup/new and fill in the details, but input "breakme" for the "User" field. This will raise an exception in the User model, simulating an error thereby forcing the transaction to be rolled back. The records are properly rolled back in the database, however @order.account.contact_info.id will contain the record number, resulting the view to be rendered with an additional hidden field for the id: order[account_attributes][contact_info_attributes][id]

When submitted again, and @order = Order.new(params[:order]) is executed, it will result in a nil.new_record? error because the "id" attribute is specified for contact_info.

Also note though, that I wasn't able to replicate this bug with a single nested case such as account, account.contact_info and account.user, the order nested model was necessary to reproduce the bug.

Solution: Make sure that ALL id attributes are restored to the original state after a rollback (ie. nil).

Comments and changes to this ticket

  • porras

    porras May 18th, 2009 @ 08:57 PM

    • Tag changed from 2.3.2, activerecord, nested, rollback to 2.3.2, activerecord, nested, rollback, testcase

    I also hit this in our app. I uploaded a failing test which reproduces it.

  • calavera

    calavera May 21st, 2009 @ 11:08 AM

    Following the clues I just arrived to this method into active_record/transactions.rb:

    # Reset id and @new_record if the transaction rolls back.
        def rollback_active_record_state!
          id_present = has_attribute?(self.class.primary_key)
          previous_id = id
          previous_new_record = new_record?
          yield
        rescue Exception
          @new_record = previous_new_record
          if id_present
            self.id = previous_id
          else
            @attributes.delete(self.class.primary_key)
            @attributes_cache.delete(self.class.primary_key)
          end
          raise
        end
    

    When I executes the test that @porras uploaded the condition has_attribute?(self.class.primary_key) is false so the method doesn't modify the id, but I don't understand why it's returning false.

    I'm just thinking there is a worst problem in this method. What happen when a user doesn't follow the convention and changes the name of the primary key attribute? I suppose these lines will raise an error because the id method won't be defined:

    line 206: previous_id = id
    line 212: self.id = previous_id
    
  • porras

    porras May 21st, 2009 @ 11:59 AM

    @calavera it's not related. AR wraps each save (and its validations and callbacks) in a transaction (which is nested if you already have one opened). That's the transaction being rolled back there, not the big one. That is: if you have a transaction with 5 saves and the last one fails, the 5 saves are rolled back, but only the object whose save failed gets its id fixed.

    Anyway, your issue with hardcoding 'id' is completely true.

  • calavera

    calavera May 21st, 2009 @ 12:20 PM

    ok. I'm opening a new ticket so XD

  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from 2.x to 3.x
  • rails

    rails February 26th, 2011 @ 12:00 AM

    • Tag changed from 2.3.2, activerecord, nested, rollback, testcase to 232, activerecord, nested, rollback, testcase
    • State changed from “new” to “open”
    • Importance changed from “” to “”

    This issue has been automatically marked as stale because it has not been commented on for at least three months.

    The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

    Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

  • rails

    rails February 26th, 2011 @ 12:00 AM

    • State changed from “open” to “stale”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages