Odditiy in find_initial

I stumbled across this today:

def find_initial(options)    options.update(:limit => 1) unless options[:include]    find_every(options).first end

And I can't think why the special casing of include is necessary. Maybe there was a time when :include didn't do the right thing, but for at least some time :include has handled this properly, and so any one doing Foo.find :first, :include => [...] is inadvertently making rails and the db do a lot more work than they think? Is this just a relic or is there some reason I missed ?

Fred

У Чцв, 24/04/2008 у 10:04 +0100, Frederick Cheung піша:

I stumbled across this today:

def find_initial(options)    options.update(:limit => 1) unless options[:include]    find_every(options).first end

Several month ago I asked same question. I didn't searched history, but I guess it's artifact of some very ancient times.

It works perfectly without this (actually, with :include it always fetches whole result set and cuts it in memory, which is stupid).

Our local version of rails has this turned off for almost year now. And has it long before new :include implementation landed. So it's safe to kill this. I hadn't yet time to post a patch for this though.

У Чцв, 24/04/2008 у 10:04 +0100, Frederick Cheung піша:

I stumbled across this today:

def find_initial(options)   options.update(:limit => 1) unless options[:include]   find_every(options).first end

Several month ago I asked same question. I didn't searched history,
but I guess it's artifact of some very ancient times.

It works perfectly without this (actually, with :include it always fetches whole result set and cuts it in memory, which is stupid).

Pretty much what I thought. I can't quite think how I'd write a test
to verify that it's not doing this though.

Fred

У Чцв, 24/04/2008 у 12:26 +0300, Aliaksey Kandratsenka піша:

У Чцв, 24/04/2008 у 10:04 +0100, Frederick Cheung піша: > I stumbled across this today: > > def find_initial(options) > options.update(:limit => 1) unless options[:include] > find_every(options).first > end Several month ago I asked same question. I didn't searched history, but I guess it's artifact of some very ancient times.

It works perfectly without this (actually, with :include it always fetches whole result set and cuts it in memory, which is stupid).

Our local version of rails has this turned off for almost year now. And has it long before new :include implementation landed. So it's safe to kill this. I hadn't yet time to post a patch for this though.

Actually, with git it's easier. Here we go.

0001-add-limit-1-in-ActiveRecord-Base-find_initial.patch (855 Bytes)

У Чцв, 24/04/2008 у 10:45 +0100, Frederick Cheung піша:

> > У Чцв, 24/04/2008 у 10:04 +0100, Frederick Cheung піша: >> I stumbled across this today: >> >> def find_initial(options) >> options.update(:limit => 1) unless options[:include] >> find_every(options).first >> end > Several month ago I asked same question. I didn't searched history,
> but > I guess it's artifact of some very ancient times. > > It works perfectly without this (actually, with :include it always > fetches whole result set and cuts it in memory, which is stupid). Pretty much what I thought. I can't quite think how I'd write a test
to verify that it's not doing this though.

It seems, only mocking can help.

What happens when you specify a :conditions hash that relies on the included association?

У Чцв, 24/04/2008 у 20:26 -0700, Xavier Shay піша:

What happens when you specify a :conditions hash that relies on the included association?

It's not very obvious, but it's supported. Whole bunch of code is there to support it (and according to git history it was there since late 2005 (svn r2675). Look at ActiveRecord::Associations::ClassMethods#add_limited_ids_condition!

Not adding limit was commited in r1811 along with initial support for :limit and :include. Actually, even then it could be smarter. I.e. :limit => 1 could be left if all included associations are :belong_to or :has_one.

Then when proper support was implemented, somebody simply forgot to remove it.

Then when proper support was implemented, somebody simply forgot to remove it.

Thanks for looking into that. I've checked and we have what *looks* like fairly decent coverage in eager_test, so I've removed it.

Cheers!