Fix integer out of range #284

PhilippMeissner · 2018-03-09T14:32:34Z

Hi folks!
We've lately had a few users trying to search for values that were not meant to be searched but still caused datatables to try and search for it. This lead to this error

9056216999123 is out of range for ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Integer with limit 4

This issue only applies to :eq searches and only if it's an integer value that's stored in this very column.

Maybe this PR is of use. If not, have it removed please 👍

n-rodriguez · 2018-04-18T22:31:00Z

Hi! Thanks for your contribution! There are some changes to do before I can merge it. Also can you please rebase on master? Thank you!

PhilippMeissner · 2018-04-20T07:07:32Z

Hey @n-rodriguez - What do you mean by rebase? Do you want me to squash the three commits of mine so that it is only one? That should be available in the dropdown when you merge the PR through the UI. But if you mean something else, hit me up. What changes do you need to be done? Maybe I can spend some more time.

n-rodriguez · 2018-04-21T15:29:43Z

Hi! I mean merging the last commits I've made into your branch. But since it's a rebase it's gonna take my commits first then apply yours. It works like that :

git remote add upstream https://github.com/jbox-web/ajax-datatables-rails.git
git fefch upstream/master # or 'upstream master' don't remember
git checkout <your branch>
git rebase upstream/master

You'll have to do git push -f -u origin <your branch>to push your changes.

PhilippMeissner · 2018-04-22T19:28:48Z

Hey @n-rodriguez. Thanks for the small tour on rebasing - I've followed it and pushed everything :)

n-rodriguez

There are some changes to do before I can merge it. Thank you?

n-rodriguez · 2018-04-18T22:32:54Z

lib/ajax-datatables-rails/datatable/column/search.rb

@@ -49,12 +52,20 @@ def regex_search
          end
        end

+        def empty_search
+          casted_column.matches("")


Use of simple quote is recommanded

n-rodriguez · 2018-04-18T22:34:02Z

lib/ajax-datatables-rails/datatable/column/search.rb

+              empty_search
+            else
+              numeric_search
+            end


I think it would be more readable if you invert the logic :

if is_searchable_integer? numeric_search else empty_search end

n-rodriguez · 2018-04-18T22:34:55Z

lib/ajax-datatables-rails/datatable/column/search.rb

+          table.engine.columns_hash[field.to_s].sql_type == 'integer' && is_integer?(search.value) && is_out_of_range?(search.value)
+        end
+
+        def is_out_of_range? search_value


Add parenthesis around arguments

n-rodriguez · 2018-04-18T22:36:40Z

lib/ajax-datatables-rails/datatable/column/search.rb

@@ -82,6 +93,18 @@ def numeric_search
          end
        end

+        def is_searchable_integer?
+          return false unless table.respond_to?(:engine)
+          table.engine.columns_hash[field.to_s].sql_type == 'integer' && is_integer?(search.value) && is_out_of_range?(search.value)


Invert the last test to match with the previous recommandation :

def is_searchable_integer? return false unless table.respond_to?(:engine) table.engine.columns_hash[field.to_s].sql_type == 'integer' && is_integer?(search.value) && !is_out_of_range?(search.value) end

PhilippMeissner · 2018-04-26T10:32:13Z

All change requests have been added and the tests are still running fine (on my end: bundle exec rspec).
Thanks for your suggestions and time 👍

n-rodriguez · 2018-05-06T03:29:31Z

Hi! Tests are failing for Rails < 5.x. Can you please take a look?

n-rodriguez · 2018-05-06T04:26:23Z

Hi! There is something wrong with your implementation.

n-rodriguez · 2018-05-06T04:27:14Z

lib/ajax-datatables-rails/datatable/column/search.rb

@@ -82,6 +89,18 @@ def numeric_search
          end
        end

+        def is_searchable_integer?
+          return true unless table.respond_to?(:engine)


This always return true on Rails 5.x because table doesn't respond to :engine

n-rodriguez · 2018-05-06T04:31:09Z

lib/ajax-datatables-rails/datatable/column/search.rb

@@ -82,6 +89,18 @@ def numeric_search
          end
        end

+        def is_searchable_integer?
+          return true unless table.respond_to?(:engine)
+          table.engine.columns_hash[field.to_s].sql_type == 'integer' && is_integer?(search.value) && !is_out_of_range?(search.value)


You get to this point on Rails 4 but is_out_of_range? always return false because it (can) receives an array of integer and is_integer?(string) shadows the casting error by rescuing to false.

1) AjaxDatatablesRails::ORM::ActiveRecord filter conditions it can filter records with condition :in should filter records matching Failure/Error: Integer(search_value) > LARGEST_PQ_INTEGER || Integer(search_value) < SMALLEST_PQ_INTEGER TypeError: can't convert Array into Integer

n-rodriguez · 2018-05-06T04:34:43Z

Actually your PR does nothing.

n-rodriguez · 2018-05-10T22:11:37Z

Ok, I think you forgot to create a DB field with a limited int size. By default Rails use int(11) on 4.2 and bigint on 5.x so no error is triggered with your tests and the default config.

n-rodriguez · 2018-05-10T22:46:08Z

Ok, it turns out you're using the wrong datatable in tests so it's doesn't use :eq condition but :like ;)

n-rodriguez · 2018-05-10T22:47:17Z

spec/ajax-datatables-rails/orm/active_record_filter_records_spec.rb

@@ -387,6 +387,32 @@
      end
    end

+    describe 'Integer overflows' do
+      let(:largest_postgresql_integer_value) { 2147483647 }


It misses let(:datatable) { DatatableCondEq.new(view) }

PhilippMeissner · 2018-05-11T06:28:07Z

Hey. Thank you for the effort. I am sorry I did not reply, was quite busy these days (plus there were a lot of holidays in Germany this month ;)).
Thanks for fixing and merging!

n-rodriguez · 2018-05-11T19:21:55Z

Thanks for fixing and merging!

Thank you too, you found it 🎉 I realized latter I could have merged your PR and make the fixes right after... sorry

n-rodriguez self-assigned this Apr 18, 2018

philippmeissner added 3 commits April 22, 2018 21:26

added specs

2b4676a

added helper methods

eb32b40

catched invalid integer values; added empty-search fallback

c7f3c6f

PhilippMeissner force-pushed the fix-integer-out-of-range branch from 4fcc4a2 to c7f3c6f Compare April 22, 2018 19:26

n-rodriguez requested changes Apr 24, 2018

View reviewed changes

fixed paranthesis, quotes; made logic more readable

2a73f9e

n-rodriguez force-pushed the master branch 6 times, most recently from 881e955 to fc1c987 Compare May 6, 2018 03:17

n-rodriguez reviewed May 6, 2018

View reviewed changes

n-rodriguez force-pushed the master branch 4 times, most recently from e975b07 to bd0393b Compare May 10, 2018 21:50

n-rodriguez force-pushed the master branch from bd0393b to 82f3445 Compare May 10, 2018 22:24

n-rodriguez reviewed May 10, 2018

View reviewed changes

n-rodriguez mentioned this pull request May 10, 2018

Fix integer out of range #289

Merged

n-rodriguez closed this May 10, 2018

PhilippMeissner deleted the fix-integer-out-of-range branch May 11, 2018 06:28

fate83 mentioned this pull request May 23, 2018

Columns mapping: equals condition for string filter. #291

Closed

n-rodriguez mentioned this pull request Oct 16, 2018

Integer only support #303

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix integer out of range #284

Fix integer out of range #284

PhilippMeissner commented Mar 9, 2018 •

edited

Loading

n-rodriguez commented Apr 18, 2018

PhilippMeissner commented Apr 20, 2018

n-rodriguez commented Apr 21, 2018 •

edited

Loading

PhilippMeissner commented Apr 22, 2018

n-rodriguez left a comment

n-rodriguez Apr 18, 2018

n-rodriguez Apr 18, 2018

n-rodriguez Apr 18, 2018

n-rodriguez Apr 18, 2018

PhilippMeissner commented Apr 26, 2018

n-rodriguez commented May 6, 2018

n-rodriguez commented May 6, 2018

n-rodriguez May 6, 2018

n-rodriguez May 6, 2018

n-rodriguez commented May 6, 2018

n-rodriguez commented May 10, 2018 •

edited

Loading

n-rodriguez commented May 10, 2018 •

edited

Loading

n-rodriguez May 10, 2018

PhilippMeissner commented May 11, 2018

n-rodriguez commented May 11, 2018

Fix integer out of range #284

Fix integer out of range #284

Conversation

PhilippMeissner commented Mar 9, 2018 • edited Loading

n-rodriguez commented Apr 18, 2018

PhilippMeissner commented Apr 20, 2018

n-rodriguez commented Apr 21, 2018 • edited Loading

PhilippMeissner commented Apr 22, 2018

n-rodriguez left a comment

Choose a reason for hiding this comment

n-rodriguez Apr 18, 2018

Choose a reason for hiding this comment

n-rodriguez Apr 18, 2018

Choose a reason for hiding this comment

n-rodriguez Apr 18, 2018

Choose a reason for hiding this comment

n-rodriguez Apr 18, 2018

Choose a reason for hiding this comment

PhilippMeissner commented Apr 26, 2018

n-rodriguez commented May 6, 2018

n-rodriguez commented May 6, 2018

n-rodriguez May 6, 2018

Choose a reason for hiding this comment

n-rodriguez May 6, 2018

Choose a reason for hiding this comment

n-rodriguez commented May 6, 2018

n-rodriguez commented May 10, 2018 • edited Loading

n-rodriguez commented May 10, 2018 • edited Loading

n-rodriguez May 10, 2018

Choose a reason for hiding this comment

PhilippMeissner commented May 11, 2018

n-rodriguez commented May 11, 2018

PhilippMeissner commented Mar 9, 2018 •

edited

Loading

n-rodriguez commented Apr 21, 2018 •

edited

Loading

n-rodriguez commented May 10, 2018 •

edited

Loading

n-rodriguez commented May 10, 2018 •

edited

Loading