OK, the title gives this one away, and the most obvious bug is that there wasn’t a unit test that would have caught this, but here’s a variant of the Ruby on Rails production code, designed to copy invoices from one year to another:
invoice = Invoice.find(params[:invoice_id]) new_invoice = invoice.clone new_invoice.edition = new_edition new_invoice.summary = "Keep your ad the same" new_invoice.amount_paid = 0 new_invoice.created_at = Time.zone.now skus = EditionPrice.for_edition(@edition) invoice.items.each do |item| new_item = InvoiceItem.new new_item.description = item.description new_item.sku = item.sku new_item.amount = item.amount new_invoice.items << new_item end new_invoice.save
This worked great in the Ruby on Rails 2.3.5 days, but after upgrading to 3.2 as part of a Heroku migration, the call would get killed by the 30 second timeout rule. At first I thought there were simply too many ads to clone (there are also image assets in the production code, which would take time to replicate in Amazon S3) but then I reloaded the referring page.
And saw 27,000 new line items in the first invoice.
So here’s the deal: in the Rails 3.1 release notes for ActiveRecord, there’s this little blurb:
- ActiveRecord::Base#dup and ActiveRecord::Base#clone semantics have changed to closer match normal Ruby dup and clone semantics.
- Calling ActiveRecord::Base#clone will result in a shallow copy of the record, including copying the frozen state. No callbacks will be called.
- Calling ActiveRecord::Base#dup will duplicate the record, including calling after initialize hooks. Frozen state will not be copied, and all associations will be cleared. A duped record will return true for new_record?, have a nil id field, and is saveable.
And what will this mean, those of you who lack unit tests to automatically detect breaking changes like this one? Well, the clone call will still copy the record, but including the id field. Which means in the code above, we’re adding line items not to the new invoice, but the exact same invoice, since they have the same id. And we’re iterating through that invoice’s line items. And copying them. To a list that keeps growing until the app is killed.
So to summarize: if you want to make a quick copy of an ActiveRecord that’s in addition to the one that’s already in the table, use dup, not clone, as of Ruby on Rails 3.1. And add tests and read release notes…