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

Rails 4.1.1 scope named 'search' conflict with Ransack. #369

Closed
phallstrom opened this issue May 8, 2014 · 11 comments
Closed

Rails 4.1.1 scope named 'search' conflict with Ransack. #369

phallstrom opened this issue May 8, 2014 · 11 comments

Comments

@phallstrom
Copy link

I just ran into an error with 4.1.1 and ransack. I don't use ransack myself, but activeadmin does apparently. I have a scope on an AR model named 'search'. Starting my rails app would result in the following error:

activerecord-4.1.1/lib/active_record/scoping/named.rb:143:in `scope': You tried to define a scope named "search" on the model "MyModel", but Active Record already defined a class method with the same name. (ArgumentError)

4.1.1 will now complain if you try to use a scope name it considers dangerous. Dangerous is defined as any method name already defined by ActiveRecord::Base. Here's the git commit for it:

rails/rails@7e8e91c

Took me awhile to find it since AR doesn't define 'search' anywhere. Ransack does though:

alias :search :ransack unless base.respond_to? :search

https://github.com/activerecord-hackery/ransack/blob/master/lib/ransack/adapters/active_record/base.rb#L7

I'm guessing that ActiveAdmin doesn't use 'search' directly since this all worked in Rails 4.0.5 using my model's search method and ActiveAdmin on the same model.

Not sure what the right solution is since ransack wouldn't know about my scope name until it's too late and I'm sure most of your user's use 'search'. Maybe not.

I found the following to work if placed in a config/initializer file.

Ransack::Adapters::ActiveRecord::Base.class_eval('remove_method :search')
@jonatack
Copy link
Contributor

jonatack commented May 9, 2014

Hi @phallstrom, thanks for the information.

Rails 4.1.1 and Ransack (official/master/rails-4.1 branch) are all working fine for me.

Which branch of Ransack are you using? Simple or advanced mode? Have you tried it without AA?

As this appears to be a problem with AA or from something else, and you have found a workaround, I'm closing this for now.

@jonatack jonatack closed this as completed May 9, 2014
@phallstrom
Copy link
Author

@jonatack It actually doesn't have anything to do with AA. I only encountered it because AA requires ransack (I don't use it myself). It may or may not be a bigger problem. The issue is that ransack aliases 'search' to 'ransack' and both of those get included into AR::Base.

With Rails 4.1.1 you can't have a scope that has the same name as any method in AR::Base. So, the net effect is that if you use ransack you can't have 'search' as a scope in any model.

I mostly brought it up because you are aliasing it and doing a check to not do it if it's already defined. Just wondering if you should add a configuration option to not use it all for those of us like myself who want a scope named 'search' but have some other gem using ransack.

@seanlinsley
Copy link
Contributor

I'm guessing that ActiveAdmin doesn't use 'search' directly since this all worked in Rails 4.0.5 using my model's search method and ActiveAdmin on the same model.

That's correct; Active Admin uses ransack

I'm +1 on making search optionally defined.

@jonatack
Copy link
Contributor

jonatack commented May 9, 2014

Thanks again for the heads up on that Rails commit; I follow Rails master but had not seen that one.

If I understand correctly, the issue comes from using a scope named 'search' for another use than search with Ransack, concurrent with Ransack adding the name 'search' to AR::Base?

If yes, it seems to me that this is a problem that most (intentional) users of Ransack will not encounter. Still, I'd be okay with a PR that doesn't add more overhead to Ransack to make search optionally defined.

@jonatack jonatack reopened this May 9, 2014
@jonatack
Copy link
Contributor

jonatack commented May 9, 2014

Another possible solution would be to mention your workaround in the Ransack README and wiki.

@tmandry
Copy link

tmandry commented May 30, 2014

+1 for optionally defined, this was a headache back when ActiveAdmin used meta_search.

@michaelschmitz
Copy link

+1 for optionally defined or renamed as well. I encountered the same problem by using ActiveAdmin/Ransack and Acts-As-Messageable.

@jonatack
Copy link
Contributor

jonatack commented Nov 9, 2014

As @phallstrom mentions, making it opt-in would break a bunch of apps that depend on the search alias. The time to do that would be if/when we add any other backward-incompatible changes in a v.2.0.0 release. Will add @phallstrom's workaround to the README and wiki.

Allowing users to opt-out or set a different name in config/initializers/ransack.rb might be a good idea. If anyone wants to make a PR or has a better solution, that would be great.

@jonatack jonatack closed this as completed Nov 9, 2014
jonatack referenced this issue Nov 10, 2014
TODO: allow opting-out of `alias :search :ransack`
in the Ransack config/initializer file.

[skip ci]
jonatack referenced this issue Jan 6, 2015
and mention possible future deprecation of #search being
available by default.

[skip ci]
jonatack referenced this issue Jan 6, 2015
of #search default alias in Ransack 2.0 to fix reported issues of
method name conflicts with other gems.

[skip ci]
@fragoulis
Copy link

With Rails 4.1.1 you can't have a scope that has the same name as any method in AR::Base. So, the net effect is that if you use ransack you can't have 'search' as a scope in any model.

Can please someone point me to the respective rails PR????

@fragoulis
Copy link

To answer my own question rails/rails#13389

@danielolivaresd
Copy link

I can confirm that to this day (ransack 2.1.1, rails 6.0.0, activeadmin 2.6.1), having existing ::search scopes declared in one or more Rails models and using ransack via ActiveAdmin gives this error.

The simple fix is to change the existing ::search scope name on your model(s) to something like ::own_search when possible.
However, I would agree that having the ability to configure the name of the scope used would be a nice benefit (please let me know if there's a way of achieving this, since I couldn't find it).

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

No branches or pull requests

7 participants