Proper Encapsulation of SQL WHERE / ORDER BY Clauses

Testing RSpec
1

Hi TDD Fans,

I’m pretty new to Ruby / RSpec / Rails but not to TDD.

This is more of a general ‘how do you do good design in a rails app’
question than an rspec-specific question. I’m asking it here because
I know this list is read by lots of people who care about good
design, but please feel free to point me somewhere else if you think
it’s not relevant to this list.

Probably through my inexperience with the language / framework, I’m
finding that I’m tending to clutter my controllers with SQL-specific
stuff.

e.g.

 def get_cities

   City.paginate(:all, get_find_params.merge!( :page => params

[:page] ))

 end

 def get_find_params

   find_params = { :order => get_order_clause }

   if params[:name] || params[:last_24]
     find_params.merge! :conditions => get_conditions
   end

   return find_params

 end

 def get_conditions
   "name like '%#{params[:name]}%'" + (params[:last_24] ? " AND

created_at >= ‘#{DateTime.now - 1.days}’" : “”)
end

 def get_order_clause
   (params[:sort] ? 'created_at DESC, ' : "") + 'name ASC'
 end

This is obviously horribly brittle to write specs for, but I’m not
really sure what I should do instead…

How do I get my models to encapsulate this stuff, especially given
I’m using the will_paginate plug-in?

Any tips / pointers greatly appreciated.

cheers,
Matt

http://blog.mattwynne.net

2

On Fri, Aug 15, 2008 at 5:28 AM, Matt W. [email protected] wrote:

def get_cities
  end
def get_order_clause
  (params[:sort] ? 'created_at DESC, ' : "") + 'name ASC'
end

This is obviously horribly brittle to write specs for, but I’m not really
sure what I should do instead…
How do I get my models to encapsulate this stuff, especially given I’m using
the will_paginate plug-in?
Any tips / pointers greatly appreciated.
cheers,
Matt

Hey Matt - welcome!

The paginate() method lives on the model class, so there’s nothing
stopping you from wrapping those calls in methods on the model,
slinging around the params object.

CityController

def get_cities
City.paginate_all(params)
end

City

def self.paginate_all(params)
self.paginate(:all, get_find_params(params).merge!(:page =>
params[:page]))
end

etc

HTH,
David

3

On 15 Aug 2008, at 12:25, David C. wrote:

end

City

def self.paginate_all(params)
self.paginate(:all, get_find_params(params).merge!(:page => params
[:page]))
end

etc

Aha. Cool, thanks.

For my next question: how do I go about driving out change to the
model, spec-first?

I’m thinking I would call (in my spec)

City.should_receive(:paginate).with(:conditions => “name like ‘%#
{test_params[:name}%’” … )
City.paginate_all(test_params)

Thereby covering the code in get_find_params()

Is that the right approach?

cheers,
Matt

4

On Aug 15, 2008, at 6:46 AM, Matt W. [email protected] wrote:

def get_cities
etc
‘%#{test_params[:name}%’" … )
City.paginate_all(test_params)

Thereby covering the code in get_find_params()

Is that the right approach?

That’s probably how I would do it. Might also consider wrapping the
params in a separate object that manages the extraction.

David

5

On Aug 15, 2008, at 9:29 AM, David C. wrote:

CityController

end

City.should_receive(:paginate).with(:conditions => “name like
‘%#{test_params[:name}%’” … )
City.paginate_all(test_params)

Thereby covering the code in get_find_params()

Is that the right approach?

That’s probably how I would do it. Might also consider wrapping the
params in a separate object that manages the extraction.

That’s how I’ve started doing it - putting sql statements in a module:

This allows me to test the sql statements seperately from the actual
finder.

Also - just to give you the heads up - You should almost never use
literal string substitutions in sql statements - it allows for sql
injection attacks:

Best,

Scott T.

6

Thanks Scott. I refactored it today to use what I called a
QueryAdapter, namespaced inside the model. It basically subclasses
Hash, takes the params from the controller into the constructor, and
becomes the hash to be sent to find_all.

I feels much better, as I now have the code that’s coupled to the
database in one place, but I’d welcome feedback:

 # VenuesController
 def get_venues
   Venue.paginate( :all, Venue::QueryAdapter.new(params) )
 end

 # Responsible for mapping a hash of parameters that will

typically be POSTed to a controller into a hash that can be sent to
find(:all)
# containing SQL clauses in :conditions / :order.
# This helps us decouple the view / controller layers from any
database specific stuff.
class Venue::QueryAdapter < Hash

   def initialize(params)

     parse params

     self.merge!(get_find_params)

   end

   private

   def parse(params)
     @sort_column = params[:sort]
     @city_id = params[:city_id]
     @name = params[:name]
     @page = params[:page]
   end

   def get_find_params

     find_params = {}

     find_params.merge!( :order      => get_order_clause ) if

get_order_clause.length > 0
find_params.merge!( :conditions => get_where_clause ) if
get_where_clause.length > 0
find_params.merge!( :page => @page )

     return find_params

   end

   def get_where_clause

     clause = []

     clause << "city_id = #{@city_id}" if @city_id
     clause << "name like '%#{@name}%'" if @name

     return clause.join(" AND ")

   end

   def get_order_clause

     clause = []

     clause << 'created_on DESC' if @sort_column == 'created_at'
     clause << 'name ASC'

     return clause.join(", ")

   end

 end

cheers,
Matt

http://blog.mattwynne.net

In case you wondered: The opinions expressed in this email are my own
and do not necessarily reflect the views of any former, current or
future employers of mine.

7

On Mon, Aug 18, 2008 at 1:26 PM, Matt W. [email protected] wrote:

 def get_where_clause

   clause = []

   clause << "city_id = #{@city_id}" if @city_id
   clause << "name like '%#{@name}%'" if @name

I think you’ve still got SQL injection problems here.

///ark

8

Thanks for the reminder. This stuff is in a protected admin area so I
don’t really care, but I should play on the safe side anyhow.

cheers,
Matt

http://blog.mattwynne.net

In case you wondered: The opinions expressed in this email are my own
and do not necessarily reflect the views of any former, current or
future employers of mine.