Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes polymorphic joins. #1122

Merged

Conversation

PhilCoggins
Copy link
Contributor

@PhilCoggins PhilCoggins commented May 11, 2020

  • Polymorphic joins have been missing a critical join constraint that
    is responsible for only returning polymorphic records of the specified
    type.

  • Override ActiveRecord::Reflection::AbstractReflection#join_scope to
    add this constraint if the reflection is polymorphic.

Polymorphic searching has been broken in Ransack since rails/rails@49bcb00.

This PR closes #1039, closes #1120, and closes #1077.

This bug has been a source of great frustration for our organization, and presumably many users of this library are completely unaware of the incorrect queries being formed by this library. I have no idea how long these Polymorphic hacks on ActiveRecord will last, but hopefully this will be caught in the future with the test addition in this PR.

@PhilCoggins PhilCoggins force-pushed the fix_polymorphic_joins branch from 387a71f to 8d418b4 Compare May 11, 2020 20:33
* Polymorphic joins have been missing a critical join constraint that
is responsible for only returning polymorphic records of the specified
type.

* Override ActiveRecord::Reflection::AbstractReflection#join_scope to
add this constraint if the reflection is polymorphic.
@PhilCoggins PhilCoggins force-pushed the fix_polymorphic_joins branch from 8d418b4 to 01d35ef Compare May 11, 2020 20:46
@scarroll32
Copy link
Member

Thank you @PhilCoggins

@scarroll32 scarroll32 merged commit abc8e45 into activerecord-hackery:master May 27, 2020
@Jacoblab1
Copy link

Jacoblab1 commented Sep 1, 2020

I've just upgraded to Rails 6.1 (master branch), and am having an error come up with polymorphic joins using Ransack.
I'm not sure if this is a breaking change in 6.1 or what. I want to use 6.1 for the new ActiveStorage proxying, but I can't if all my polymorphic searches break.

Any ideas @PhilCoggins ? Been doing some digging but can't find a solution yet.

Here's a framework trace that I get:


ransack (c9cc20de9e0f) lib/polyamorous/activerecord_6.0_ruby_2/join_dependency.rb:31:in `join_constraints'
rails (854587c268a5) activerecord/lib/active_record/relation/query_methods.rb:1294:in `build_joins'
rails (854587c268a5) activerecord/lib/active_record/relation/query_methods.rb:1158:in `build_arel'
rails (854587c268a5) activerecord/lib/active_record/relation/query_methods.rb:1081:in `arel'
rails (854587c268a5) activerecord/lib/active_record/relation.rb:845:in `block (2 levels) in exec_queries'
rails (854587c268a5) activerecord/lib/active_record/relation/finder_methods.rb:405:in `apply_join_dependency'
rails (854587c268a5) activerecord/lib/active_record/relation.rb:840:in `block in exec_queries'
rails (854587c268a5) activerecord/lib/active_record/relation.rb:868:in `skip_query_cache_if_necessary'
rails (854587c268a5) activerecord/lib/active_record/relation.rb:835:in `exec_queries'
rails (854587c268a5) activerecord/lib/active_record/association_relation.rb:48:in `exec_queries'
rails (854587c268a5) activerecord/lib/active_record/relation.rb:639:in `load'
rails (854587c268a5) activerecord/lib/active_record/relation.rb:250:in `records'
rails (854587c268a5) activerecord/lib/active_record/relation/delegation.rb:88:in `each'

And error is: wrong number of arguments (given 3, expected 2)

It seems as though join_constraints only takes two args, but there are three being passed to it from ActiveRecord.

@PhilCoggins
Copy link
Contributor Author

Hi @Jacoblab1 , I have not started using Rails 6.1 yet, and it looks like this project isn't building against it either.

I think for a start you'll want to add an override in lib/polyamorous/activerecord_6.1_ruby_2/join_dependency.rb that contains its own version of #join_constraints with three args.

That would honestly just be a start. I would frankly caution against using this library altogether beyond Rails 6.0, Ransack modifies a ton of ActiveRecord private API that has changed a lot, Ransack just hasn't been able to keep up with it all. Our organization is slowly replacing Ransack with an internal implementation (ideally we will OSS, but no definite timeframe on that).

@Jacoblab1
Copy link

Hi @Jacoblab1 , I have not started using Rails 6.1 yet, and it looks like this project isn't building against it either.

I think for a start you'll want to add an override in lib/polyamorous/activerecord_6.1_ruby_2/join_dependency.rb that contains its own version of #join_constraints with three args.

That would honestly just be a start. I would frankly caution against using this library altogether beyond Rails 6.0, Ransack modifies a ton of ActiveRecord private API that has changed a lot, Ransack just hasn't been able to keep up with it all. Our organization is slowly replacing Ransack with an internal implementation (ideally we will OSS, but no definite timeframe on that).

Thanks for getting back to me! I played around with patching the override, but it ended opening up some other problems that were getting pretty complicated.

As I'm only really using Ransack for filters on one controller, I think I'll just go ahead and re-build the filters myself with scopes. I figure that will be easier to maintain for me.

@kul-p
Copy link

kul-p commented Jun 25, 2021

@PhilCoggins @Jacoblab1 So is the rails 6.1 about "polyamorous/activerecord_6.1_ruby_2/join_dependency" was resolved ? I just follow up but didn't get it. Perhaps you can help to clarify. Or what solution to use/Thanks

@kul-p
Copy link

kul-p commented Jun 25, 2021

OK Got it, just upgrade ransack and that errors are gone.

gem 'ransack', '~> 2.4', '>= 2.4.2'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants