Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• 

  Tarmo Tänav October 7th, 2008 @ 03:45 AM

  • State changed from “new” to “wontfix”

  I don't think this is going to happen, it's too dangerous to assume anything about the string that we replace variables in and we don't really want to start parsing SQL either.

  You can however use :conditions => { :id => nil } and it should generate the proper comparison, be it a scalar, array or nil.

• You flagged this item as spam.
  

  Phillip Koebbe October 7th, 2008 @ 03:36 PM

  Really? But this is just plain wrong SQL. The expression "= NULL" will always fail as nothing is equal to NULL.

  A couple of years ago, an enhancement was made to fix this behavior in dynamic finders, but since that time, the dynamic finder code has been simplified greatly to rely on other code. See

  http://github.com/rails/rails/co...

  If this really is the opinion of core, I have to accept it, but my opinion is that proper SQL should be generated in this case and it's not that hard to do.

  Peace.

• 

  Michael Koziarski October 7th, 2008 @ 03:54 PM

  It's actually quite a bit harder to do than it seems at first, and we have tried this in the past, with the sql server adapter I believe.

  However as tarmo mentioned, the :conditions=>{:id=>nil} case already handles this and is your best bet here.

• You flagged this item as spam.
  

  Phillip Koebbe October 7th, 2008 @ 04:00 PM

  Okie dokie. Thanks for the consideration.

  Peace.

• You flagged this item as spam.
  

  Phillip Koebbe October 7th, 2008 @ 04:18 PM

  For posterity's sake:

  Digging into it, I think I understand the difficulty. The method in question appears to be

  
          def replace_named_bind_variables(statement, bind_vars) #:nodoc:
            statement.gsub(/(:?):([a-zA-Z]\w*)/) do
              if $1 == ':' # skip postgresql casts
                $& # return the whole match
              elsif bind_vars.include?(match = $2.to_sym)
                quote_bound_value(bind_vars[match])
              else
                raise PreparedStatementInvalid, "missing value for :#{match} in #{statement}"
              end
            end
          end
  

  Using the gsub block approach allows one to easily coarse through the SQL fragment replacing all symbol-looking things. If one were to try to replace an = that exists in statement, the fragment would have to be ripped apart, chunk by chunk. And since there could be combinations of AND, OR, parenthesis, etc, it would become quite complex quite quickly. And since there is an alternative, there's no real benefit to adding such complexity.

  Peace.

• You flagged this item as spam.
  

  Phillip Koebbe October 8th, 2008 @ 09:17 PM

  For my own curiosity (or pain, as the case may be), I couldn't let go of this, so I twiddled around until I came up with:

  
  def replace_named_bind_variables(statement, bind_vars) #:nodoc:
  	statement = statement.gsub(/(:?):([a-zA-Z]\w*)/) do
  		if $1 == ':' # skip postgresql casts
  			$& # return the whole match
  		elsif bind_vars.include?(match = $2.to_sym)
  			quote_bound_value(bind_vars[match])
  		else
  			raise PreparedStatementInvalid, "missing value for :#{match} in #{statement}"
  		end
  	end
  
  	nil_quote = connection.quote(nil)
  	statement = statement.gsub('!=') { |sub_str| $'.match(/([a-zA-Z]\w*)/).to_s == nil_quote ? 'IS NOT' : sub_str }
  	statement = statement.gsub('=') { |sub_str| $'.match(/([a-zA-Z]\w*)/).to_s == nil_quote ? 'IS' : sub_str }
  	statement
  end
  

  I don't have any tests as I (shamefully) don't yet know how to go about testing something like this. I have hit it with a few different scenarios and it worked correctly.

  One assumption I do make is that the database does not consider "NOT =" the same as "!=". Postgres errors on "NOT =", but I don't know about others. I wouldn't expect that to be valid SQL, but I don't know for sure.