Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  nikz June 22nd, 2008 @ 12:30 PM

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

• You flagged this item as spam.
  

  MatthewRudy June 22nd, 2008 @ 03:11 PM

  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.

• You flagged this item as spam.
  

  Josh Susser June 22nd, 2008 @ 04:20 PM

  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.

• You flagged this item as spam.
  

  nikz June 22nd, 2008 @ 11:16 PM

  @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 :)

  • hash_conditions_sugar.diff 3 KB

• You flagged this item as spam.
  

  Lawrence Pit June 22nd, 2008 @ 11:53 PM

  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)

  • dynfind.patch 2.3 KB

• You flagged this item as spam.
  

  nikz June 23rd, 2008 @ 12:12 AM

  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

  • hash_conditions_sugar_with_polymorphic_sprinkles.diff 7.9 KB

• You flagged this item as spam.
  

  MatthewRudy June 23rd, 2008 @ 12:22 AM

  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}
  

• You flagged this item as spam.
  

  Lawrence Pit June 23rd, 2008 @ 12:33 AM

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

• You flagged this item as spam.
  

  Josh Susser June 23rd, 2008 @ 01:10 AM

  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.

• You flagged this item as spam.
  

  nikz June 23rd, 2008 @ 01:14 AM

  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 (!).

• You flagged this item as spam.
  

  Josh Susser June 23rd, 2008 @ 01:42 AM

  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!).

• You flagged this item as spam.
  

  nikz June 23rd, 2008 @ 01:52 AM

  @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 June 27th, 2008 @ 01:55 PM

  • Tag set to “activerecord, doc, edge, patch, tests”

  I'm -1 on syntactic sugar. I'd like it if :conditions always refer to the actual column names in the db, or table names. find_by_association and find_all_by_association idea sounds good though.

• 

  Pratik June 27th, 2008 @ 03:36 PM

  • Tag changed from “activerecord, doc, edge, patch, tests” to “activerecord, edge, patch, tests”

  Removing "doc" keyword.

• You flagged this item as spam.
  

  nikz June 28th, 2008 @ 02:09 AM

  @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) :)

• 

  Pratik June 28th, 2008 @ 02:20 AM

  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.

• You flagged this item as spam.
  

  Lawrence Pit June 29th, 2008 @ 12:40 AM

  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.

• You flagged this item as spam.
  

  MatthewRudy June 29th, 2008 @ 01:38 AM

  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.

• 

  Pratik July 12th, 2008 @ 02:20 PM

  • State changed from “new” to “wontfix”
  • Assigned user set to “Pratik”

  Closing this ticket for timebeing. It'd be nice to continue the discussion in mailing list. And if there is enough interest in this, will roll it in.

  Thanks.

• You flagged this item as spam.
  

  Evgeniy Dolzhenko October 20th, 2009 @ 04:16 PM

  +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 :)