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

Fix find_by with range conditions #29842

Merged
merged 1 commit into from
Aug 2, 2017

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Jul 18, 2017

StatementCache doesn't support range conditions. So we need to through
the args to FinderMethods#find_by if range value is passed.

@@ -192,7 +192,9 @@ def find_by(*args) # :nodoc:
hash = args.first

return super if !(Hash === hash) || hash.values.any? { |v|
v.nil? || Array === v || Hash === v || Relation === v || Base === v
case v
when NilClass, Array, Range, Hash, Relation, Base then true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be better written like:

types = [NilClass, Array, Range, Hash, Relation, Base].freeze

v.class.in?(types)

it's a little bit odd to have a case statement which has only one branching and returns true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v.class.in?(types) is not a same with when *types.
Post.new.class is Post, not Base.
Post.all.class is Post::ActiveRecord_Relation, not Relation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right.

`StatementCache` doesn't support range conditions. So we need to through
the args to `FinderMethods#find_by` if range value is passed.
@matthewd matthewd merged commit 75436a4 into rails:master Aug 2, 2017
@kamipo kamipo deleted the fix_find_by_with_range branch August 2, 2017 11:03
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 this pull request may close these issues.

None yet

3 participants