rspec users - Aug 2008 - Proper Encapsulation of SQL WHERE / ORDER BY Clauses

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
http://songkick.com

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20080815/8647e5e3/attachment-0001.html>

On Fri, Aug 15, 2008 at 5:28 AM, Matt Wynne <matt at mattwynne.net>
wrote:
> 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
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

On 15 Aug 2008, at 12:25, David Chelimsky wrote:
> 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
>
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

On Aug 15, 2008, at 6:46 AM, Matt Wynne <matt at mattwynne.net> wrote:
> On 15 Aug 2008, at 12:25, David Chelimsky wrote:
>
>> 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
>>
>
> 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?
That''s probably how I would do it. Might also consider wrapping the  
params in a separate object that manages the extraction.

David
>
>
> cheers,
> Matt
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users

On Aug 15, 2008, at 9:29 AM, David Chelimsky wrote:
> On Aug 15, 2008, at 6:46 AM, Matt Wynne <matt at mattwynne.net>
wrote:
>
>> On 15 Aug 2008, at 12:25, David Chelimsky wrote:
>>
>>> 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
>>>
>>
>> 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?
>
> 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:

http://gist.github.com/5675

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:

http://en.wikipedia.org/wiki/Sql_injection

Best,

Scott Taylor

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
http://songkick.com

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.

On 16 Aug 2008, at 00:19, Scott Taylor wrote:
>
> On Aug 15, 2008, at 9:29 AM, David Chelimsky wrote:
>
>> On Aug 15, 2008, at 6:46 AM, Matt Wynne <matt at mattwynne.net>
wrote:
>>
>>> On 15 Aug 2008, at 12:25, David Chelimsky wrote:
>>>
>>>> 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
>>>>
>>>
>>> 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?
>>
>> 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:
>
> http://gist.github.com/5675
>
> 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:
>
> http://en.wikipedia.org/wiki/Sql_injection
>
> Best,
>
> Scott Taylor
>
>
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users

On Mon, Aug 18, 2008 at 1:26 PM, Matt Wynne <matt at mattwynne.net> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20080818/0e73f1f2/attachment.html>

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
http://songkick.com

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.

On 18 Aug 2008, at 22:18, Mark Wilden wrote:
> On Mon, Aug 18, 2008 at 1:26 PM, Matt Wynne <matt at mattwynne.net>  
> 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
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20080819/addb16d4/attachment.html>