Destroying a record with habtm association using foreign keys on association table raises a exception #402

Closed
sobrinho opened this Issue May 5, 2011 · 24 comments

Projects

None yet

7 participants

@sobrinho

Hi,

I have this case:

class Profile < ActiveRecord::Base
  has_and_belongs_to_many :roles
end

class Role < ActiveRecord::Base
  has_and_belongs_to_many :profiles
end

And the association table:

class CreateProfilesRoles < ActiveRecord::Migration
  def self.up
    create_table :profiles_roles, :id => false do |t|
      t.integer :profile_id
      t.integer :role_id
    end

    add_foreign_key :profiles_roles, :profiles
    add_foreign_key :profiles_roles, :roles
  end

  def self.down
    drop_table :profiles_roles
  end
end

When I try to destroy the profile, I receive an error from postgresql due to foreign keys on join table.

The join table is cleared after destroying the record and this should be done before (see: https://github.com/rails/rails/blob/3-0-stable/activerecord/lib/active_record/associations.rb#L1716)

If I change this after_destroy to before_destroy the issue is solved but breaks 4 tests on activerecord suite (using sqlite3 adapter).

I checked this issue on 3.0.0, 3.0.7, and 3.1.0.beta1

@josevalim
Ruby on Rails member
@sobrinho

Should I use SQL to create the foreign keys on test case or use the foreigner?

@josevalim
Ruby on Rails member

@tenderlove, wdyt?

@fcheung

There was a ticket on this in lighthouse, but it's currently inaccessible (in a thread on rails corr about this a few weeks ago, Jose had come up with a way of fixing this while preserving the semantics of before_destroy)

@josevalim
Ruby on Rails member

Yeah, I remember my patch! :) that's why I just need the test case ;)

@fcheung

I did come up with a test case but I wasn't very happy with it - I wrote a model with an after_destroy callback that checked that the association had been cleared, but it felt quite vulnerable to the sort of issues that introduced this issue. I couldn't think of anything better short of actually adding foreign keys to the rails test database

@josevalim
Ruby on Rails member

For reference, this is the Google Groups thread. @sobrinho, you can find some patches there:

http://groups.google.com/group/rubyonrails-core/browse_thread/thread/d44e0c4162ccedff/f155709fffa5f0a7

@fcheung, agreed, the after_destroy callback approach would be too brittle.

@josevalim
Ruby on Rails member

Btw, is this a regression in 3.1? If so, I may apply the patch with a brittle test like @fcheung originally provided. I don't want people to not upgrade to Rails 3.1 because of this small glitch.

@fcheung

The foreign key breakage was introduced in 3.0.5 (we spotted it in our app when we upgraded from 3.0.3 to 3.0.5), so people who are on 3-0-stable will already have this breakage

@sobrinho
@josevalim
Ruby on Rails member
@sobrinho
@tenderlove
Ruby on Rails member

@sobrinho use foreign keys. We can add conditionals if we need to around dbs that support it.

@josevalim
Ruby on Rails member

@tenderlove ❤️

@sobrinho
@tenderlove tenderlove was assigned May 6, 2011
@tenderlove
Ruby on Rails member

I'm moving this off the 3.0.8 milestone. @sobrinho, I'll schedule this for a release once we get your test case! :-D

@joevandyk

tenderlove, we're also running into this. will this get fixed in 3-0-stable?

(i heard something about only security fixes going into 3-0-stable now)

@tenderlove
Ruby on Rails member

@joevandyk yes this will get fixed in 3-0-stable. There is no plan to EOL 3.0 yet. You can help get it fixed faster by providing a test case. :-)

@sobrinho
@tomas-stefano tomas-stefano pushed a commit that referenced this issue May 31, 2011
Tomas D'Stefano Attempt to fix issue #402 #402 48d735f
@tomas-stefano

I tried to fix here and add a test case tomas-stefano/rails@48d735f
But 3 other tests failed. I'll see why in a moment, =).

@fcheung

It's not as simple as just changing the after destroy to a before destroy - the deleting needs to happen before the parent record is deleted but after any other before destroys - see jose's post to rails-core (linked to above)

@tomas-stefano

Thanks for the link, I used the solution posted in the link and all tests pass(including the one that I added with the foreign keys).
I think my solutions is not the best, but it works.
tomas-stefano/rails@1ef6772
Thanks @sobrinho, thanks to you, this is my first commit in Rails! =p
s2 lol!

Cheers,
Tomás D'Stefano

@sobrinho

❤️

@tomas-stefano tomas-stefano pushed a commit that referenced this issue Jun 1, 2011
Tomas D'Stefano Attempt to fix issue #402 #402
Reverting but left the test there

Fix issue #402
7b27d85
@jonleighton jonleighton added a commit that referenced this issue Jul 8, 2011
Tomas D'Stefano Destroy association habtm record before destroying the record itself.…
… Fixes issue #402.
28f057c
@jonleighton jonleighton added a commit that referenced this issue Jul 8, 2011
Tomas D'Stefano Destroy association habtm record before destroying the record itself.…
… Fixes issue #402.
15f5c3b
@jonleighton jonleighton added a commit that referenced this issue Jul 8, 2011
Tomas D'Stefano Destroy association habtm record before destroying the record itself.…
… Fixes issue #402.
ea4b94a
@jonleighton jonleighton closed this Jul 8, 2011
@ttosch ttosch pushed a commit that referenced this issue Jan 19, 2015
Tomas D'Stefano Destroy association habtm record before destroying the record itself.…
… Fixes issue #402.
4364ae8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment