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

RangeError when querying for big integer using #where in Rails 4.2 #21309

Closed
januszm opened this issue Aug 20, 2015 · 11 comments
Closed

RangeError when querying for big integer using #where in Rails 4.2 #21309

januszm opened this issue Aug 20, 2015 · 11 comments

Comments

@januszm
Copy link

januszm commented Aug 20, 2015

Rails 3

Product.where(associated_id: 1311344470881970878875083923).first
=> nil

Rails 4.2.3

Product.where(associated_id: 1311344470881970878875083923).first
RangeError: 1311344470881970878875083923 is out of range for ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Integer with limit 8
from ~/.rvm/gems/ruby-2.2.1@gemset/gems/activerecord-4.2.3/lib/active_record/type/integer.rb:45:in ensure_in_range

I'd expect that the finder returns nothing in Rails 4.2.3 as well in such a case. It's just searching for results, we don't need to call ensure_in_range.

As a workaround, someone could use find_by, but when there's a need for returning multiple records, then there's no such method as find_all_by that could be used. BTW. find_by works because it rescues from RangeError.

@matthewd
Copy link
Member

It's just searching for results, we don't need to call ensure_in_range.

Yes, we do. That value won't fit into a PostgreSQL bigint, so we cannot run the query with it.

Previous versions didn't use parameterized queries, so PostgreSQL was able to dynamically guess the type based on the size of the literal.

@januszm
Copy link
Author

januszm commented Aug 20, 2015

@matthewd look, it's just a simple query to find something in the database

development=> select * from products where associated_id = 1184827921141444343875083923;
(0 rows)

PostgreSQL 9.4.4 , no error.

Anyway, perhaps it's easier to just add find_all_by method as I described above.

def find_all_by(*args)
  where(*args).load
rescue RangeError
  none
end

@matthewd
Copy link
Member

look, it's just a simple query

Great! Patch welcome.

perhaps it's easier to just add find_all_by method

We can't do that, for the same reason we don't currently rescue the RangeError everywhere: it would behave incorrectly any time the out-of-range value didn't imply no results. (e.g., where.not(id: BIG_NUMBER).find_all_by(name: 'Fred'))

@agis
Copy link
Contributor

agis commented Oct 2, 2015

@matthewd What about introducing a new exception class and raising that instead of (the more abstract) RangeError? Rescuing RangeError in ApplicationController doesn't sound very reasonable. /cc @sgrif

@avgerin0s
Copy link
Contributor

@matthewd

If you use find(903475239045782034957203945782934058230498) you get an ActiveRecord::RecordNotFound with a customized message for the out of range id.

If you use where(id: 903475239045782034957203945782934058230498), you get a RangeError.

IMHO, this behavior is kind of strange. ActiveRecord should be able to perform select queries with everything that the RDBMS allows you to.

I believe that this validation of range must only happen when trying to save to the database.

@sgrif
Copy link
Contributor

sgrif commented Oct 2, 2015

The for where is that we don't actually have a thing that we can do consistently to handle it other than raise an exception.

ActiveRecord should be able to perform select queries with everything that the RDBMS allows you to.

The RBDMS will not allow you to query with an out of range integer (sometimes, depending on context specifically using prepared statements is the main problem). This change was made to allow you to handle a RangeError with consistent semantics, rather than sometimes get StatementInvalid

@januszm
Copy link
Author

januszm commented Oct 2, 2015

I'm OK with it but it would be good to add a method which returns multiple records, similar to find_by to FinderMethods module

##
# Like <tt>find_by</tt>, but returns multiple records or none relation
#
def find_all_by(*args)
  where(*args).load
rescue RangeError
  none
end

UPDATE: for Rails 4.2.7.1 I had to add that method to AR Core class directly (ActiveRecord::Core::ClassMethods.module_eval do ...).

For Rails 5 such definition should go to ApplicationRecord

@agis
Copy link
Contributor

agis commented Oct 2, 2015

@sgrif What about #21309 (comment)?

@sgrif
Copy link
Contributor

sgrif commented Oct 2, 2015

@januszm If it were that simple, we would have just done that in where. Whether none vs all is correct varies by context in non-intuitive ways that we can't infer programmatically.

@agis- I think that RangeError is sufficiently specific for a value that is out of the valid range.

@agis
Copy link
Contributor

agis commented Oct 3, 2015

@sgrif Thing is, we are now forced to rescue RangeError in ApplicationController just to handle the ActiveRecord cases, when in fact RangeError may be coming from lots of other places which have nothing to do with AR. I was thinking something more specific like QueryRangeError which could inherit from RangeError.

@kdavh
Copy link

kdavh commented Feb 15, 2016

@agis- I completely agree, something like ActiveRecord::RangeError would be much better than a generic error like RangeError.

@sgrif I would agree that RangeError is acceptable, except the context of ActiveRecord's general use makes it so that a more specific error class is appropriate. class ActiveRecord::RangeError < RangeError is a good direction to go.

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

6 participants