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.

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

ActiveRecord "column"_changed? method issue with nil values #1571

Closed
fabn opened this issue Feb 24, 2015 · 5 comments
Closed

ActiveRecord "column"_changed? method issue with nil values #1571

fabn opened this issue Feb 24, 2015 · 5 comments

Comments

@fabn
Copy link

fabn commented Feb 24, 2015

I'm using CarrierWave backgrounder which relies on xxx_changed? method to enqueue processing jobs. In a failing test I discovered this CW behavior which triggers background jobs when files are not present at all.

Given this class

class Source < ActiveRecord::Base
  # CarrierWave integration for logos
  mount_uploader :logo, LogoUploader
end

the following happens

[12] pry(main)> s = Source.new
+----+------+---------+------+------------+------------+
| id | name | website | logo | created_at | updated_at |
+----+------+---------+------+------------+------------+
|    |      |         |      |            |            |
+----+------+---------+------+------------+------------+
1 row in set
[13] pry(main)> s.logo_changed?
false
[14] pry(main)> s.logo = nil
nil
[15] pry(main)> s.logo_changed?
true
[16] pry(main)> s.changes
{
    "logo" => [
        [0] #<LogoUploader:0x007fcc972db7a0 @model=#<Source id: nil, name: nil, website: nil, logo: nil, created_at: nil, updated_at: nil>, @mounted_as=:logo>,
        [1] #<LogoUploader:0x007fcc95a7d460 @model=#<Source id: nil, name: nil, website: nil, logo: nil, created_at: nil, updated_at: nil>, @mounted_as=:logo>
    ]
}
@fabn
Copy link
Author

fabn commented Feb 24, 2015

Another weird behavior:

[1] pry(main)> s = Source.create!(name: 'foo', website: 'http://www.example.com', remote_logo_url: '')
+----+------+------------------------+------+-------------------------+-------------------------+
| id | name | website                | logo | created_at              | updated_at              |
+----+------+------------------------+------+-------------------------+-------------------------+
| 1  | foo  | http://www.example.com |      | 2015-02-24 14:42:52 UTC | 2015-02-24 14:42:52 UTC |
+----+------+------------------------+------+-------------------------+-------------------------+
1 row in set
[2] pry(main)> s.previous_changes[:logo] # !!! should be nil
[
    [0] #<LogoUploader:0x007fa7fb932880 @model=#<Source id: 1, name: "foo", website: "http://www.example.com", logo: nil, created_at: "2015-02-24 14:42:52", updated_at: "2015-02-24 14:42:52">, @mounted_as=:logo>,
    [1] #<LogoUploader:0x007fa7fb9329e8 @model=#<Source id: 1, name: "foo", website: "http://www.example.com", logo: nil, created_at: "2015-02-24 14:42:52", updated_at: "2015-02-24 14:42:52">, @mounted_as=:logo, @storage=#<CarrierWave::Storage::File:0x007fa7fb9251d0 @uploader=#<LogoUploader:0x007fa7fb9329e8 ...>>>
]

@fabn
Copy link
Author

fabn commented Feb 24, 2015

I created a couple of failing specs for this in https://github.com/fabn/carrierwave/tree/issue-1571

I can try to fix them if you think this is an actual issue

@bensie
Copy link
Member

bensie commented Mar 4, 2015

@fabn If you can work up a fix for this that would be great! Looks valid to me.

We're piggy-backing on ActiveModel::Dirty as best we can, but there are definitely cases where it falls apart. You'll see when you dig in that we call send(:"\#{column}_will_change!") where appropriate, but I wouldn't be surprised at all if there are some holes in that.

avgerin0s added a commit that referenced this issue Apr 28, 2015
…ng something blank to a mounter that was originally blank
@avgerin0s
Copy link
Member

@fabn @bensie I'm trying to tackle this issue. You may take a look at the #1635 PR

@fabn
Copy link
Author

fabn commented Apr 28, 2015

@eavgerinos I'll try it as soon as I have some spare time. If this works is there any ETA for a rubygems release? I don't like to use github in Gemfile for production stuff.

avgerin0s added a commit that referenced this issue Apr 29, 2015
…ng something blank to a mounter that was originally blank
avgerin0s added a commit that referenced this issue Apr 30, 2015
…ng something blank to a mounter that was originally blank
avgerin0s added a commit that referenced this issue Apr 30, 2015
…ng something blank to a mounter that was originally blank
avgerin0s added a commit that referenced this issue May 20, 2015
[Fix #1571] - Do not write to ActiveModel::Dirty changes when assigning ...
mynock pushed a commit to mynock/carrierwave that referenced this issue Aug 9, 2016
…hanges when assigning something blank to a mounter that was originally blank
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants