Skip to content
This repository

Unbreak update_column/update_columns for the primary key attribute. #8053

Merged
merged 1 commit into from over 1 year ago

5 participants

Henrik Nyh Carlos Antonio da Silva Rafael Mendonça França José Valim Steve Klabnik
Henrik Nyh

Didn't work before because it updated the model-in-memory first, so the DB query couldn't find the record.

activerecord/lib/active_record/persistence.rb
... ... @@ -224,11 +224,11 @@ def update_columns(attributes)
224 224 verify_readonly_attribute(key.to_s)
225 225 end
226 226
  227 + self.class.where(self.class.primary_key => id).update_all(attributes) == 1
4
José Valim Owner

This change is modifying the return value of update_column. We should probably add a test to ensure it returns a boolean.

Henrik Nyh
henrik added a note

Yeah, I just noticed this. Should I go ahead and fix? Any preference between amend/force push vs. multiple commits in a feature branch?

Steve Klabnik Collaborator

Generally we prefer commits to be squashed, so amend/force push.

Henrik Nyh
henrik added a note

Alright, pushed an amended commit: henrik@cd0efa5

Just tested that it returns true. Good enough? Seems the return value is undocumented anyway, and I'm not sure what it'd take to make it return false. Multiple records with the same id or for the query to somehow update nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/CHANGELOG.md
... ... @@ -1,5 +1,9 @@
1 1 ## Rails 4.0.0 (unreleased) ##
2 2
  3 +* Fix bug where update\_attributes and update\_attribute would not let you update the primary key column.
4

Update attribute(s)?

Henrik Nyh
henrik added a note

I used "update_attribute(s)" for the commit message but figured I should avoid the possible confusion with method invocation, in the CHANGELOG. I'd be fine with either though!

I meant to say that you're fixing a "bug" in update_column/update_columns methods, not update_attribute(s) as the changelog says.

Henrik Nyh
henrik added a note

Oh, sorry! Fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/lib/active_record/persistence.rb
((6 lines not shown))
227 229 attributes.each do |k,v|
228 230 raw_write_attribute(k,v)
229 231 end
230 232
231   - self.class.where(self.class.primary_key => id).update_all(attributes) == 1
  233 + result_of_update == 1
232 234 end
2

How about update_count?

Henrik Nyh
henrik added a note

Good idea. Changed it to updated_count.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carlos Antonio da Silva

@henrik I think it's "the query updating nothing", in any case, it's just better to not change the result value to avoid possible breakage (and it'd be weird to return the attributes enumerable anyways). I made small comments, but overall it looks good, thanks!

Carlos Antonio da Silva

@henrik thanks. Github says it cannot be merged anymore, can you rebase it from master please? It's probably the changelog.

Carlos Antonio da Silva

And since you're touching the changelog, how about using backticks like this instead of \ escaping: "update_columns and update_column"

Rafael Mendonça França
Owner

Do you mean this? `update_column` and `update_column`

Carlos Antonio da Silva

Yes.

Henrik Nyh

Good idea - used backticks, rebased against master.

Henrik Nyh

Oops, a merge commit snuck in there. Fixing.

Henrik Nyh henrik Enable update_column(s) for the primary key attribute.
Didn't work before because it updated the model-in-memory first, so the DB query couldn't find the record.
1849665
Henrik Nyh

And there we go.

Carlos Antonio da Silva carlosantoniodasilva merged commit c6f47c1 into from
Carlos Antonio da Silva

@henrik awesome, thanks.

Henrik Nyh

Wow, that was faster and less ceremony than expected. Thanks a lot, guys!

Rafael Mendonça França
Owner

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Oct 28, 2012
Henrik Nyh henrik Enable update_column(s) for the primary key attribute.
Didn't work before because it updated the model-in-memory first, so the DB query couldn't find the record.
1849665
This page is out of date. Refresh to see the latest.
4 activerecord/CHANGELOG.md
Source Rendered
... ... @@ -1,5 +1,9 @@
1 1 ## Rails 4.0.0 (unreleased) ##
2 2
  3 +* Fix bug where `update_columns` and `update_column` would not let you update the primary key column.
  4 +
  5 + *Henrik Nyh*
  6 +
3 7 * The `create_table` method raises an `ArgumentError` when the primary key column is redefined.
4 8 Fix #6378
5 9
4 activerecord/lib/active_record/persistence.rb
@@ -224,11 +224,13 @@ def update_columns(attributes)
224 224 verify_readonly_attribute(key.to_s)
225 225 end
226 226
  227 + updated_count = self.class.where(self.class.primary_key => id).update_all(attributes)
  228 +
227 229 attributes.each do |k,v|
228 230 raw_write_attribute(k,v)
229 231 end
230 232
231   - self.class.where(self.class.primary_key => id).update_all(attributes) == 1
  233 + updated_count == 1
232 234 end
233 235
234 236 # Initializes +attribute+ to zero if +nil+ and adds the value passed as +by+ (default is 1).
13 activerecord/test/cases/persistence_test.rb
@@ -592,6 +592,19 @@ def test_update_columns_with_one_changed_and_one_updated
592 592 assert_equal 'super_title', t.title
593 593 end
594 594
  595 + def test_update_columns_changing_id
  596 + topic = Topic.find(1)
  597 + topic.update_columns(id: 123)
  598 + assert_equal 123, topic.id
  599 + topic.reload
  600 + assert_equal 123, topic.id
  601 + end
  602 +
  603 + def test_update_columns_returns_boolean
  604 + topic = Topic.find(1)
  605 + assert_equal true, topic.update_columns(title: "New title")
  606 + end
  607 +
595 608 def test_update_attributes
596 609 topic = Topic.find(1)
597 610 assert !topic.approved?

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.