Fix wrong table aliases (#1217) #1447
Conversation
On second thought: The original implementation of the |
This simply mirrors ActiveRecord's "deduplication" of joined tables, to not use table aliases that are not actually in the final query's JOIN clause.
deivid-rodriguez
force-pushed
the
has_one_through_fix
branch
from
f39a6f6
to
c9f2344
Compare
|
Wow, this is so awesome! It's been an unresolved issue for a long time, and biting several people as you point out! Thanks 😍. I have one question, we even got some regression specs contributed to demonstrate the issue at #1280. Do you think that spec is worth keeping on top of the ones you added, or they are basically duplicates and should be discarded? |
|
Thank you so much for responding so quickly and for the praise 😊
Those seem to be roughly equivalent to what I did (and I wish I had seen them sooner). The main difference seems to be the number of test records created (I used the bare minimum). Other than that it seems to demonstrate the exact same problem and would be redundant. I only had a cursory glance though, I might have missed some nuance there. |
|
Awesome, getting this in then, thanks again! |
I encountered the same issue as reported in #1217 (and possibly in a bunch of other issues as well1).
It was correctly pointed out, that a change in ActiveRecord was responsible. rails/rails@10b36e8 tried to fix a bug with eager loading that resulted from aliasing a joined table that was (then incorrectly) referenced in a WHERE clause.
The solution (as far as I understand it - I never had to look at that code before and cannot say I understand all of what is going on there) was to cache tables already joined and reuse those joins if possible.
The problem in #1217 is caused by ransack prepopulating table information before this deduplication code is run. So there is no way to detect that a JOIN clause with an alias, that would otherwise have been used, fell victim to the deduplication.
My "solution" is a poor one. I simply copied the deduplication logic from ActiveRecord. This should give the same results as ActiveRecord would come up with on its own.
A better solution would of course be to somehow get the table information at a later point in time and/or extract the deduplication logic in AR to be able to reuse this more easily. But I suspect the first approach would take a huge refactoring of ransack and I am not 100% sure if it is even possible. Also, I am not convinced that ActiveRecord currently cleans up references to table aliases that are not being used due to the deduplication correctly. So patches for AR itself might be needed anyways.
Footnotes
The following issues seem like they could be related: Invalid references to table aliases when joining multiple tables #1238, Ransack broke ActiveRecord sql when same table and different "belongs_to" joining (even though not use ransack search) #1253, Invalid table alias when searching has_one through associations #1227, Incorrect reference to table alias. #1153, Cannot join same table multiple times in new version #1144 ↩