Rails’ find_by_sql is the devil. Ninety nine percent of the time find_by_sql is unnecessary and problematic, but it’s sooo seductive. I can’t even begin to count the ways that find_by_sql can cause trouble, but here’s a few:
- Plugins like acts_as_paranoid rely on developers *not* using the back door to get around the dynamic conditions to exclude deleted rows.
- There quite a few gotchas, ie: “SELECT * FROM users JOIN another_table …” won’t work because ActiveRecord will use the last ID field, not the first.
- Logic “hidden” in find_by_sql is not reusable (as compared to a fancy association, etc)
- It offends my aesthetic sense. We all like to pretend our ORM layer isn’t leaky.. don’t we?
I remember a time when we had to use find_by_sql when we found a feature was missing from ActiveRecord. I propose that the only remaining legitimate use of ActiveRecord may be to call a stored procedure.
So, keeping with the hyperactivity theme, I suggest that we all take some ActiveRitalin: A plugin for Rails that tells ActiveRecord to chill out, and causes developers to sit and think a bit before proceeding.
ActiveRitalin makes find_by_sql private and introduces find_by_sql_with_excuse that warns upon each use. The implementation is very simple:
module ActiveRitalin
def self.append_features(klass)
super
klass.class_eval %(
def self.find_by_sql_with_excuse(excuse, sql)
RAILS_DEFAULT_LOGGER.warn("Find By Sql called with excuse: " + excuse )
find_by_sql(sql)
end
class << self
private :find_by_sql
end
)
end
end
To use, from your rails application’s root dir:
script/plugin install git://github.com/lukegalea/activeritalin.git
Think you need find_by_sql? Ask yourself the following questions:
- Can I just use :include, :select, :join, :conditions or some combination of the above?
- Should this be an association? (perhaps with :conditions and :select on it? Maybe :readonly?)
11 comments ↓
Ok, I get your point. But… I tend to do a fair amount of statistical correlations and I could load those monster data sets into AR and enumerate through them, but do I really need the AR memory and CPU hit? Not with a quick and speedy find_by_sql to get me the number I want. RDBMS are good for this, so I let them do what they do best.
I’m not talking about your bread-and-butter CRUD ops, but try do to a simple sum across three levels of a join with AR. No can do. So I use a regular find to get the record data I need, and a sprinkle of find_by_sql to get the sums. And it’s fast, too.
I’m sorry, but I need my find_by_sql.
Good call. Definately want the RDBMS to do that stuff.
I know this is going against the Rails mantras, but I usually use a view for that stuff.
Something like:
has_many :house_statistics, :class => ‘House’, :table => ’some_view’, :readonly => true
I really like Luke’s view suggestion. Some DBs also allow indexed views, which could increase your performance even more.
Also, even if you use find_by_sql, I always consider it good practice to leave a comment above the code saying *why* I needed to use it there (e.g. “# PERF: used 30MB of memory just to load 100 records the normal way!!! this takes 300KB.” or “# PERF: took 30sec for 15K records using AR. takes 0.2sec using this”. find_by_sql_with_excuse simply “forces” you to think about it and let others know why you made that decision.
Perhaps, if you wanted to be less opinionated about it, you could alias it to “find_by_sql_with_explanation”. (similar to how svn has “blame”, “annotate”, and “praise”). But I like the opinionated version.
Good conversation, I hope more join in.
I’ll stand against the wind here and say that I’ve never been a big fan of views. Let me explain:
1) I see the value of views if multiple DBAs or inexperienced users are using a database. But what I do rarely has more than one DBA, me.
2) I like to see my select statements right there around the lines I’m working on. That way, three months later, I don’t have to go chasing the CREATE VIEW statements down to recall exactly what they are doing.
3) I put my find_by_sql in the model (where they belong) so that it pretty much like same as creating a view, and I don’t have to create all the views. Indexed views not withstanding.
But, I really like Nicholas’s idea of commenting each find_by_sql statement with specific reasons why it is being used. Bravo!
I have never used views in a rails project. Just out of curiosity, where do you put your CREATE VIEW statements, in the migrations?
I agree, if you are a team of one and you are using a DB without indexed views (or don’t need the performance) then it doesn’t make a big difference.
I’d be worried that some day you won’t be the only one or that later you may find the need to migrate to “better” DB.
It’s only marginally more effort to make a view and then you don’t have to worry about refactoring everything later if you need to scale up the team/technology.
We put our views in the migrations and I believe we fiddled with the schema dumper to have it written out into schema.rb as well.
Karl - perhaps what would meet your use_case is a version of active_ritalin that makes find_by_sql protected, so you can call it from within your models but not from your controller or (GASP!) views.
Luke: even with find_by_sql as private, you can still call it from within the models.
If you wanted to kick active_ritalin up a notch (or down a notch, depending on your POV), you could instead:
* capture the find_by_sql method as an (unbound method) object
* define_method :find_by_sql_with_excuse, using the captured find_by_sql method.
* make both find_by_sql and find_by_sql_with_excuse protected (so it can only be used from within the model class)
* redefine find_by_sql to ‘raise FindBySqlWithoutExplanation’
Then find_by_sql without an excuse is gone for good, and even with an explanation it can only be called from within the model.
What do you think?
Is there code within the rail framework that depends on the existence of find_by_sql and would break if you did this?
I tried something similar (although less complex) originally and found that most of the ActiveRecord::Base methods ultimately called find_by_sql.
So, we need to make sure Base methods can call it. But limiting public access and access from within the descendants of ActiveRecord::Base is what we’re after.
It’s too bad ruby’s private doesn’t work like most language’s private:
>> class Parent; private; def foo; puts “in private”; end; end
=> nil
>> class Child nil
>> Child.new.call_foo
in private
=> nil
>> Parent.new.foo
NoMethodError: private method `foo’ called for #
from (irb):4
hmmm… It looks like the less-than sign used on the subclass (and the rest of the subclass definition) was eaten by your blog. This is why every blog should have a “preview” option.
[...] ar-extensions with the duplicate key error update trick. Some colleagues of mine, who invented a plugin to invalidate find_by_sql unless you provide an excuse for each use, would put it more strongly than [...]
You must log in to post a comment.