Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #6466 (reopened defect)

Opened 2 years ago

Last modified 10 months ago

[PATCH] fix for has_many :through push and delete with legacy/non-standard foreign_keys

Reported by: naffis Assigned to: technoweenie@gmail.com
Priority: normal Milestone: 1.2.7
Component: ActiveRecord Version: 1.2.2
Severity: normal Keywords: has_many through push delete foreign key
Cc: ian@almamatcher.com, runako@onepercentsoftware.com, codesujal@gmail.com, henrik@nyh.se, evan

Description

Fix for #6049.

It currently uses "find_all_by_#{@reflection.source_reflection.association_foreign_key}" which doesn't work in situations with legacy id's or non-standard id's (most commonly seen in self-referential relationships).

Additionally fixed << method to use the foreign_key when constructing join attributes. Currently it ignores the foreign_key and fails for example like the one below.

Example:

class User < ActiveRecord::Base

has_many :relationships, :foreign_key => 'user1_id' has_many :friends, :through => :relationships, :source => :friend_of

end

class Relationship < ActiveRecord::Base

belongs_to :friend_of, :class_name => "User", :foreign_key => :user2_id belongs_to :user1, :class_name => "User", :foreign_key => :user1_id belongs_to :user2, :class_name => "User", :foreign_key => :user2_id

end

david = User.new.save! rick = User.new.save! david.friends << rick david.friends.delete(rick)

Attachments

fix_add_and_delete_for_has_many_through.diff (6.9 kB) - added by naffis on 10/26/06 03:17:37.
fix add and delete for has many through, includes test

Change History

10/23/06 00:08:36 changed by david

  • owner changed from David to technoweenie@gmail.com.

We need tests to verify this issue, so that it doesn't regress again at a later point. Thanks.

10/26/06 03:17:37 changed by naffis

  • attachment fix_add_and_delete_for_has_many_through.diff added.

fix add and delete for has many through, includes test

10/26/06 03:21:41 changed by naffis

New attachment includes fixtures and tests. Left the original tests for tag/tagging in place to verify that it works with standard id values.

12/21/06 21:45:43 changed by ijcd

  • cc set to ian@almamatcher.com.

02/12/07 00:32:35 changed by runakog

  • cc changed from ian@almamatcher.com to ian@almamatcher.com, runako@onepercentsoftware.com.
  • version changed from edge to 1.2.1.
  • milestone deleted.

I'm experiencing the incorrect behavior in << for HMT associations using non-standard foreign keys (my model relates users to other users). Applying this patch appears to solve the problem for me.

I'm actually on 1.2.2, but the version box below only goes to 1.2.1.

02/19/07 19:40:19 changed by evan

I too am experiencing this problem with {association}.delete on edge.

03/04/07 19:41:43 changed by skwp

I tested this patch, it fixes the ability to do proper has many through << when you have a self referential has_many through (I was modeling a two way friend/friend_of friendship/befriendship) relationship. I hope this makes it into a release very soon as I have seen many references on the web to people having this problem and it's not easy to find this particular ticket. Thanks!

This patch was not applied to 1.2.2 as far as I know, my 1.2.2. still had the problem.

03/05/07 10:35:12 changed by alpinegizmo

Ticket 7153 is a duplicate of this one.

03/05/07 11:37:38 changed by bitsweat

  • status changed from new to closed.
  • resolution set to duplicate.

Fixed in [6336] trunk, [6337] 1-2-stable using the simpler #7153 patch.

03/12/07 18:30:32 changed by skwp

  • status changed from closed to reopened.
  • version changed from 1.2.1 to 1.2.2.
  • resolution deleted.

Patch applied to 1-2-stable is _no good_. When I applied the patch included on this ticket to rails 1.2.2 it was working fine, however the rails 1-2-stable branch that I just checked out now does not work properly. Specifically, the .delete() method on the through association is not working as expected.

Please apply the patch attached to this ticket instead of the other patch to 1-2-stable.

03/12/07 18:50:34 changed by bitsweat

  • keywords set to has_many through push delete foreign key.
  • milestone set to 1.2.3.

Thanks for the report, skwp.

04/03/07 12:49:23 changed by davetroy

I am experiencing the same problem on 1.2.3 and on r6500 Edge, specifically the '<<' push operator works but the delete method seems to still be calling the conventional finder method.

NoMethodError: undefined method `find_all_by_user_id' for UserRelationship:Class

If there is a current patch I should apply to 1.2.3 or Edge to fix this I would love to know about it. Meantime '<<' is working while it did not in 1.2.2.

04/13/07 05:19:53 changed by skwp

1-2-stable still does not have the correct patch (which is attached to the top of this ticket). If you are running 1-2-stable or 1.2.2. or something like that you can use the patch at the top of this ticket. If you guys patch this into 1-2-stable that would be great so the rest of us wouldn't have to maintain patched copies of rails...thanks!

07/06/07 18:06:18 changed by sujal

  • cc changed from ian@almamatcher.com, runako@onepercentsoftware.com to ian@almamatcher.com, runako@onepercentsoftware.com, codesujal@gmail.com.

09/19/07 14:08:07 changed by pradeep

As of r7514, the delete method still appears to be calling the conventional finder method. Any word on when this patch will be merged in? Thanks.

10/24/07 15:37:09 changed by Henrik N

  • cc changed from ian@almamatcher.com, runako@onepercentsoftware.com, codesujal@gmail.com to ian@almamatcher.com, runako@onepercentsoftware.com, codesujal@gmail.com, henrik@nyh.se.

10/27/07 10:35:51 changed by evan

  • cc changed from ian@almamatcher.com, runako@onepercentsoftware.com, codesujal@gmail.com, henrik@nyh.se to ian@almamatcher.com, runako@onepercentsoftware.com, codesujal@gmail.com, henrik@nyh.se, evan.

10/27/07 18:49:55 changed by bitsweat

(In [8042]) Fix has_many :through delete with custom foreign keys. References #6466.

10/27/07 18:51:36 changed by bitsweat

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [8043]) Fix has_many :through delete with custom foreign keys. Closes #6466.

10/27/07 19:25:26 changed by bitsweat

(In [8044]) Missed svn adds for [8042]. References #6466.

11/12/07 01:20:01 changed by lastobelus

  • status changed from closed to reopened.
  • resolution deleted.

Note that this patch breaks code which depends on flatten_deeper being called on the arguments.

After patch is installed, you can no longer pass an Array to delete.

Patch should flatten_deeper first.

11/12/07 01:30:36 changed by lastobelus

P.S. Example of code that breaks is the Tagging setup that HasManyPolymorphs generates.

11/12/07 02:30:31 changed by lastobelus

After looking more, I see that it is only broken on 1-2-stable, not on trunk.