Hash Conditions belongs_to sugar

Hummm... Lighthouse won't let me upload a diff :(

hey there nikz,

your diff did upload,

but looking at it,

seems it reverts a whole load of new changes.

like "merge conditions" becoming public.

I imagine the only code change you intended to do was

@@ -2015,7 +2012,14 @@ module ActiveRecord #:nodoc:
             else
               table_name = quoted_table_name
             end
-
+            
+            unless column_names.include?(attr) 
+              # Lookup the foreign key for belongs_to associations
+              if reflection = reflect_on_all_associations(:belongs_to).find { |a| a.name.to_s == attr }
+                attr = reflection.association_foreign_key
+              end
+            end
+          
             "#{table_name}.#{connection.quote_column_name(attr)} #{attribute_condition(value)}"
           end.join(' AND ')

but you should probably clean up your diff.

I like this, but you should look at the case when the belongs_to association is polymorphic. I'd expect that to set both the x_id and x_type conditions.

@MatthewRudY: Hummm, that diff stuff is weird... Guess I'm not using git format-patch correctly (although I followed the guide).

Anyway, here's a clean diff.

@Josh: Working on it, give me a couple hours :)

I had created a similar patch a couple of weeks ago, but never uploaded. I've uploaded it now, maybe it's of some use. My take was different. Instead of the example given I was looking for this:

Person.find_all_by_university(victoria_wellington)

I've added the code + tests for polymorphic associations too.

Didn't think it needed any doc changes, as like Josh says, seems pretty much along the lines of what you'd expect.

Hopefully this diff is clean :s

yeah,

diff looks clean... nice.

like Lawrence's change also.

surely these should be merged together?

class Model
  belongs_to :association
end

Model.find_by_association(@association).should ==
Model.find(:first, :conditions => {:association => @association}

I'd also like to point out that I'd support has_one associations as well.

Lawrence: didn't see your patch for the find_by_whatever change, but I did that a while ago. I never submitted it as a patch to Rails since it was pretty ugly to get the polymorphic flavor working. If you want to look at what I did (nearly 2 years ago), here's the link:

http://blog.hasmanythrough.com/2...

I'd love to see this get into AR, but I think it should support polymorphic associations or things could get crazy.

What's the best way to integrate everything?

I agree with MatthewRudy that

Model.find_by_association(@association).should ==
Model.find(:first, :conditions => {:association => @association}

But, is it best to do both at once? Perhaps the simpler case (the Hash conditions IMHO), then we can tackle the dynamic polymorphic finders (!).

Nik: agreed. keep this ticket scoped to the :conditions changed. I only piled on the dynamic finders here cause I couldn't find the other ticket (sorry!).

@Josh: Sweet, I'll see what I can do about merging yours and Lawrence's changes, and the :conditions changes. After I (hopefully) get this accepted.

@Lawrence: Yeah, I'm giving has_one a go. The conditions aren't that hard, the only problem is you've got to include the foreign table too (i.e you have to select * from people, universities ...)

Unless I'm missing something, of course :) I can upload what I've got, which sets the conditions correctly, and has a failing test, if someone feels like fixing that problem :)

@Pratik: awww, but this changeset is everything I need to get has_one working :(

Are we certain we don't want the alias working for the hash conditions?

If so, I'll work on getting Josh's patch up with the play (assuming Josh is cool with that) :)

Hey nikz,

This is just my opinion :) I think it's better to send an email to core mailing list and see what everyone else thinks. I was planning to do that cd994eff9a343df376bfaec59de5b24a2ab51256 for a while, which was one of the reason why I was/am -1 on this patch.

We can also easily support association names inside condition hash, in which case, we'd break our current rule that everything inside :conditions refers to table name/alias and columns. So if we do end up having that, your original patch would be a nice addition.

Need to hear more opinions.

Now that Pratik mentions it, I agree with what he's saying. The things within a condition hash should always refer to actual table and column names.

don't know if I agree with that.

if I wanted to be rigidly tied to the db structure I'd write everything as SQL strings

the hash :conditions already brings a little bit of magic;

"#{key} " + if value.is_a? Array
  "IN (...)"
elsif value.nil?
  "IS NULL"
else
  "= ..."
end

I don't see why it shouldn't extend to the provided keys.

if has_association(key)
  "#{association.foreign_key} = ..."
else
  "#{key} = ..."
end

could use the same thing to make :conditions => {:id => 5} into a magic alias for "#{Model.primary_key} = 5", so it'd work when mixed into a legacy Model from a plugin perhaps.

+1 on this patch.

Another place where conditions magic takes place is expand_hash_conditions_for_aggregates method, which makes the following conditions translation possible (from docs):

# And for value objects on a composed_of relationship:
#   { :address => Address.new("123 abc st.", "chicago") }
#     # => "address_street='123 abc st.' and address_city='chicago'"

Which creates precedent IMO for merging this in :)