Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• 

  CancelProfileIsBroken August 6th, 2009 @ 02:12 PM

  • Tag changed from “2.3.2, :joins, active_record, patch, update_all” to “2.3.2, :joins, active_record, bugmash, patch, update_all”

• You flagged this item as spam.
  

  Jacob Kjeldahl August 7th, 2009 @ 09:50 AM

  Here is the patch file

  • joins_in_update_all.diff 3.3 KB

• You flagged this item as spam.
  

  Josh Sharpe August 8th, 2009 @ 03:33 AM

  The idea makes sense, but the patch doesn't apply:

  Patch does not have a valid e-mail address.

• You flagged this item as spam.
  

  John Trupiano August 9th, 2009 @ 06:09 PM

  +1 on the idea. i have revised the patch so that it will apply cleanly (unfortunately it has my commit info and not Jacob's).

  The patch, however, causes 3 sqlite test failures. It seems to me that sqlite does not support the "UPDATE tbl INNER JOIN tbl2 ON tbl.id = tbl2.tbl_id" syntax. How do we go about solving this? Do we add a method to each of the adapters (:supports_updates_with_joins?) to check before applying any :joins options? If so, do we fail or simply ignore when :joins is passed in when being used with sqlite (or any other db that doesn't support the syntax)?

  • 2558-joins-in-update-all.diff 3.9 KB
• 

  Rizwan Reza August 9th, 2009 @ 06:12 PM

  verified

  +1 The patch works only on 2-3-stable. All tests pass.

• 

  Matt Jones August 9th, 2009 @ 06:22 PM

  The patch looks interesting, but I don't think it's a good idea: it seems likely that update_all is limited for a good reason. Encouraging people to, essentially, bypass ActiveRecord goes against the intention of the framework.

• You flagged this item as spam.
  

  Elad Meidar August 9th, 2009 @ 06:51 PM

  +1 Verified, -1 Patch i agree with Matt, seems like it's a better approach to leave #update_all limited and not encourage AR workarounds that will probably lead to unexpected issues.

• You flagged this item as spam.
  

  Derander August 9th, 2009 @ 09:03 PM

  -1 verified the patch

  Agreed w/ Elad, if you're going to do something like this, might as well just write the plain sql.

• 

  Jeremy Kemper August 10th, 2009 @ 06:50 AM

  • State changed from “new” to “wontfix”
  • Tag changed from “2.3.2, :joins, active_record, bugmash, patch, update_all” to “2.3.2, :joins, active_record, patch, update_all”

  Agree that dropping to SQL is clearer.

• You flagged this item as spam.
  

  Jens July 19th, 2010 @ 05:10 PM

  • Importance changed from “” to “”

  This is not true and prevents one from clean separation between objects:

  class Parent < AR::Base
    has_many :children
    has_many :grandchildren, :through => :children, :dependent => :nullify
  
    def before_update
      self.grandchildren.delete_all  # this and ".clear" does NOT WORK, thus:
      self.grandchildren.disconnect_disliked_ones # see below
    end
  end
  
  class Child < AR::Base
    has_many :grandchildren
  end
  
  class Grandchild < AR::Base
    def self.disconnect_disliked_ones
      self.update_all("child_id = NULL", "brave IS FALSE")
    end
  end
  

  The idea is that I put the intricacies of the "disconnect" operation where the implementation is, i.e. in the grandchild. Also, the type of connection between grandchild and parent is not restricted per se.

  However, if called from within a Parent instance with ID=42, this will result in an invalid SQL query like

  UPDATE "grandchildren" 
    SET grandchildren.child_id = NULL
    WHERE (grandchildren.brave IS FALSE) 
      AND (children.parent_id = 42)
  

  which will result in an error, because I cannot specify a JOIN clause.

  Of course, I can use Parent.find_by_sql and specify the whole UPDATE operation verbatim, however, then the separation between Parent and Grandchild internal structure is violated.

  Is it not possible to put the JOIN patch into the source and leave it to the user whether to use it or not?

  Thanks,

  Jens

• You flagged this item as spam.
  

  Claudio Poli October 1st, 2010 @ 03:30 PM

  I agree on having this functionality in core.

• You flagged this item as spam.
  

  eydaimon November 12th, 2010 @ 06:43 PM

  +1 on supporting this.

  I'd want to be able to do:

  User.find(30).tasks.update_all completed: true