Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed ActiveRecord::Relation#update behavior so that it will work on Relation objects without giving id #11898

Merged
merged 1 commit into from Jan 2, 2015

Conversation

@prathamesh-sonpatki
Copy link
Member

prathamesh-sonpatki commented Aug 15, 2013

No description provided.

@egilburg
egilburg reviewed Aug 15, 2013
View changes
activerecord/lib/active_record/relation.rb Outdated
def update(id, attributes)
if id.is_a?(Array)
id.map.with_index { |one_id, idx| update(one_id, attributes[idx]) }
if attributes.is_a?(Array)

This comment has been minimized.

Copy link
@egilburg

egilburg Aug 15, 2013

Contributor

This is probably simpler:

        id.map.with_index do |one_id, idx| 
          attrs = attributes.is_a?(Array) ? attributes[idx] : attributes

          update(one_id, attrs)
        end

This comment has been minimized.

Copy link
@prathamesh-sonpatki

prathamesh-sonpatki Aug 15, 2013

Author Member

@egilburg Thanks. Updated :)

@senny
Copy link
Member

senny commented Aug 15, 2013

This implementation issues a single query per record to update. Since all the records are updated with the same condition we can perform the update in a single query. Is there really a need for this PR, when you could use:

Person.where(id: [1,2,3,4]).update_all(last_seen: Time.now)
@egilburg
Copy link
Contributor

egilburg commented Aug 15, 2013

Isn't update_all being deprecated?

@senny
Copy link
Member

senny commented Aug 15, 2013

Not that I know of => https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation.rb#L304

But you need to call it on a Relation, I think it was available at the class level at some point Person.update_all but that is no longer supported.

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Aug 15, 2013

@senny update_all doesn't run callbacks and validations.

@senny
Copy link
Member

senny commented Aug 15, 2013

@prathamesh-sonpatki yea right. I just browsed the API and it seems there is no method that acts on the Relation to update and also runs callbacks.

@rafaelfranca @carlosantoniodasilva is there a reason we only have Person.update(ids, values) to update with callbacks and nothing that acts on the Relation?

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Aug 15, 2013

Investigated it a bit. Right now if you try to call update on a relation, ActiveRecord::Relation#update gets called. If we pass correct ids and values it will update the records with callbacks. But when we are dealing with relation, we should not pass ids at all. Just pass the hash of values.

I think we have 3 usecases

  1. Update different records with different values - Existing behavior of update
  2. Update different records with same values using ids - Implemented in this PR
  3. Update different records with same values using relation - Missing
@senny
Copy link
Member

senny commented Aug 15, 2013

@prathamesh-sonpatki I'm not sure we need 2) if we have 3). Let's wait for some feedback.

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Aug 19, 2013

@carlosantoniodasilva Any thoughts?

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Aug 29, 2013

any updates on this?

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Nov 12, 2013

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Dec 1, 2013

I have rebased with current master

@rafaelfranca
Copy link
Member

rafaelfranca commented Dec 1, 2013

I prefer to have 3) and not implement 2).

@prathamesh-sonpatki could you take a look if it is possible to implement 3)?

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Dec 1, 2013

@rafaelfranca Its possible. i will update soon

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Dec 5, 2013

@rafaelfranca Sorry for the delay. I have updated. Please take a look

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Dec 31, 2013

@rafaelfranca Can you see this issue?

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 1, 2014

Sure, but I don't want to include this on 4.1, so we will have to wait the final release to merge it

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Jun 4, 2014

@rafaelfranca any updates on this issue?

@rafaelfranca
rafaelfranca reviewed Jun 4, 2014
View changes
activerecord/lib/active_record/relation.rb Outdated
# # Updates multiple records from the result of a relation
# people = Person.where(group: 'expert')
# people.update(group: 'masters')
def update(id = nil, attributes)

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jun 4, 2014

Member

I don't think this will work. If you pass only one argument it will be always id.

This comment has been minimized.

Copy link
@matthewd

matthewd Jun 4, 2014

Member

I wasn't sure about 1.9, but just checked: it's fine.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jun 4, 2014

Member

You are right. Same with 1.9.

This comment has been minimized.

Copy link
@matthewd

matthewd Jun 4, 2014

Member

.. that said, I think I'd rather that .update(nil, group: 'masters') continued to fail, instead of suddenly meaning "update all the things".

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jun 4, 2014

Member

Yes. This make sense.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jun 4, 2014

Member

Yes, it is the best name. But .update(nil, group: 'masters') updating all the things can be a huge problem in existing applications upgrading to 4.2. Maybe we should check if id is a hash and trigger this new behaviour.

Also I was thinking. Depending on the number of record your relation return you may have a huge performance issue since it will do a update query for each record.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jun 4, 2014

Member

Ah, yes, maybe defaulting the id value to :all would fix this concern.

This comment has been minimized.

Copy link
@prathamesh-sonpatki

prathamesh-sonpatki Jun 4, 2014

Author Member

@rafaelfranca For performance issues, should we do find_each instead of to_a ?

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jun 4, 2014

Member

We would have an improvement to not load all the record on the memory but the number of queries would be even worse. I think we should only document this possible performance problem and point users to update_all

This comment has been minimized.

Copy link
@prathamesh-sonpatki

prathamesh-sonpatki Jun 5, 2014

Author Member

@rafaelfranca I updated the PR. Should we also update ActiveRecord getting started guide where we mention update and update_all

@rafaelfranca
Copy link
Member

rafaelfranca commented Jun 4, 2014

I really not sure about this. @dhh @jeremy @tenderlove WDYT?

@dhh
Copy link
Member

dhh commented Jun 4, 2014

I’m personally a big fan of this. I really don’t like the #update_all thing. And this strikes me as a far more welcome syntax. So 👍 from my end.

On Jun 4, 2014, at 4:19 PM, Rafael Mendonça França notifications@github.com wrote:

I really not sure about this. @dhh @jeremy @tenderlove WDYT?


Reply to this email directly or view it on GitHub.

@tenderlove
Copy link
Member

tenderlove commented Jun 4, 2014

+1

Aaron Patterson
http://tenderlovemaking.com/
I'm on an iPhone so I apologize for top posting.

On Jun 4, 2014, at 7:18 AM, Rafael Mendonça França notifications@github.com wrote:

I really not sure about this. @dhh @jeremy @tenderlove WDYT?


Reply to this email directly or view it on GitHub.

@al2o3cr
Copy link
Contributor

al2o3cr commented Jun 4, 2014

This seems (IMO) confusingly close to update_all, but with radically different implications for the number of queries (N+1 here, 1 for update_all). Is this something people do often enough that an explicit each loop is a readability problem?

@dhh
Copy link
Member

dhh commented Jun 4, 2014

If you want callbacks to fire, then you have to do N+1. I think this is a legit use case, it’s neatly readable, and we document clearly what’s going on.

On Jun 4, 2014, at 8:49 PM, Matt Jones notifications@github.com wrote:

This seems (IMO) confusingly close to update_all, but with radically different implications for the number of queries (N+1 here, 1 for update_all). Is this something people do often enough that an explicit each loop is a readability problem?


Reply to this email directly or view it on GitHub.

@egilburg
Copy link
Contributor

egilburg commented Jun 4, 2014

If update_all fires no callbacks, perhaps it should be deprecated in favor of update_columns.

update_all is ambiguous and actually a bit scary since the name may imply "all as in all records in db, as opposed to current relation scope"

Also, should similar change be made for Relation#delete?

Eugene

On Jun 4, 2014, at 1:53 PM, David Heinemeier Hansson notifications@github.com wrote:

If you want callbacks to fire, then you have to do N+1. I think this is a legit use case, it’s neatly readable, and we document clearly what’s going on.

On Jun 4, 2014, at 8:49 PM, Matt Jones notifications@github.com wrote:

This seems (IMO) confusingly close to update_all, but with radically different implications for the number of queries (N+1 here, 1 for update_all). Is this something people do often enough that an explicit each loop is a readability problem?


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub.

@dhh
Copy link
Member

dhh commented Jun 4, 2014

Agree. We should make that clearer. And uniform across the API. So #delete should follow #update in the same way.

On Jun 4, 2014, at 9:07 PM, Eugene Gilburg notifications@github.com wrote:

If update_all fires no callbacks, perhaps it should be deprecated in favor of update_columns.

update_all is ambiguous and actually a bit scary since the name may imply "all as in all records in db, as opposed to current relation scope"

Also, should similar change be made for Relation#delete?

Eugene

On Jun 4, 2014, at 1:53 PM, David Heinemeier Hansson notifications@github.com wrote:

If you want callbacks to fire, then you have to do N+1. I think this is a legit use case, it’s neatly readable, and we document clearly what’s going on.

On Jun 4, 2014, at 8:49 PM, Matt Jones notifications@github.com wrote:

This seems (IMO) confusingly close to update_all, but with radically different implications for the number of queries (N+1 here, 1 for update_all). Is this something people do often enough that an explicit each loop is a readability problem?


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub.

@matthewd
Copy link
Member

matthewd commented Jun 4, 2014

So, I think this can go in, to round out:

delete_all : delete :: destroy_all : destroy :: update_all : update

And then we separately consider whether the _all methods might be renamed to something more explicitly different/dangerous. Right?

@egilburg
Copy link
Contributor

egilburg commented Jun 4, 2014

IMO In an ideal world the strategy would be determined by common arguments and not separate methods

.create .save, .update, .destroy, including the whether on record, assoc, or relation, would all take following arguments:

validate: true/false
callbacks: true/false

Both would default to true if not set.

Bang methods like save! should also honor the arguments and only raise if save failed WITH given arguments, rather than always assume save! requires validation.

This way a whole bunch of methods can be deprecated. The choice of n+1 or single query should be implementation detail which Rails can do in most efficient way possible under given circumstances. Mist users just want things to work and not have to research which method out of a myriad of similar looking ones is the right one.

Eugene

On Jun 4, 2014, at 2:22 PM, Matthew Draper notifications@github.com wrote:

So, I think this can go in, to round out:

delete_all : delete :: destroy_all : destroy :: update_all : update

And then we separately consider whether the _all methods might be renamed to something more explicitly different/dangerous. Right?


Reply to this email directly or view it on GitHub.

@egilburg
Copy link
Contributor

egilburg commented Jun 4, 2014

Example of how this could clean API up:

.update_all => .update

.update_attributes => .update(validate: false)

.update_columns => .update(validate: false, callbacks: false)

Let users say what they want done, not how to do it.

Ditto for all the other things...

Eugene

On Jun 4, 2014, at 2:34 PM, Eugene Gilburg eugene.gilburg@gmail.com wrote:

IMO In an ideal world the strategy would be determined by common arguments and not separate methods

.create .save, .update, .destroy, including the whether on record, assoc, or relation, would all take following arguments:

validate: true/false
callbacks: true/false

Both would default to true if not set.

Bang methods like save! should also honor the arguments and only raise if save failed WITH given arguments, rather than always assume save! requires validation.

This way a whole bunch of methods can be deprecated. The choice of n+1 or single query should be implementation detail which Rails can do in most efficient way possible under given circumstances. Mist users just want things to work and not have to research which method out of a myriad of similar looking ones is the right one.

Eugene

On Jun 4, 2014, at 2:22 PM, Matthew Draper notifications@github.com wrote:

So, I think this can go in, to round out:

delete_all : delete :: destroy_all : destroy :: update_all : update

And then we separately consider whether the _all methods might be renamed to something more explicitly different/dangerous. Right?


Reply to this email directly or view it on GitHub.

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Jun 10, 2014

@rafaelfranca Build is 💚

@mechanicles
Copy link
Contributor

mechanicles commented Jun 10, 2014

👍

@dhh
Copy link
Member

dhh commented Jun 10, 2014

Agree that we shouldn’t mix the methods that update records one at the time with callbacks with the bulk SQL update commands. These are so different in strategy that we shouldn’t try to pave over that.

On Jun 4, 2014, at 9:41 PM, Eugene Gilburg notifications@github.com wrote:

Example of how this could clean API up:

.update_all => .update

.update_attributes => .update(validate: false)

.update_columns => .update(validate: false, callbacks: false)

Let users say what they want done, not how to do it.

Ditto for all the other things...

Eugene

On Jun 4, 2014, at 2:34 PM, Eugene Gilburg eugene.gilburg@gmail.com wrote:

IMO In an ideal world the strategy would be determined by common arguments and not separate methods

.create .save, .update, .destroy, including the whether on record, assoc, or relation, would all take following arguments:

validate: true/false
callbacks: true/false

Both would default to true if not set.

Bang methods like save! should also honor the arguments and only raise if save failed WITH given arguments, rather than always assume save! requires validation.

This way a whole bunch of methods can be deprecated. The choice of n+1 or single query should be implementation detail which Rails can do in most efficient way possible under given circumstances. Mist users just want things to work and not have to research which method out of a myriad of similar looking ones is the right one.

Eugene

On Jun 4, 2014, at 2:22 PM, Matthew Draper notifications@github.com wrote:

So, I think this can go in, to round out:

delete_all : delete :: destroy_all : destroy :: update_all : update

And then we separately consider whether the _all methods might be renamed to something more explicitly different/dangerous. Right?


Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub.

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:patch-update branch 3 times, most recently Sep 7, 2014
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:patch-update branch Sep 27, 2014
…th callbacks and validations

- Right now, there is no method to update multiple records with
  validations and callbacks.
- Changed the behavior of existing `update` method so that when `id`
  attribute is not given and the method is called on an `Relation`
  object, it will execute update for every record of the `Relation` and
  will run validations and callbacks for every record.
- Added test case for validating that the callbacks run when `update` is
  called on a `Relation`.
- Changed test_create_columns_not_equal_attributes test from
  persistence_test to include author_name column on topics table as it
  it used in before_update callback.
- This change introduces performance issues when a large number of
  records are to be updated because it runs UPDATE query for every
  record of the result. The `update_all` method can be used in that case
  if callbacks are not required because it will only run single UPDATE
  for all the records.
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:patch-update branch to 5ef713c Dec 20, 2014
@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Dec 20, 2014

@rafaelfranca Rebased with latest master. Can we take a look at this now?

@rafaelfranca rafaelfranca merged commit 5ef713c into rails:master Jan 2, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
rafaelfranca added a commit that referenced this pull request Jan 2, 2015
Changed ActiveRecord::Relation#update behavior so that it will work on Relation objects without giving id

Conflicts:
	activerecord/CHANGELOG.md
@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Jan 2, 2015

@rafaelfranca Thanks :)

@prathamesh-sonpatki prathamesh-sonpatki deleted the prathamesh-sonpatki:patch-update branch Jan 2, 2015
@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016
kamipo added a commit that referenced this pull request Jul 30, 2018
Since 9ac7dd4, class level `update`, `destroy`, and `delete` were placed
in the `Persistence` module as class methods.

But `Relation#update` without passing ids which was introduced at #11898
is not a class method, and it was caused the extra scoping regression
#33470.

I moved the relation method back into the `Relation` to fix the
regression.

Fixes #33470.
kamipo added a commit that referenced this pull request Jul 31, 2018
Since 9ac7dd4, class level `update`, `destroy`, and `delete` were placed
in the `Persistence` module as class methods.

But `Relation#update` without passing ids which was introduced at #11898
is not a class method, and it was caused the extra scoping regression
#33470.

I moved the relation method back into the `Relation` to fix the
regression.

Fixes #33470.
kamipo added a commit that referenced this pull request Jul 31, 2018
Since 9ac7dd4, class level `update`, `destroy`, and `delete` were placed
in the `Persistence` module as class methods.

But `Relation#update` without passing ids which was introduced at #11898
is not a class method, and it was caused the extra scoping regression
#33470.

I moved the relation method back into the `Relation` to fix the
regression.

Fixes #33470.
kamipo added a commit to kamipo/rails that referenced this pull request Jan 1, 2019
That ability was introduced at rails#11898 as `Relation#update` without
giving ids, so the ability on the class level is not documented and not
tested.

c83e30d which fixes rails#33470 has lost two undocumented abilities.

One has fixed at 5c65688, but I missed the ability on the class level.

Removing any feature should not be suddenly happened in a stable version
even if that is not documented.

I've restored the ability and added test case to avoid any regression in
the future.

Fixes rails#34743.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.