Fix find_by with ActiveRecord::Base object

[kamipo]

Fixes #26210.

[rails-bot]

r? @arthurnn

(@rails-bot has picked a reviewer for you, use r? to override)

[matthewd]

Noted in passing: this sounds like it would let a Range through, but the below seems ill-equiped to handle that.

[rafaelfranca]

We expliclty didn't want to users to use active record instances in find methods we even have deprecations to that in 4.2 https://github.com/rails/rails/blob/4-2-stable/activerecord/lib/active_record/core.rb#L139. The proper fix to this is raise an exception.

[kamipo]

We have a PredicateBuilder::BaseHandler therefore the following find_by query works without deprecation.

  Post.find_by(author_id: author, type: ["Post", "SpecialPost"])

Should we raise an exception for find_by with AR::Base only query?

[rafaelfranca]

That seems weird. In 4.2 we told people to not use AR instances in finder methods, now we are allowing again. I believe that case is a left over of @tenderlove's work to deprecate support of AR instances in finders.

@tenderlove WDYT about this?

[kamipo]

This deprecation was added to AR::Core::ClassMethods#find and AR::FinderMethods#find_one (in AR::FinderMethods#find). But find_by should work with AR instance. Otherwise the following query does not work.

  Essay.find_by(writer: author)
  # => SELECT  "essays".* FROM "essays" WHERE "essays"."writer_type" = 'Author' AND "essays"."writer_id" = 'David' LIMIT ?  [["LIMIT", 1]]

[rafaelfranca]

oh, I see what you mean. Yeah it makes sense. I'll think about a bit before taking the decision to merge or not. r? @rafaelfranca

[tenderlove]

I think something like:

  Essay.find_by(writer: author)

is be OK. What I'd really like to eliminate is

Post.find_by(author_id: author, type: ["Post", "SpecialPost"])

AR objects aren't ids, so it doesn't make sense.

[kamipo]

Actually find_by is an alias to where(arg, *args).take therefore Post.where(author_id: author, type: ["Post", "SpecialPost"]).take works because PredicateBuilder::BaseHandler exists.

https://github.com/rails/rails/blob/v5.0.0/activerecord/lib/active_record/relation/finder_methods.rb#L78
https://github.com/rails/rails/blob/v5.0.0/activerecord/lib/active_record/relation/predicate_builder/base_handler.rb

Should we deprecate PredicateBuilder::BaseHandler like PredicateBuilder::ClassHandler?

https://github.com/rails/rails/blob/v5.0.0/activerecord/lib/active_record/relation/predicate_builder/class_handler.rb

[matthewd]

IMO, no. Unlike Model.find(obj), which is pointless, that form is a useful implicit cast, and fits in perfectly with the number of other more elaborate predicate forms we support.

[kamipo]

I think that it should be consistent behavior both AR::Core::ClassMethods#find_by and AR::FinderMethods#find_by.
Currently Post.find_by(author_id: author) and Post.all.find_by(author_id: author) have inconsistent behavior.
For recovering consistency, I think we have 2 choices.

• Preproccess AR objects like PredicateBuilder::BaseHandler (This PR).
  • It is better for performance. StatementCache caches compiled Arel ASTs and preparable (predicate builder does not support AR objects preparable yet Make ActiveRecord::Base object to preparable for predicate builder #26202).
• Simply pass AR::FinderMethods#find_by.

--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -205,7 +205,7 @@ def find_by(*args) # :nodoc:
         hash = args.first

         return super if hash.values.any? { |v|
-          v.nil? || Array === v || Hash === v || Relation === v
+          v.nil? || Array === v || Hash === v || Relation === v || Base === v
         }

[sgrif]

Long term I'd actually like to reuse the handlers from the predicate builder as part of the statement cache, as well as provide a point of extension for the connection so we can do things like change arrays to use = ANY on PG. It'd also potentially let us handle ranges properly since there's a known limited subset of queries that we will execute for a given range.

Short term though I think the second option makes the most sense as the temporary stop-gap

[rafaelfranca]

Closing in favor of #27564