Rails - Jan 2006 - Would someone like to tell me why this code will not solve my problem? (it''s short)

I am building an invoicing system but cannot use the auto_increment  
field to determine the invoice number (because they are running 3  
different companies off the one system.  I need to find the last  
invoice number from any given company and then add 1 to it to get the  
next invoice number.  BUT, there is a unique case on the very first  
invoice produced because there is no earlier invoice number.  My  
solution is as follows (and does not work)

This is in the ''create'' controller


     if Invoice.find(:all)
       @temp = Invoice.find_by_sql("select max(inv_number) as inv_number
                               from invoices where company_id = params 
[:company_id")
       @invoice.inv_number = @temp[0].inv_number +1
     else
       @invoice.inv_number=1
     end



The error I am getting is

NoMethodError in Invoices#create

You have a nil object when you didn''t expect it!
You might have expected an instance of Array.
The error occured while evaluating nil.+
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://wrath.rubyonrails.org/pipermail/rails/attachments/20060103/5346b923/attachment.html

bruce balmer wrote:
> 
> if Invoice.find(:all)
> @temp = Invoice.find_by_sql("select max(inv_number) as inv_number
>                            from invoices where company_id 
>                            = params[:company_id")
This line might be part of the problem.  the "params[:company_id]" is 
being inserted literally into your sql string.  You need to do something 
like

... = #{params[:company_id]}" to insert it.

WARNING... this is highly unsafe.  A user could manually pass all sorts 
of badness through the company_id param and totally hose you.  Please 
find some other (secure) way to do this.  The [''company_id =
?'',
params[:company_id]] approach is better, but I''m not sure how to do
this
with the find_by_sql function.

By the way,  you could do something like..

Invoice.find(:first, :condition=>["company_id = ?", 
params[:company_id]], :order=>"inv_number DESC")

This should give you the record with the highest invoice number.

_Kevin

-- 
Posted via http://www.ruby-forum.com/.

On Mon, Jan 02, 2006 at 09:10:47PM -0700, Bruce Balmer
wrote:
>    This is in the ''create'' controller
Invoice.find(:all) will always be boolean true. If there are no Invoices,
then find(:all) will return an empty array ([]), which is always boolean
true.

A way of expressing your intentions is:
  
  unless Invoice.count.zero?
>        if Invoice.find(:all) 
  params[:company_id] is not being interpolated. It''s being passed
along as a
literal value. String interpolation in Ruby is done with #{}. For example:

  foo = ''Marcel''
  "My name is #{foo}"
  => "My name is Marcel"

The following find_by_sql, makes you vulnerable to SQL injection. Rather than
using find_by_sql, you could use find, passing the :select and the
:conditions options. Something like:

  Invoice.find(:first, :select => ''MAX(inv_number) as
inv_number'', :conditions => [''company_id = ?'',
params[:company_id]).inv_number.to_i

But there is no need to do this MAX business. You can instead use
ActiveRecord''s increment or increment! or increment_counter method:

http://api.rubyonrails.com/classes/ActiveRecord/Base.html#M000768
http://api.rubyonrails.com/classes/ActiveRecord/Base.html#M000727

Increment allows you to avoid the entire conditional.
>          @temp = Invoice.find_by_sql("select max(inv_number) as
inv_number
>                                  from invoices where company_id >   
params[:company_id")
>          @invoice.inv_number = @temp[0].inv_number +1
>        else
>          @invoice.inv_number=1
>        end
marcel
-- 
Marcel Molina Jr. <marcel@vernix.org>

On 3.1.2006, at 6.55, Marcel Molina Jr. wrote:
>
> But there is no need to do this MAX business. You can instead use
> ActiveRecord''s increment or increment! or increment_counter
method:
>
> http://api.rubyonrails.com/classes/ActiveRecord/Base.html#M000768
> http://api.rubyonrails.com/classes/ActiveRecord/Base.html#M000727
>
> Increment allows you to avoid the entire conditional.
Also keep in mind that selecting max() from a table is NOT a  
transaction safe method. If you have two users inserting an invoice  
concurrently, they might end up with the same numbe. That''s why many  
database management systems have sequences [1].

//jarkko

[1] http://www.postgresql.org/docs/8.1/interactive/sql- 
createsequence.html

--
Jarkko Laine
http://jlaine.net
http://odesign.fi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2363 bytes
Desc: not available
Url :
http://wrath.rubyonrails.org/pipermail/rails/attachments/20060103/2112814f/smime.bin

Marcel:

Thanks very much.  That was awesome!

bruce


On 2-Jan-06, at 9:55 PM, Marcel Molina Jr. wrote:
> On Mon, Jan 02, 2006 at 09:10:47PM -0700, Bruce Balmer wrote:
>>    This is in the ''create'' controller
>
> Invoice.find(:all) will always be boolean true. If there are no  
> Invoices,
> then find(:all) will return an empty array ([]), which is always  
> boolean
> true.
>
> A way of expressing your intentions is:
>
>   unless Invoice.count.zero?
>
>>        if Invoice.find(:all)
>
>   params[:company_id] is not being interpolated. It''s being passed
> along as a
> literal value. String interpolation in Ruby is done with #{}. For  
> example:
>
>   foo = ''Marcel''
>   "My name is #{foo}"
>   => "My name is Marcel"
>
> The following find_by_sql, makes you vulnerable to SQL injection.  
> Rather than
> using find_by_sql, you could use find, passing the :select and the
> :conditions options. Something like:
>
>   Invoice.find(:first, :select => ''MAX(inv_number) as  
> inv_number'', :conditions => [''company_id =
?'', params
> [:company_id]).inv_number.to_i
>
> But there is no need to do this MAX business. You can instead use
> ActiveRecord''s increment or increment! or increment_counter
method:
>
> http://api.rubyonrails.com/classes/ActiveRecord/Base.html#M000768
> http://api.rubyonrails.com/classes/ActiveRecord/Base.html#M000727
>
> Increment allows you to avoid the entire conditional.
>
>>          @temp = Invoice.find_by_sql("select max(inv_number) as  
>> inv_number
>>                                  from invoices where company_id
>>    params[:company_id")
>>          @invoice.inv_number = @temp[0].inv_number +1
>>        else
>>          @invoice.inv_number=1
>>        end
>
> marcel
> -- 
> Marcel Molina Jr. <marcel@vernix.org>
> _______________________________________________
> Rails mailing list
> Rails@lists.rubyonrails.org
> http://lists.rubyonrails.org/mailman/listinfo/rails