Fix a bug with embedded documents #95

Merged
merged 1 commit into from May 7, 2013

Conversation

Projects
None yet
3 participants
@did
Contributor

did commented May 6, 2013

Hi @jnicklas,

I found a bug when I was upgrading LocomotiveCMS to mongoid 3.x. Actually, mongoid prevents carriewave to attach a file to a embedded document owned by an existing document. It only occurs if the embedded document didn't have a attached file when the parent document was created. A little bit more of explanation here: jnicklas#64 (comment)

To be honest, I'm not satisfied by my "patch" but anyway, it solves the bug.
Thanks !

Didier

@rmm5t

This comment has been minimized.

Show comment
Hide comment
@rmm5t

rmm5t May 6, 2013

Member

@did Could you help me better understand this a bit more? Are you saying that we must write a bogus value (old) to the uploader field upon save of a new file if the document was once saved with a nil file?

Could you help me better understand why our current #{column}_changed? workaround doesn't work for this scenario? (we're already trying to force dirty attributes)

Member

rmm5t commented May 6, 2013

@did Could you help me better understand this a bit more? Are you saying that we must write a bogus value (old) to the uploader field upon save of a new file if the document was once saved with a nil file?

Could you help me better understand why our current #{column}_changed? workaround doesn't work for this scenario? (we're already trying to force dirty attributes)

rmm5t added a commit that referenced this pull request May 7, 2013

Merge pull request #95 from locomotivecms/master
Fix a bug with embedded documents

@rmm5t rmm5t merged commit 8a87725 into carrierwaveuploader:master May 7, 2013

1 check passed

default The Travis build passed
Details
@rmm5t

This comment has been minimized.

Show comment
Hide comment
@rmm5t

rmm5t May 7, 2013

Member

@did I started playing with this myself and better understood the issue. I agree that I'd like to see a better solution to this problem if possible, but merged nonetheless. Thank you!

Member

rmm5t commented May 7, 2013

@did I started playing with this myself and better understood the issue. I agree that I'd like to see a better solution to this problem if possible, but merged nonetheless. Thank you!

rmm5t added a commit that referenced this pull request May 7, 2013

@did

This comment has been minimized.

Show comment
Hide comment
@did

did May 7, 2013

Contributor

@rmm5t, the problem seems to be located in the mongoid gem, here: https://github.com/mongoid/mongoid/blob/3.1.0-stable/lib/mongoid/dirty.rb#L68. Is it related to only embedded documents ? I've no idea.
Just one more thing, in the 2.4.x mongoid version, there is no such "if change" condition in the dirty module. Don't know why they added it.
Thanks !

Contributor

did commented May 7, 2013

@rmm5t, the problem seems to be located in the mongoid gem, here: https://github.com/mongoid/mongoid/blob/3.1.0-stable/lib/mongoid/dirty.rb#L68. Is it related to only embedded documents ? I've no idea.
Just one more thing, in the 2.4.x mongoid version, there is no such "if change" condition in the dirty module. Don't know why they added it.
Thanks !

@rmm5t

This comment has been minimized.

Show comment
Hide comment
@rmm5t

rmm5t May 7, 2013

Member

@did Good find. Commenting out the if change in the mongoid library reveals one test that relies on it.

https://github.com/mongoid/mongoid/blob/master/spec/mongoid/changeable_spec.rb#L610

No change is flagged even if {field}_will_change! is called (which is what carrierwave-mongoid is trying to do).

Haling @durran to see if he can help shed some light here and help us come up with a better solution to our problem.

Member

rmm5t commented May 7, 2013

@did Good find. Commenting out the if change in the mongoid library reveals one test that relies on it.

https://github.com/mongoid/mongoid/blob/master/spec/mongoid/changeable_spec.rb#L610

No change is flagged even if {field}_will_change! is called (which is what carrierwave-mongoid is trying to do).

Haling @durran to see if he can help shed some light here and help us come up with a better solution to our problem.

@did

This comment has been minimized.

Show comment
Hide comment
@did

did May 7, 2013

Contributor

I'm looking forward to seeing @durran's explanation :-)
Thanks Ryan !

Contributor

did commented May 7, 2013

I'm looking forward to seeing @durran's explanation :-)
Thanks Ryan !

ShamoX pushed a commit to Gnuside/carrierwave-mongoid that referenced this pull request Dec 13, 2013

Fix removing uploaded files in embedded documents.
Sending `remove_file = "1"` to an embedded document does not work: Mongoid won’t consider the document dirty and so `save` won’t do anything. (Note that this bug only occurs if there are no other changes to the document.)

I'm not 100% sure how this relates to carrierwaveuploader#95 – calling `uploader_column_will_change!` seems to suffice in this case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment