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.

Sign up for GitHub

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

Jump to bottom

Rails 4.2 call save in getter method causes "instance variable @cached_changed_attributes not defined" bug. #20531

Closed
raven-chen opened this issue Jun 12, 2015 · 4 comments
Closed

Rails 4.2 call save in getter method causes "instance variable @cached_changed_attributes not defined" bug. #20531

raven-chen opened this issue Jun 12, 2015 · 4 comments
Assignees
@sgrif
Labels
activerecord regression

Comments

@raven-chen
Copy link

If I overwrite a getter method and call save inside it like

  def some_attribute
     update_attribute :some_attribute, "value"
  end

cache_changed_attributes will raise instance variable @cached_changed_attributes not defined exception. because it will be called twice, the one inside yield removed @cached_changed_attributes so the second time, remove_instance_variable(:@cached_changed_attributes) can not find @cached_changed_attributes.

I've created test case here https://gist.github.com/raven-chen/fc5669ecda5f8b62e2fa

the change that bring this bug made here 9384fa43c
@matthewd
Copy link
Member

@raven-chen independent of the error, that sounds like you're breaking the "known that the computed value cannot change" assumption that allows us to cache in the first place. @sgrif?

@sgrif
Copy link
Contributor

sgrif commented Jun 12, 2015

I mean it should go without saying that having side effects in a getter is a bad idea. Still, this is technically valid so we should handle it.

@sgrif sgrif self-assigned this Jun 12, 2015
@sgrif sgrif closed this as completed in 07b4078 Jun 12, 2015
sgrif added a commit that referenced this issue Jun 12, 2015
@sgrif
Don't crash when mutating attributes in a getter
02e6bcc 
If a getter has side effects on the DB, `changes_applied` will be called
twice. The second time will try and remove the changed attributes cache,
and will crash because it's already been unset. This also demonstrates
that we shouldn't assume that calling getters won't change the value of
`changed_attributes`, and we need to clear the cache if an attribute is
modified.

Fixes #20531.
@sgrif
Copy link
Contributor

sgrif commented Jun 12, 2015

Backported in 02e6bcc

@raven-chen
Copy link
Author

Thanks 👍

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

@sgrif sgrif

Labels
activerecord regression
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@matthewd @raven-chen @sgrif