Ruby on Rails

Comments and changes to this ticket

• 

  nikz June 22nd, 2008 @ 12:30 PM

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

• 

  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.

• 

  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.

• 

  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
• 

  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
• 

  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
• 

  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}
  

• 

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

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

• 

  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.

• 

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

• 

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

• 

  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 changed from “” 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.

• 

  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.

• 

  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.

• 

  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 changed from “” 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.