update_attribute violates POLA

rubyonrails-core
1

POLA Principle of least astonishment - Wikipedia

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
2

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

3

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.

4

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

5

I don't understand. What were you expecting?

You have updated the name, not the username.

6

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.

7

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.

8

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?

9

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.

10

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.

11

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.

12

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.

13

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”.

14

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

15

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:

this was later reverted:

because a deprecation policy was being discussed.

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

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:

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.