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: Parked at Loopia

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 - Google Code Archive - Long-term storage for Google Code Project Hosting. 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: