Rails - Nov 2009 - How can I improve a Ruby on Rails code that hast a lot of SQL as strings?

Hello Railists,

I have a piece of Ruby on Rails code that has a complex SQL query (well, not
that complex, but as far as I know beyond the ORM capabilities) and for my
taste it has too many strings and harcoded values. I''d like to improve
it as
much as possible, so my question is open ended, what else can I do to
improve it?

Some particular issues I have

- Is there a way to get a table name to use it in a query in the same
escaped way as the ORM does? I think this is database independent,
being `items` for MySQL but not for other databases.
- In the same vein, is there a way to get a field name the same way
Rail''s
ORM would put it in a SQL query?
- Maybe there''s a way to get both, the table name and the field name in
one
operation. I''m imaging something like Item.voteable_id.for_query
=> "`items`.`voteable`".
- How do I escape code to avoid SQL injection when not in conditions?
I''m
using the user_id variable directly in a query and although it''s
impossible
for a user to put anything in it, I''d rather escape it properly. In a
condition I would do [''user_id = ?'', user_id], but in a join
or a select,
how do I do it?
- Does my use of class constants here make sense?
- Is there any chance at all of using the ORM and less string?
- Any other thing to do to it?

The code is this one

class Item < ActiveRecord::Base
  has_many :votes, :as => :voteable

  def self.ranking(user_id)
    Item.find(:all,
      # items.* for all the Item attributes, score being the sum of votes,
user_vote is the vote of user_id (0 if no vote) and voter_id is just user_id
for latter reference.
      :select => "items.*,
                  IFNULL(sum(all_votes.value), 0) as score,
                  user_votes.value as user_vote,
                  \"#{user_id}\" as voter_id",
      # The first join gets all the votes for a single item (to be summed
latter).
      # The second join gets the vote for a single user for a single item.
      :joins => ["LEFT JOIN votes as all_votes ON
                    all_votes.voteable_id = items.id and
                    all_votes.voteable_type = \"Item\"",
                 "LEFT JOIN votes as user_votes ON
                    user_votes.voteable_id = items.id and
                    user_votes.user_id = \"#{user_id}\" and
                    user_votes.voteable_type = \"Item\""
                ],
      :group => :id,
      :order => "score DESC")

    # This is the query it should generate
    # SELECT items.*, user_votes.value as user_vote, sum(all_votes.value) as
score
    # FROM items
    # LEFT JOIN votes as all_votes ON
    #     all_votes.voteable_id = items.id and
    #     all_votes.voteable_type = "Item"
    # LEFT JOIN votes as user_votes ON
    #     user_votes.voteable_id = items.id and
    #     user_votes.user_id = 2 and
    #     user_votes.voteable_type = "Item"
    # GROUP BY items.id
    # ORDER BY score DESC
  end

  def score
    s = read_attribute("score")
    if s == nil
      votes.sum :value
    else
      Integer(s)
    end
  end

  def user_vote(user_id)
    if Integer(read_attribute("voter_id")) == user_id
      Integer(read_attribute("user_vote"))
    else
      vote = votes.find(:first, :conditions => ["user_id = ?",
user_id])
      if vote
        vote.value
      else
        0
      end
    end
  end
end

Thanks.
--
J. Pablo Fernández <pupeno-GAtDADarczzQT0dZR+AlfA@public.gmane.org>
(http://pupeno.com)

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

check out the acts as voteable plugin on github, looks like you''ve
already
got your modeling done correctly to just drop it in.

-C
2009/11/8 J. Pablo Fernández
<pupeno-GAtDADarczzQT0dZR+AlfA@public.gmane.org>
> Hello Railists,
>
> I have a piece of Ruby on Rails code that has a complex SQL query (well,
> not that complex, but as far as I know beyond the ORM capabilities) and for
> my taste it has too many strings and harcoded values. I''d like to
improve it
> as much as possible, so my question is open ended, what else can I do to
> improve it?
>
> Some particular issues I have
>
> - Is there a way to get a table name to use it in a query in the same
> escaped way as the ORM does? I think this is database independent,
> being `items` for MySQL but not for other databases.
> - In the same vein, is there a way to get a field name the same way
Rail''s
> ORM would put it in a SQL query?
> - Maybe there''s a way to get both, the table name and the field
name in one
> operation. I''m imaging something like Item.voteable_id.for_query
> => "`items`.`voteable`".
> - How do I escape code to avoid SQL injection when not in conditions?
I''m
> using the user_id variable directly in a query and although it''s
impossible
> for a user to put anything in it, I''d rather escape it properly.
In a
> condition I would do [''user_id = ?'', user_id], but in a
join or a select,
> how do I do it?
> - Does my use of class constants here make sense?
> - Is there any chance at all of using the ORM and less string?
> - Any other thing to do to it?
>
> The code is this one
>
> class Item < ActiveRecord::Base
>   has_many :votes, :as => :voteable
>
>   def self.ranking(user_id)
>     Item.find(:all,
>       # items.* for all the Item attributes, score being the sum of votes,
> user_vote is the vote of user_id (0 if no vote) and voter_id is just
user_id
> for latter reference.
>       :select => "items.*,
>                   IFNULL(sum(all_votes.value), 0) as score,
>                   user_votes.value as user_vote,
>                   \"#{user_id}\" as voter_id",
>       # The first join gets all the votes for a single item (to be summed
> latter).
>       # The second join gets the vote for a single user for a single item.
>       :joins => ["LEFT JOIN votes as all_votes ON
>                     all_votes.voteable_id = items.id and
>                     all_votes.voteable_type = \"Item\"",
>                  "LEFT JOIN votes as user_votes ON
>                     user_votes.voteable_id = items.id and
>                     user_votes.user_id = \"#{user_id}\" and
>                     user_votes.voteable_type = \"Item\""
>                 ],
>       :group => :id,
>       :order => "score DESC")
>
>     # This is the query it should generate
>     # SELECT items.*, user_votes.value as user_vote, sum(all_votes.value)
> as score
>     # FROM items
>     # LEFT JOIN votes as all_votes ON
>     #     all_votes.voteable_id = items.id and
>     #     all_votes.voteable_type = "Item"
>     # LEFT JOIN votes as user_votes ON
>     #     user_votes.voteable_id = items.id and
>     #     user_votes.user_id = 2 and
>     #     user_votes.voteable_type = "Item"
>     # GROUP BY items.id
>     # ORDER BY score DESC
>   end
>
>   def score
>     s = read_attribute("score")
>     if s == nil
>       votes.sum :value
>     else
>       Integer(s)
>     end
>   end
>
>   def user_vote(user_id)
>     if Integer(read_attribute("voter_id")) == user_id
>       Integer(read_attribute("user_vote"))
>     else
>       vote = votes.find(:first, :conditions => ["user_id = ?",
user_id])
>       if vote
>         vote.value
>       else
>         0
>       end
>     end
>   end
> end
>
> Thanks.
> --
> J. Pablo Fernández <pupeno-GAtDADarczzQT0dZR+AlfA@public.gmane.org>
(http://pupeno.com)
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

J. Pablo Fernández wrote:
> Hello Railists,
> 
> I have a piece of Ruby on Rails code that has a complex SQL query (well, 
> not
> that complex, but as far as I know beyond the ORM capabilities) and for 
> my
> taste it has too many strings and harcoded values. I''d like to
improve
> it as
> much as possible, so my question is open ended, what else can I do to
> improve it?
In this case, break it into two.  See below.
> 
> Some particular issues I have
> 
> - Is there a way to get a table name to use it in a query in the same
> escaped way as the ORM does? I think this is database independent,
> being `items` for MySQL but not for other databases.
> - In the same vein, is there a way to get a field name the same way 
> Rail''s
> ORM would put it in a SQL query?
> - Maybe there''s a way to get both, the table name and the field
name in
> one
> operation. I''m imaging something like Item.voteable_id.for_query
> => "`items`.`voteable`".
Check out quote_table_name and quote_column_name.
> - How do I escape code to avoid SQL injection when not in conditions? 
> I''m
> using the user_id variable directly in a query and although it''s 
> impossible
> for a user to put anything in it, I''d rather escape it properly.
In a
> condition I would do [''user_id = ?'', user_id], but in a
join or a
> select,
> how do I do it?
You really shouldn''t be using user_id in a join as you''re
doing in the
query below -- it belongs in a WHERE clause.  And at that point, 
find_by_sql will handle it.  Check the docs.
> - Does my use of class constants here make sense?
You''re not using class constants.
> - Is there any chance at all of using the ORM and less string?
Yes.  See my suggestions below.
> - Any other thing to do to it?
> 
> The code is this one
> 
> class Item < ActiveRecord::Base
>   has_many :votes, :as => :voteable
> 
>   def self.ranking(user_id)
>     Item.find(:all,
>       # items.* for all the Item attributes, score being the sum of 
> votes,
> user_vote is the vote of user_id (0 if no vote) and voter_id is just 
> user_id
> for latter reference.
>       :select => "items.*,
>                   IFNULL(sum(all_votes.value), 0) as score,
>                   user_votes.value as user_vote,
>                   \"#{user_id}\" as voter_id",
>       # The first join gets all the votes for a single item (to be 
> summed
> latter).
>       # The second join gets the vote for a single user for a single 
> item.
>       :joins => ["LEFT JOIN votes as all_votes ON
>                     all_votes.voteable_id = items.id and
>                     all_votes.voteable_type = \"Item\"",
>                  "LEFT JOIN votes as user_votes ON
>                     user_votes.voteable_id = items.id and
>                     user_votes.user_id = \"#{user_id}\" and
>                     user_votes.voteable_type = \"Item\""
>                 ],
>       :group => :id,
>       :order => "score DESC")
> 
>     # This is the query it should generate
>     # SELECT items.*, user_votes.value as user_vote, 
> sum(all_votes.value) as
> score
>     # FROM items
>     # LEFT JOIN votes as all_votes ON
>     #     all_votes.voteable_id = items.id and
>     #     all_votes.voteable_type = "Item"
>     # LEFT JOIN votes as user_votes ON
>     #     user_votes.voteable_id = items.id and
>     #     user_votes.user_id = 2 and
>     #     user_votes.voteable_type = "Item"
>     # GROUP BY items.id
>     # ORDER BY score DESC
>   end
I think your logic is poor.  Although it''s tempting, it feels wrong to 
join both all the votes (in the first join) and a subset of votes (in 
the second join).  I usually advocate one big query, but in this case I 
think it should probably be two queries:
Vote.sum(:value, :conditions => {:voteable_type =>
''Item''}, :group =>
''voteable_id'', :joins => ''left join items on
(item.id = voteable_id)'',
:order => ''sum(value) desc'')
Item.find(:all, :joins => :votes, :conditions => {:votes => {:user_id
=>
user_id}})

Best,
--
Marnen Laibow-Koser
http://www.marnen.org
marnen-sbuyVjPbboAdnm+yROfE0A@public.gmane.org
-- 
Posted via http://www.ruby-forum.com/.

Hello Chris,

I''ve checked. There was one little thing the act as votable
couldn''t do; I
can''t remember exactly what, maybe the negative votes or something like
that. So I did my own.

But what I''m really after is improving the so many strings.

On Mon, Nov 9, 2009 at 04:36, Chris Drappier
<chris.drappier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>wrote:
> check out the acts as voteable plugin on github, looks like you''ve
already
> got your modeling done correctly to just drop it in.
>
> -C
> 2009/11/8 J. Pablo Fernández
<pupeno-GAtDADarczzQT0dZR+AlfA@public.gmane.org>
>
> Hello Railists,
>>
>> I have a piece of Ruby on Rails code that has a complex SQL query
(well,
>> not that complex, but as far as I know beyond the ORM capabilities) and
for
>> my taste it has too many strings and harcoded values. I''d like
to improve it
>> as much as possible, so my question is open ended, what else can I do
to
>> improve it?
>>
>> Some particular issues I have
>>
>> - Is there a way to get a table name to use it in a query in the same
>> escaped way as the ORM does? I think this is database independent,
>> being `items` for MySQL but not for other databases.
>> - In the same vein, is there a way to get a field name the same way
Rail''s
>> ORM would put it in a SQL query?
>> - Maybe there''s a way to get both, the table name and the
field name in
>> one operation. I''m imaging something like
Item.voteable_id.for_query
>> => "`items`.`voteable`".
>> - How do I escape code to avoid SQL injection when not in conditions?
I''m
>> using the user_id variable directly in a query and although
it''s impossible
>> for a user to put anything in it, I''d rather escape it
properly. In a
>> condition I would do [''user_id = ?'', user_id], but in
a join or a select,
>> how do I do it?
>> - Does my use of class constants here make sense?
>> - Is there any chance at all of using the ORM and less string?
>> - Any other thing to do to it?
>>
>> The code is this one
>>
>> class Item < ActiveRecord::Base
>>   has_many :votes, :as => :voteable
>>
>>   def self.ranking(user_id)
>>     Item.find(:all,
>>       # items.* for all the Item attributes, score being the sum of
votes,
>> user_vote is the vote of user_id (0 if no vote) and voter_id is just
user_id
>> for latter reference.
>>       :select => "items.*,
>>                   IFNULL(sum(all_votes.value), 0) as score,
>>                   user_votes.value as user_vote,
>>                   \"#{user_id}\" as voter_id",
>>       # The first join gets all the votes for a single item (to be
summed
>> latter).
>>       # The second join gets the vote for a single user for a single
item.
>>       :joins => ["LEFT JOIN votes as all_votes ON
>>                     all_votes.voteable_id = items.id and
>>                     all_votes.voteable_type = \"Item\"",
>>                  "LEFT JOIN votes as user_votes ON
>>                     user_votes.voteable_id = items.id and
>>                     user_votes.user_id = \"#{user_id}\" and
>>                     user_votes.voteable_type = \"Item\""
>>                 ],
>>       :group => :id,
>>       :order => "score DESC")
>>
>>     # This is the query it should generate
>>     # SELECT items.*, user_votes.value as user_vote,
sum(all_votes.value)
>> as score
>>     # FROM items
>>     # LEFT JOIN votes as all_votes ON
>>     #     all_votes.voteable_id = items.id and
>>     #     all_votes.voteable_type = "Item"
>>     # LEFT JOIN votes as user_votes ON
>>     #     user_votes.voteable_id = items.id and
>>     #     user_votes.user_id = 2 and
>>     #     user_votes.voteable_type = "Item"
>>     # GROUP BY items.id
>>     # ORDER BY score DESC
>>   end
>>
>>   def score
>>     s = read_attribute("score")
>>     if s == nil
>>       votes.sum :value
>>     else
>>       Integer(s)
>>     end
>>   end
>>
>>   def user_vote(user_id)
>>     if Integer(read_attribute("voter_id")) == user_id
>>       Integer(read_attribute("user_vote"))
>>     else
>>       vote = votes.find(:first, :conditions => ["user_id =
?", user_id])
>>       if vote
>>         vote.value
>>       else
>>         0
>>       end
>>     end
>>   end
>> end
>>
>> Thanks.
>> --
>> J. Pablo Fernández
<pupeno-GAtDADarczzQT0dZR+AlfA@public.gmane.org> (http://pupeno.com)
>>
>>
>>
>
> >
>

-- 
J. Pablo Fernández <pupeno-GAtDADarczzQT0dZR+AlfA@public.gmane.org>
(http://pupeno.com)

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---