This project is archived and is in readonly mode.
collection_singular_ids breaks when used with :include
-
Michael Koziarski- Tag changed from :include, activerecord, eager_loading, patch to :include, activerecord, bugmash, eager_loading, patch
-
diraverified
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.69 KB
-
-
Jason RoelofsI just ran into this bug. Asking for collection_singular_ids shouldn't try to load any include-ed associations.
-
Rizwan Reza- Tag changed from :include, activerecord, bugmash, eager_loading, patch to :include, activerecord, eager_loading, patch
-
-
Dan Pickett- 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?
-
Diego Algorta- 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)
-
Diego Algorta
I forgot to include one file in the test patch, So I'm attaching a new commit with the missing file.
-
Diego Algorta
- No changes were found…
-
Federico Brubacher
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 -
Diego Algorta
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.58 KB
-
2-fix_bug.diff
2.79 KB
-
José Valim- 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.
-
Diego Algorta
- 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.
-
PratikPlease use .except(:includes) instead.
-
Rizwan Reza
- Assigned user changed from Jeremy Kemper to Pratik
-
Diego Algorta
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
-
Repository
- 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
- Tag changed from :include, activerecord, bugmash, eager_loading, patch to :include, activerecord, eager_loading, patch