#2896 collection_singular_ids breaks when used with :include - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2896 ✓resolved

collection_singular_ids breaks when used with :include

Reported by Diego Algorta | July 9th, 2009 @ 09:30 PM | in 3.x

Given a has_many association defined with an :include option, you can't use the #collection_singular_ids method because it fails trying to eager load the 2nd level association when it's unneeded.

The attached patch includes a test case where you can see the need for it.

I'm not exactly happy with the patch because it involves adding a new :ignore_include valid option to ActiveRecord::Base.find, but I think I've read people requesting that kind of option too so maybe this is something you may want too.

Please review the patch and suggest any improvements or raise concerns.

Comments and changes to this ticket

Michael Koziarski August 3rd, 2009 @ 06:11 AM
- Tag changed from “:include, activerecord, eager_loading, patch” to “:include, activerecord, bugmash, eager_loading, patch”
You flagged this item as spam.
dira August 9th, 2009 @ 12:51 PM
verified

Updated just the tests from the patch to apply cleanly.

I think there should be a cleaner implementation - so I did not include the original one in the patch.
- singular_ids_tests.patch 4.7 KB
You flagged this item as spam.
Jason Roelofs November 19th, 2009 @ 07:05 PM
I just ran into this bug. Asking for collection_singular_ids shouldn't try to load any include-ed associations.
Rizwan Reza February 12th, 2010 @ 12:46 PM
- Tag changed from “:include, activerecord, bugmash, eager_loading, patch” to “:include, activerecord, eager_loading, patch”
[bulk edit]
Jeremy Kemper May 4th, 2010 @ 06:48 PM
- Milestone changed from “2.x” to “3.x”
[bulk edit]
Dan Pickett May 9th, 2010 @ 07:18 PM
- Tag changed from “:include, activerecord, eager_loading, patch” to “:include, activerecord, bugmash, eager_loading, patch”
Can a bugmasher verify this errant behavior on master and supply an updated path?
You flagged this item as spam.
Diego Algorta May 10th, 2010 @ 09:27 AM
- Assigned user set to “Jeremy Kemper”
I'm attaching two separate patches in case the core team doesn't like my fix, or someone shows up with a better one (which is quite possible).

add_failing_singular_ids_with_include_test.diff is an updated version of the failing test needed to prove the bug.

fix_failing_singular_ids_with_include.diff is the patch to fix activerecord so that the previously added test, passes. It's VERY different from my original patch when creating this ticket as rails itself has changed a lot since then. I still think someone with better knowledge of activerecord's internals could do it better, but this one seems to be good enough (and I think less messy than the previous one).

Both apply cleanly on top of ce5827ea4791e8b8143919ecceb0231e36e8932e (master from may 9)
You flagged this item as spam.
Diego Algorta May 11th, 2010 @ 12:36 AM
I forgot to include one file in the test patch, So I'm attaching a new commit with the missing file.
You flagged this item as spam.
Diego Algorta May 11th, 2010 @ 12:47 AM
- no changes were found...
You flagged this item as spam.
Federico Brubacher May 11th, 2010 @ 01:26 AM
The last patch looks much better than the first time, i ran the tests on latest version of master and so far so good. It seems a issue worth fixing.
+1
You flagged this item as spam.
Diego Algorta May 11th, 2010 @ 07:24 PM
As there where too many patch files already, I deleted the ones which were mine and I'm submitting them merged in just 2 files.
- 1-new_failing_test.diff adds a new test to demonstrate the bug.
- 2-fix-bug.diff fixes the bug, gets the test to pass. Maybe someone can think of a better fix, but I think this one is good enough.
- 1-new_failing_test.diff 5.6 KB
- 2-fix_bug.diff 2.8 KB
José Valim May 15th, 2010 @ 03:35 PM
- Tag changed from “:include, activerecord, bugmash, eager_loading, patch” to “:include, activerecord, eager_loading, patch”
- State changed from “new” to “verified”
- Assigned user changed from “Jeremy Kemper” to “Pratik”
Assigning to Pratik since he knows better about ActiveRecord.
You flagged this item as spam.
Diego Algorta May 15th, 2010 @ 03:42 PM
- Tag changed from “:include, activerecord, eager_loading, patch” to “:include, activerecord, bugmash, eager_loading, patch”
- Assigned user changed from “Pratik” to “Jeremy Kemper”
I've attached a patch.

As requested by rizwanreza on #railsbridge, I'm attaching a merged patch with both the test and the fix in just one commit. Rebased to latest master branch.
- merged_patch_fixing_singular_ids_with_include.diff 8.1 KB
Pratik May 15th, 2010 @ 03:48 PM
Please use .except(:includes) instead.
Rizwan Reza May 15th, 2010 @ 03:49 PM
- Assigned user changed from “Jeremy Kemper” to “Pratik”
You flagged this item as spam.
Diego Algorta May 15th, 2010 @ 04:40 PM
I've attached a patch.

This new patch has a way much simpler test and fix. Thx Pratik for pointing me to the right direction as the old patch and fix were based on the initial work for rails 2.3
- much_simpler_test_and_fix_leveraging_except_includes.diff 2.6 KB
Repository May 15th, 2010 @ 04:59 PM
- State changed from “verified” to “resolved”
(from [3436fdfc12d58925e3d981e0afa61084ea34736c]) Fix for get_ids when including a belongs_to association on a has_many association [#2896 state:resolved]

Signed-off-by: Pratik Naik pratiknaik@gmail.com
http://github.com/rails/rails/commit/3436fdfc12d58925e3d981e0afa610...
Rizwan Reza May 15th, 2010 @ 05:21 PM
- Tag changed from “:include, activerecord, bugmash, eager_loading, patch” to “:include, activerecord, eager_loading, patch”