ActiveRecord::Persistence#touch should not use default_scope #1519

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@dmitriy-kiriyenko
Contributor

dmitriy-kiriyenko commented Jun 7, 2011

Hi.

Using update_all in touch looks smart enough, but there is a hidden issue in this: update_all uses default_scope.

Imagine a situation: you have the following User class:

class User < ActiveRecord::Base
  default_scope :active => true
end

user = User.unscoped.where(:active => false).first

user.touch
# This fires sql "UPDATE `users` SET `updated_at` = ? WHERE `users`.`active` = 0"
# and so our user instance remains changed (and the database remains unchanged)
user.changed? #=> true
user.reload # brings old updated_at value back to life

I've provided a test proving that and a fix for the issue.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jun 7, 2011

Contributor

Great patch. Could you just please tell which Rails version this is happening? /cc @jonleighton @tenderlove

Contributor

josevalim commented Jun 7, 2011

Great patch. Could you just please tell which Rails version this is happening? /cc @jonleighton @tenderlove

@dmitriy-kiriyenko

This comment has been minimized.

Show comment
Hide comment
@dmitriy-kiriyenko

dmitriy-kiriyenko Jun 7, 2011

Contributor

This is happening in edge (master) and 3.0.7, that I am using currently.
In 3.0.7 there is also additional similar issue: if there is a default_scope joins(:association), update_all produces sql like:
UPDATEbase_table_nameSETupdated_at= ? WHEREid= ? JOINassociation_table_nameON ...
and results in error "column updated_at is ambiguous". However, this particular bug not found in edge.

I'd say that this happens since e4943e9 where current touch implementation was introduced, but I've tested only in 3.0.7 and master.

Contributor

dmitriy-kiriyenko commented Jun 7, 2011

This is happening in edge (master) and 3.0.7, that I am using currently.
In 3.0.7 there is also additional similar issue: if there is a default_scope joins(:association), update_all produces sql like:
UPDATEbase_table_nameSETupdated_at= ? WHEREid= ? JOINassociation_table_nameON ...
and results in error "column updated_at is ambiguous". However, this particular bug not found in edge.

I'd say that this happens since e4943e9 where current touch implementation was introduced, but I've tested only in 3.0.7 and master.

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Jun 7, 2011

Member

I have merged this, thanks!

Member

jonleighton commented Jun 7, 2011

I have merged this, thanks!

@jonleighton jonleighton closed this Jun 7, 2011

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Jun 7, 2011

Member

Incidentally, ActiveRecord::Relation has never supported joins on updates, which is the root cause of the error on 3-0-stable you describe. I plan to fix this for 3.2.

Member

jonleighton commented Jun 7, 2011

Incidentally, ActiveRecord::Relation has never supported joins on updates, which is the root cause of the error on 3-0-stable you describe. I plan to fix this for 3.2.

porras added a commit to porras/rails that referenced this pull request Sep 7, 2011

jonleighton added a commit that referenced this pull request Sep 7, 2011

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