Is this a bug? Records deleted when assigned to belonging association

Before I post this as a bug in the issue tracker I want to get some other opinions. Is this a bug?

class Request < ApplicationRecord
  has_many :items, class: 'RequestItem', dependent: :delete_all
end

class RequestItem < ApplicationRecord
  belongs_to :request
end

Request.new(id: 1, items: [RequestItem.new(start_at: Time.now, end_at: Time.now + 36000)])
# also
r = Request.new(id: 1)
r.items = [RequestItem.new(start_at: Time.now, end_at: Time.now + 36000)]

Where a request with ID 1 exists in the database, any existing request items are deleted. However when I remove dependent: :delete_all in the Request model. It tries to update the items in place (which I my case fails due to database contraints).

I’m using activerecord 5.2.4.4, and activerecord-sqlserver-adapter 5.2.0 on Ruby 2.7.2

Thanks in advance for your help!

UPDATE: I was just able to replicate the same behavior using sqlite. Here’s a test that replicates the behavior: https://github.com/delonnewman/destroy_all/blob/master/test/models/request_test.rb

I can’t quite speak to whether there’s specifically a bug, but here’s some thoughts after looking at your repo, and playing with it in console.

  • delete_all goes straight to SQL, without hitting callbacks. This might be why there’s no errors squawking from the following notes.
  • request.items = [...] appears to work by replacing the collection in the DB. i.e., the moment you call r.items = [...], it executes the sql to replace the collection (delete and insert based on r.id).
  • If you were to follow up your test with one more move: r.save! after calling #new you’d find that this is unable to be inserted, because AR is treating it as a new record, as opposed to an existing record.

Finally, I get to the main observation: It looks like the collection setter (request.items = [...]) attempts to replace the collection immediately in the DB if the record was already persisted. It succeeds at deleting the existing records based on id, but it never follows up with inserting the records. This seems to be related to calling .new and passing in an existing id, rather than querying for the existing record. The AR instance sees this as a new record, but it in fact conflicts with an existing record so it cannot be saved. Thus, it cannot get an ID in order to populate the associations.

The more usual pattern would be to use find_or_initialize_by, letting AR know that you are pulling up an already-persisted record if such an ID already exists.

Here’s the bit of Rails code in question: rails/collection_association.rb at 291a3d2ef29a3842d1156ada7526f4ee60dd2b59 · rails/rails · GitHub

You can see that there’s a chunk that checks the ar attribute: #new_record? which is true from a #new, but wouldn’t be from a #find, etc.

Here’s some things to try in a console – specifically watch the DB logs for each variation:

r = Request.new(request_type: :wfh, status: :draft)
# => In memory record, not persisted
r.items = [RequestItem.new(start_at: Time.now, end_at: Time.now + 36000)]
# => Still in memory, nothing is persisted. Now, table this observation until the last example.

# Let's actually get a persisted record using the same example code:
r = Request.create(request_type: :wfh, status: :draft)
# => `r` is a new AR object, and it's persisted in DB due to #create

r.items = [RequestItem.new(start_at: Time.now, end_at: Time.now + 36000)]
# => Instantly triggers a db insert to add the collection, even without calling `r.save`.

# Just to check how this looks, let's replace that again before we lose this object:
#
r.items = [RequestItem.new(start_at: Time.now, end_at: Time.now + 36000)]
# => delete -> insert
#
# Notice that `=` triggered a full replacement: delete and insert.


# Now let's reconsistute this record:
#
# First, let's watch the case where AR knows a record was pulled from the DB:
# You can use `#find` or any other method that tells AR to query the db
r = Request.find(r.id)
# => loads from db. r is a new AR object, and it came from persistence. `new_record?` is false.

r.items = [RequestItem.new(start_at: Time.now, end_at: Time.now + 36000)]
# => delete -> insert
#
# Still works as expected.


# Finally, the reported case as above:
r = Request.new(id: r.id)
# No db query, only in memory.
r.items = []
# => Attempts to replace immediately, but curiously, triggers a delete, and nothing else.
#
# This is likely due to the fact that it doesn't have the persisted db PK from r,
#   since r itself cound't be saved, so it can't persist the associated records yet either.
#   I think using `delete_all` rather than `destroy` is helping "cover up"
#   this issue, but isn't necessary the issue itself.

r.save!
# => RecordNotUnique rollback

In conclusion, I’m not sure that I agree it’s a bug with delete_all, but instead the unexpected usage of instantiating a record with an id which can be a bit dangerous since that id may already exist in the DB. That said, I think it’s unexpected behavior that the delete statement triggers and doesn’t get rolled back when in fact the record couldn’t be saved; however, since it’s a #new_record?, it is expected that the collection wouldn’t be written until the #new_record? is persisted, since it needs the id.

1 Like

Thank you for the food for thought @ttilberg.

I share your doubt that it’s a bug (or should be viewed as such, anyway). I would feel more confident that it’s not a bug if it deleted and inserted the dependent records, but in this case it doesn’t. Hence, the dilemma.

Thanks again.