update_attribute violates POLA

POLA http://en.wikipedia.org/wiki/Principle_of_least_astonishment

Examples


# user.rb

class User < ActiveRecord::Base

attr_accessible :name, :username

validates :username, uniqueness: true

end

user1 = User.create(name: 'Name 1', username: 'username1')

user2 = User.create(name: 'Name 2', username: 'username2')

user1.username = 'username2'

user1.save # => false

user1.update_attribute(name: 'New Name')

user1.reload

user1.username # => 'username2'

# update_attribute is expected to update the specified attribute, not other ones. Thus it is violating POLA

update_attribute is expected to update the specified attribute, not other ones. Thus it is violating POLA

This is why you never want to rely on validates_uniqueness_of. It’s essentially a convenience for generating pretty error messages. Use your DB’s unique constraints to validate conditions dependent on other records in the relation (such as uniqueness).

update_attribute and update_attributes do this:

  1) They update the passed attributes in the receiver.

  2) They save the receiver.

The arguments say which attributes have to be updated, and as a convenience
the model is saved (callbacks run, timestamps are updated, etc.).

In addition, update_attribute has some extra semantics, in particular
validations are skipped and that's why it succeeded in your example. (Was
the example written by hand? the call does not seem to be valid.)

In recent version of Rails you can use update_column(s), whose semantics
are less confusing than the ones of update_attribute. But it is also going
to skip validations and other AR stuff because it issues straight SQL.

This behavior was exactly why update_attribute was (briefly) deprecated in 3.2.7:

https://groups.google.com/forum/?hl=en&fromgroups#!topic/rubyonrails-core/BWPUTK7WvYA

Not sure how it got un-deprecated…

—Matt Jones

I don't understand. What were you expecting?

You have updated the name, not the username.

The issue here does not seem to be what everyone is addressing. If I understood Anh correctly, when you run update_attribute X on a unsaved model that has one attribute Y set in memory, calling reload will report both X and Y as saved, so it seems to be a reload issue at first glance. I will try and reproduce it shortly.

The point is that dirty attributes get persisted, not just the one you set.
That is, the (invalid) username was saved because validations are skipped
and the model as such is saved.

Persisting dirty attributes is not an overlook, callbacks are run so you
need to take the model as a whole and store it as a whole, with its current
state, with all its consequences.

The purpose of update_attribute was to save quick stuff, a flag toggle...
that kind of things. Nowadays you'd generally use update_column.

Xavier

PS: Of course all this is documented, but we are discussing semantics, not
documented behaviour.

True. I somehow overlooked the fact that update_attribute saves dirty attributes. Xavier, you said the purpose of update_attribute “was” to save quick stuff, are there plans to deprecate this behavior?

Yeah, but it’s kinda misleading that update_attribute saved other attributes and not just the one specified in the argument, being that the method is “update_attribute” as a singular attribute. The reload works fine, as the username was saved with update_attribute, which saved more than what it was told to.

Sorry, it should be

user1.update_attribute(:name, ‘New Name’)

I switched to update_column but the old code already created some dirty records in the DB. I should have created validation at DB level instead of trusted AR completely.

True. I somehow overlooked the fact that update_attribute saves dirty

attributes. Xavier, you said the purpose of update_attribute "was" to save
quick stuff, are there plans to deprecate this behavior?

It has been discussed, but update_attribute has been there for ages and by
now there's no plan to deprecate it. Right now we consider update_attribute
and update_column can coexist.

I would be cool that the API of update_attribute mentioned update_column
though.

You should have the database constraint even if update_attribute worked exactly the way you thought it would. It's even stated in the documentation for validates_uniqueness_of:

http://api.rubyonrails.org/classes/ActiveRecord/Validations/ClassMethods.html#method-i-validates_uniqueness_of

See "Concurrency and integrity". Unless your application is not multi-thread and has a single worker process, but I don't think this is the case, right? :wink:

Best,
Rodrigo.

I know, but the problem is that ‘update_attribute’ is misleading. I’m wondering why don’t we just deprecate it and force people to use the better “update_column” instead. Or at least make “update_attribute” behave like “update_column”.

I agree and am +1 for deprecating it but I believe the decision to keep it was already taken.

I know, but the problem is that 'update_attribute' is misleading. I'm

wondering why don't we just deprecate it and force people to use the better
"update_column" instead. Or at least make "update_attribute" behave like
"update_column".

Yes, that's totally understandable and historically there have been
discussions and some temptative steps towards it.

There was a time where #update_attribute was even removed from master:

https://github.com/rails/rails/commit/a7f4b0a1231bf3c65db2ad4066da78c3da5ffb01

this was later reverted:

https://github.com/rails/rails/commit/50bdb924ba26999a468ec4844917cefec39ba08c

because a deprecation policy was being discussed.

Indeed, #update_attributes became deprecated because we felt
#update(attributes) was a better interface:

https://github.com/rails/rails/commit/2549a3b088de14c36d60de6c60c1374af75c326f

and albeit #update is considered to be the modern way to update a record,
the deprecation in the end was removed, and finally #update_attributes just
became an alias:

https://github.com/rails/rails/commit/1f3a1fedf951dbc4b72d178e2a649c4afd2f1566

Those are some commits to show your remarks resonate, but that's the tip of
the iceberg, as they suggest we've had many discussions about how to evolve
this API over the last year and a half or so. So, as you see, we sympathize
with the need of a deprecation in #update_attribute in particular, but by
now the state of the API is the outcome of everything taken in
consideration.

It could be the case that we revisit this topic in the future.