This project is archived and is in readonly mode.

#467 ✓wontfix
nikz

Hash Conditions belongs_to sugar

Reported by nikz | June 22nd, 2008 @ 12:29 PM

This is just a small patch that enables you to do:

 Person.find(:all, :conditions => { :university => victoria_wellington })

As opposed to:

 Person.find(:all, :conditions => { :university_id => victoria_wellington })

Just a little bit of syntactic sugar for belongs_to associations :)

There are tests + a little bit of documentation.

Comments and changes to this ticket

  • nikz

    nikz June 22nd, 2008 @ 12:30 PM

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

  • MatthewRudy

    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

    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

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

  • Lawrence Pit

    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)

  • nikz

    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

  • MatthewRudy

    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

    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

    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

    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

    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

    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

    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

    Pratik June 27th, 2008 @ 03:36 PM

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

    Removing "doc" keyword.

  • nikz

    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

    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

    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

    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

    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.

  • Evgeniy Dolzhenko

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

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages