Refactoring AR::Base.find

rubyonrails-core
#1

Hi guys, what do you think about this implementation (before I go off
and actually make it work). The plan is to make it easier to write
extensions or plugins for find, as well as define your own options for
the precious first-level argument, :first, :all, etc.

If you want to skip to the paste/diff, it's here: http://pastie.caboo.se/89252

The current way:

      def find(*args)
        options = args.extract_options!
        validate_find_options(options)
        set_readonly_option!(options)

        case args.first
          when :first then find_initial(options)
          when :all then find_every(options)
          else find_from_ids(args, options)
        end
      end

A new way?

      def find(*args)
        options = args.extract_options!
        validate_find_options(options)
        set_readonly_option!(options)

        finder.dispatch(args, options)
      end

      # The Finder class allows you to easily define your own custom
find implementations.
      # Simply define this class in your model, and any of its methods
will be available
      # to your #find call as the first symbol. For example,

#2

Hi guys, what do you think about this implementation (before I go off
and actually make it work). The plan is to make it easier to write
extensions or plugins for find, as well as define your own options for
the precious first-level argument, :first, :all, etc.

There's definitely some scope for tidying up some of those ....
interesting bits of Base which generate the queries.

This will also allow some fun things, like, keeping state in the
finder, or even recording the last SQL statistics.

  @user = User.find(2)
  User::Finder.records_returned => 25
  User::Finder.benchmark => 0.025

What do you guys think? All the AR tests still pass with this method,
since it's a fairly simple proxy, but it allows for much greater
customization.

What kind of customisation did you have in mind?

I've been kicking around a few more changes to AR::Base lately and
perhaps we could do something along these lines. My thoughts on our
querying at present are:

* It should be in / near the adapters.

There's already some code that needs to be different but is currently
implemented in base, by checking what the adapters support:

      # Should primary key values be selected from their corresponding
      # sequence before the insert statement? If true, next_sequence_value
      # is called before each insert to set the record's primary key.
      # This is false for all adapters but Firebird.
      def prefetch_primary_key?(table_name = nil)
        false
      end

That could ideally simply be handled with polymorphism

* It should be easier to inject conditions into the generated queries.

Basically the only reason that people are currently using with_scope
is to merge :conditions arguments. It should be easier to define
scopes, inject conditions into queries etc. This probably means some
kind of query class.

Combining these two would also aid databases like oracle which get
serious performance gains from the use of prepared statements.

#3

I've been kicking around a few more changes to AR::Base lately and
perhaps we could do something along these lines. My thoughts on our
querying at present are:

* It should be in / near the adapters.

Exactly. So, every query from AR should look like @adapter.execute() -
which would make it very simple to extend/optimize.

There's already some code that needs to be different but is currently
implemented in base, by checking what the adapters support:

      # Should primary key values be selected from their corresponding
      # sequence before the insert statement? If true, next_sequence_value
      # is called before each insert to set the record's primary key.
      # This is false for all adapters but Firebird.
      def prefetch_primary_key?(table_name = nil)
        false
      end

This reminds me to ask, are we gonna get rid of most of the adapters
from core for 2.0 ? I guess, only mysql and postgres and may be sqlite
deserves to be in core. Other adapter might be better off as external
libs.

That could ideally simply be handled with polymorphism

* It should be easier to inject conditions into the generated queries.

Basically the only reason that people are currently using with_scope
is to merge :conditions arguments. It should be easier to define
scopes, inject conditions into queries etc. This probably means some
kind of query class.

I kinda like the approach taken by scope-out plugin -
http://code.google.com/p/scope-out-rails/ Instead of query class, I
guess we need a Condition class first. What Courtnay suggested, looks
sweet too.

Just my few cents.

#4

This reminds me to ask, are we gonna get rid of most of the adapters
from core for 2.0 ? I guess, only mysql and postgres and may be sqlite
deserves to be in core. Other adapter might be better off as external
libs.

Shouldn’t we also have Oracle adapter in the core?

With an ever increasing popularity of Rails a lot of big companies are
now using it for their production quality apps and most of these run on
oracle.

I do agree that it won’t much of a difference whether Rails has oracle libs in built or are provided as external libs.

However it would be a little easy to convince the CTOs to tip the
favour towards Rails if it oracle support is provided out of the box.

#5

This ticket does remove "prefetch_primary_key?" and:
http://dev.rubyonrails.org/ticket/9253

It's just waiting for works-with-Oracle confirmation from Michael
Schoen.

#6

>
>
> This reminds me to ask, are we gonna get rid of most of the adapters
> from core for 2.0 ? I guess, only mysql and postgres and may be sqlite
> deserves to be in core. Other adapter might be better off as external
> libs.

Shouldn't we also have Oracle adapter in the core?

        [...]

However it would be a little easy to convince the CTOs to tip the favour
towards Rails if it oracle support is provided out of the box.

If stuff is being taken out to make this light, then I'd suggest that
a "batteries included" distribution be made available, so you can say
to 95% of people, "If you get the batteries include distro you will
(most likely) be covered".

        Hugh

#7

This looks like a definite improvement. Cleaner separation of
concerns, easier to extend and probably far easier to test. +1

Courtenay wrote:

#8

Beyond sharing the option-santizing methods, why is find(:first, ...),
find(:all, ...) preferable to find_first and find_all? As this patch
highlights, utilising the first argument like this makes it more
difficult to add new behaviour, but adding another dispatch layer to
lengthen the stack trace might be tackling the wrong problem to reach
the goal of making AR's internals easier to enhance.

The patched code is definitely nice, and definitely makes it simpler
to add new behaviour to the find() method. However, it also seems like
a lot of hoop-jumping to preserve a customised method-dispatch
mechanism, when it might be more sensible in the long term to figure
out a better way for concrete methods (find_first, find_all,
find_with_some_other_behaviour) to work with the option wrangling
code.

#9

+1

This is something I've been wanting for a long time...

-- Ernie P.

#10

And it should be much easier to autocompose :select lists so that lazy-loaded attributes could be implemented (perhaps by piggybacking
the blobs onto the :include option)...

#11

>
> This looks like a definite improvement. Cleaner separation of
> concerns, easier to extend and probably far easier to test. +1
>
> Courtenay wrote:
> > Hi guys, what do you think about this implementation (before I go off
> > and actually make it work). The plan is to make it easier to write
> > extensions or plugins for find, as well as define your own options for
> > the precious first-level argument, :first, :all, etc.
>

Beyond sharing the option-santizing methods, why is find(:first, ...),
find(:all, ...) preferable to find_first and find_all? As this patch
highlights, utilising the first argument like this makes it more
difficult to add new behaviour, but adding another dispatch layer to
lengthen the stack trace might be tackling the wrong problem to reach
the goal of making AR's internals easier to enhance.

It looks cooler, and hides all the find stuff behind a nice interface.

The patched code is definitely nice, and definitely makes it simpler
to add new behaviour to the find() method. However, it also seems like
a lot of hoop-jumping to preserve a customised method-dispatch
mechanism, when it might be more sensible in the long term to figure
out a better way for concrete methods (find_first, find_all,
find_with_some_other_behaviour) to work with the option wrangling
code.

Zing! You're right, of course. But my patch was specifically written
to be a simple drop-in replacement for the existing code, rather than
a huge freaking rewrite.

I actually tried rewriting it, but there are just too many
dependencies with the rest of the library, includling all that
alias_method_chain stuff.

#12

Zing! You're right, of course. But my patch was specifically written
to be a simple drop-in replacement for the existing code, rather than
a huge freaking rewrite.

Yup, and I think that's totally fine; I definitely strayed from the
central topic of the thread. Considering the benefits gained vs. the
amount of code changed, I think your patch has definite value and I
hope it does get committed.

That said, and continuing my aside -

I actually tried rewriting it, but there are just too many
dependencies with the rest of the library, includling all that
alias_method_chain stuff.

If it's felt that ActiveRecord* would benefit from some more extensive
work, what's the best way to approach this? My feeling is that large
patches are very unlikely to see any commit action, which is fair
because they're difficult to grasp.

The typical approach within a development team might be to create a
branch, but I'm not sure that works within the Rails development
process either. Any thoughts?

James

* or any other Rails framework for that matter, but most people seem
to express this more-radical-refactoring urge with ActiveRecord

#13

The typical approach within a development team might be to create a
branch, but I'm not sure that works within the Rails development
process either. Any thoughts?

There's two questions here, technical and process. The process side
of it is harder, if you're interested in undertaking some major
refactoring you should probably grab someone with commit access in
irc, and talk over the changes first. 'Refactoring' is a tough sell
without some kind of performance or functional improvement, especially
given its tendencies to break the hell out of plugins :wink:

As for the technical question... My personal 'major changes' tend to
happen in my git repository, then I commit them to svn when ready to
go. I'm happy to take patches or proposed patches via git instead of
svn diffs, but I can't promise anyone else will agree :wink:

git clone git://git.koziarski.com/rails
http://git.koziarski.com/?p=.git;a=summary

#14

One important feature of an ActiveRecord find refactoring should be
the ability to extend functionality without having to modify a core
class, although this perhaps does not come first.

After releasing ActiveRecord::Extensions 0.5.0 I started refactoring
AR and found a lot of nastiness. I like the dispatch pattern, and
there should also be a way to subscribe to the Finder to give it more
things to dispatch to.

Perhaps a callback for each key/value pair in the conditions hash. The
below would give you the ability to handle things suffixed with "_in",
ie: age_less_than => 50

IE:

ActiveRecord::Base.finder.new_find_condition do |key,val|
   if key=conditions_hash.keys.find(/(.+)_less_than$/) and val.is_a?
(Numeric)
     [ "#{$1} < ?", val ]
   end
end

Zach
P.S. - please CC my email in response to this

Courtenay wrote: