Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Strip Empty/Blank Params from Ransack Params Hash #207

Closed
wants to merge 4 commits into from

6 participants

@jwg2s

By letting key value pairs with the value being an empty string into the params hash, Ransack creates an SQL query that unnecessarily adds LEFT OUTER JOINs. These unnecessary LEFT OUTER JOINs cause duplicate data to be returned from the database.

You can either accept this pull request, or use .result(distinct: true) in order to remove the duplicates. The disadvantage of the distinct: true option is that it can cause a full table scan when one is not necessary, so with massive sets of data, this solution is preferred.

Includes 3 tests to back up changes as well.

@t27duck

An edge case, but shouldn't happen within the scope of a Rails web request, if the search value is false (not the string "false", but rather boolean false) this change would remove that search term from the params hash. Again, this shouldn't happen from a web form since everything going into the controller is a string, but something to consider if you happen to be in a Rails console doing some ad hoc "ransacking" and you get lazy with some of your inputs.

Just felt like I should bring that point up. Other than that, I like this change.

@jwg2s

Thanks, a good point about fooling around at the console level. I could just check for nil or empty string to be sure we don't catch those by accident. Worth the change?

@t27duck

Problem with only checking for empty string is that stuff like " " (there's a space there) will then slip through.

In all honesty it's probably not worth the change. I just wanted to bring up that little gotcha.

@radar
Owner

I agree with @t27duck. If people are passing in a false value for a ransackable attribute then this pull request will break that case. If this PR could be updated to support that, then I think it would be fine.

@t27duck

Not the prettiest way to do it, but this would accomplish filtering out blank values but still allowing false

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

A hasty smoke test in rails c seems to support it.

1.9.3p385 :058 > "".blank? && "" != false
 => true 
1.9.3p385 :059 > 2.blank? && 2 != false
 => false 
1.9.3p385 :060 > "false".blank? && "false" != false
 => false 
1.9.3p385 :061 > " ".blank? && " " != false
 => true 
1.9.3p385 :062 > [].blank? && [] != false
 => true 
1.9.3p385 :063 > false.blank? && false != false
 => false
@jwg2s

Alright change is in, ready to go whenever you guys are.

@jwg2s jwg2s closed this
@jwg2s jwg2s reopened this
@jwg2s

Anything holding this up? Somewhat surprised this didn't trip more people up

@foxban

We are also suffering from this, will this patch be merged?

@radar
Owner
@opti opti commented on the diff
.gitignore
@@ -2,3 +2,4 @@
.bundle
Gemfile.lock
pkg/*
+*.idea
@opti
opti added a note

I'd suggest to put it into your global .gitignore.

@jwg2s
jwg2s added a note

Probably a better idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonatack jonatack referenced this pull request from a commit
@jonatack jonatack More AR versioning on tests and merge PR207
PR207 on ernie/ransack by jwg2s (John Gerhardt) 'remove blank key value
pairs from ransack params hash'
0ccf7df
@jonatack
Owner

Hi @jwg2s, I've been trying out this PR in my repo and don't see any difference. The change doesn't seem to eliminate any LEFT OUTER JOINS or duplicate results in my use case. Could you elaborate or help me understand?

@jonatack jonatack closed this
@jonatack
Owner

@jwg2s I retract my last comment. The use case for this PR seems to be to eliminate LEFT OUTER JOINS and duplicates for the case when no associated tables are being searched by the user (as opposed my particular use case where there are usually associated tables), and this it does do correctly. This PR looks good to me.

@jwg2s

I'm not able to re-open pull requests. It would have to be @radar.

@jonatack jonatack reopened this
@jonatack
Owner

Reopened, sorry for closing it. Up to @radar.

@jonatack jonatack referenced this pull request from a commit
@jonatack jonatack Fix #158 feature request (remove empty search params from sort_link p…
…arams hash and URL)

Builds on the discussion in PR #207.

Submitted as a pull request to see if there is interest in having a shorter sort_link URL. 

If yes, will need some test coverage.
b1644d4
@jonatack
Owner

Not a big deal, but checking for params.present? or params.blank? instead of params being nil or not, would avoid running delete_if on an empty hash if params = {}.

@jonatack jonatack closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 28, 2013
  1. Remove blank key value pairs from params hash on ransack

    John Gerhardt authored
  2. Add tests to cover new feature

    John Gerhardt authored
Commits on Mar 11, 2013
  1. Allow false params for console usage

    John Gerhardt authored
This page is out of date. Refresh to see the latest.
View
1  .gitignore
@@ -2,3 +2,4 @@
.bundle
Gemfile.lock
pkg/*
+*.idea
@opti
opti added a note

I'd suggest to put it into your global .gitignore.

@jwg2s
jwg2s added a note

Probably a better idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
View
2  lib/ransack/adapters/active_record/base.rb
@@ -12,7 +12,7 @@ def self.extended(base)
end
def ransack(params = {}, options = {})
- Search.new(self, params, options)
+ Search.new(self, params ? params.delete_if { |k, v| v.blank? && v != false } : params, options)
end
def ransacker(name, opts = {}, &block)
View
14 spec/ransack/adapters/active_record/base_spec.rb
@@ -48,6 +48,20 @@ module ActiveRecord
s = Person.search(:doubled_name_eq => 'Aric SmithAric Smith')
s.result.count.should eq 1
end if defined?(Arel::Nodes::InfixOperation)
+
+ it "should remove empty key value pairs from the params hash" do
+ s = Person.search(:children_reversed_name_eq => '')
+ s.result.to_sql.should_not match /LEFT OUTER JOIN/
+ end
+
+ it "should keep proper key value pairs in the params hash" do
+ s = Person.search(:children_reversed_name_eq => 'Testing')
+ s.result.to_sql.should match /LEFT OUTER JOIN/
+ end
+
+ it "should function correctly when nil is passed in" do
+ s = Person.search(nil)
+ end
end
describe '#ransackable_attributes' do
Something went wrong with that request. Please try again.