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

Round-trip problem with empty params #366

Closed
daniel-rikowski opened this issue May 4, 2014 · 5 comments
Closed

Round-trip problem with empty params #366

daniel-rikowski opened this issue May 4, 2014 · 5 comments

Comments

@daniel-rikowski
Copy link

tl;dr: I've noticed a small problem with empty search queries if the params are piped through url_for (or link_to) and then redirected to Ransack again.

Long example:

  1. This url is sent to Rails: /orders?utf8=✓&q[id_eq]=&q[order_code_cont] and subsequentially processed by Ransack via Order.search(params[:q]).
  2. Before Order.search is executed the params hash looks like this:
{
  'action' => 'index'
  'controller' => 'orders'
  'q' => {
    'id_eq' => ''
    'order_code_cont' => ''
  }
}
  1. After Order.search is executed the params hash looks like this:
{
  'action' => 'index'
  'controller' => 'orders'
  'q' => {
  }
}

Most likely this is because of #201

  1. The actual problem shows when I'm using url_for (in this case to add a page param)
    url_for(params.merge(page: 2)) generates this url: /orders?utf8=✓&q=&page=1

  2. Suddenly Ransack has to handle an (empty) string instead of a hash which leads to various exceptions.

I can think of two ways to fix this: Either don't change params (e.g. use params.dup before calling delete_if) or use params.presence at the necessary locations. (At Ransack::Adapters::ActiveRecord::Base.ransack and Ransack::Helpers::FormHelper.sort_link)

Thank you in advance!

@jonatack
Copy link
Contributor

jonatack commented May 4, 2014

Hi @daniel-rikowski, thank you for reporting this issue. The params are being processed in the following method in search.rb which has seen some changes lately. Are you using the latest version of Ransack (version 1.2.3)? Which version of Rails/Ruby? Could you provide a failing test spec for the issue (which would be very helpful), or, even better, a pull request for a solution with tests passing (and failing without it)? Thanks :)

In Ransack::Lib::Ransack::Search:

def initialize(object, params = {}, options = {})
  params = {} unless params.is_a?(Hash)
  (params ||= {}).delete_if { |k, v| [*v].all? { |i| i.blank? && i != false } }
  @context = Context.for(object, options)
  @context.auth_object = options[:auth_object]
  @base = Nodes::Grouping.new(@context, 'and')
  build(params.with_indifferent_access)
end

@daniel-rikowski
Copy link
Author

Hello @jonatack

I'm using Ruby 2.0.0p451, Rails 4.1 and Ransack 1.2.3.

I'll try to work on this this week and hopefully make a pull request soon :)

@jonatack
Copy link
Contributor

jonatack commented May 4, 2014

@daniel-rikowski Super. Which Ransack are you using? The official release, the master branch, or the rails-4.1 branch?

@daniel-rikowski
Copy link
Author

I've checked again today and both branches have the same problem. But in my situation it's not only Ransack::Lib::Ransack::Search#initialize but Ransack::Lib::Ransack::Search#ransack too:

def ransack(params = {}, options = {})
  Search.new(self, params ? params.delete_if {
     |k, v| v.blank? && v != false } : params, options)
end

At both places the params parameter is modified by delete_if.

BTW: Why is params cleaned at two separate locations? (Ransack::Adapters::ActiveRecord::Base#ransack and Ransack::Lib::Ransack::Search#initialize) Wouldn't the one at Ransack::Lib::Ransack::Search#initialize be enough? It seems to be more comprehensive, too.

@jonatack
Copy link
Contributor

Fixed with 28433f9.

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

Successfully merging a pull request may close this issue.

2 participants