Mongoid 3: cascade_callbacks works for save but not update_attributes #64

Merged
merged 2 commits into from Aug 13, 2013

Conversation

Projects
None yet
9 participants
Member

rmm5t commented Feb 11, 2013

class Parent
    include Mongoid::Document

   embeds_many :children, cascade_callbacks: true
end

class Child
   include Mongoid::Document

   mount_uploader :image, ImageUploader

   embedded_in :parent
end

I am using Mongoid together with carrierwave. I have 2 classes like above. When I create a new Parent. parent.save works and the uploaded image got saved to the embedded child. But when I try to update a Parent, I cannot get the image to upload correctly using Parent.update_attributes().

I opened an issue in mongoid project, but I think the problem probably is in the carrierwave-mongoid side.

Member

rmm5t commented Jan 15, 2013

@benzheren In reading the issue that you opened over on the Mongoid project, I'm guessing you were waiting for an official carrierwave-mongoid release that works against Mongoid 3.0. carrierwave-mongoid 0.3.0 was released to support Mongoid 3.0, so closing this issue out for now. If it's still an issue for you, please fee free to reopen this issue.

@rmm5t rmm5t closed this Jan 15, 2013

@rmm5t I just tried with latest master branch, carrierwave 0.7.1 with Mongoid 3.0.14, this problem still exists. How can I reopen this issue? Thanks!

Member

rmm5t commented Jan 15, 2013

Sorry, I assumed the original author had access to the "Reopen & Comment" button. Reopening. If you could submit a pull-request with a failing test case, that would help a ton.

@rmm5t rmm5t reopened this Jan 15, 2013

No problem, I wil add a test asap.

Member

rmm5t commented Feb 11, 2013

@benzheren I just started poking around with this issue. I cannot reproduce the problem, but I'm afraid I'm not reproducing it correctly.

For one, I don't think you need the cascade callbacks. Second, are you sure you don't just need an accepts_nested_attributes declaration?

I guess the real question is what parameters are you passing to Parent#update_attributes?

I attached some code to this issue that adds some specs to try to reproduce the issue that you reported, but I wasn't able to get a failing test. Could you please take a look at the diff and let me know what you are doing differently?

Member

rmm5t commented Feb 13, 2013

@benzheren Just checking in again. I'm planning to close this issue out as unable to reproduce unless I hear back from you on help with building a failing test case. I couldn't trigger any failures with my first attempt, but perhaps I misunderstood something.

@rmm5t I wrote a unit test according to my situation, especially the similar parameters pattern passed to Parenet#update_attributes, the test actually passed. But in my application it still fails to do updates for uploaded file. I am uploading audio files instead of image files and have an extra class to customize file processing during upload. Not sure if these customizations cause the problems for my app.

Member

rmm5t commented Feb 14, 2013

@benzheren Thanks for getting back. Buggers. Well, I'm going to close this issue out for now. If you can distill your particular use-case into a failing test, I'd be happy to look at it further. In the meantime, I'm going to have a tough time helping out otherwise.

@rmm5t rmm5t closed this Feb 14, 2013

Contributor

aklaiber commented Mar 18, 2013

I’ve the same problem in my Project. After update_attributes carrierwave print out only the temporary image not the final version of the image. If I reload the object I get the old image from carrierwave.

In my spec I call order.reload to reproduce this behavior.

https://gist.github.com/aklaiber/5187116

Member

rmm5t commented Mar 18, 2013

@aklaiber Is it possible that your issue is related to the same thing reported in #92?

Contributor

aklaiber commented Mar 18, 2013

No, I don't use the inherited_resources gem in my rails app.

Member

rmm5t commented Mar 18, 2013

@aklaiber Okay. If someone can put together a pull-request with at least a failing test case, I can help dig deeper.

Contributor

aklaiber commented Mar 19, 2013

It is very hard to reproduce this bug in the mongid specs. I don't know why but after @doc.reload I lose the complete image object.

https://github.com/aklaiber/carrierwave-mongoid/commit/1a81e61aacfcabd395311b30641f6a4ab54a8219

Member

rmm5t commented Mar 19, 2013

Contributor

aklaiber commented Mar 19, 2013

The spec fail. The image object is nil after @doc.reload.

Member

rmm5t commented Mar 19, 2013

@aklaiber Thanks for clarifying.

Could you please try adding cascade_callbacks: true to the embeds_many declaration and see if that fixes the problem?

embeds_many :mongo_locations, cascade_callbacks: true

This is documented in the README too.

Contributor

aklaiber commented Mar 19, 2013

Oh embarrassed i forgot the most important option. Now fail the right spec :-)

https://github.com/aklaiber/carrierwave-mongoid/commit/dd455e0e04398097a35e4914734b20c537a29d93

Contributor

aklaiber commented Mar 20, 2013

I think the main problem is the update callbacks won't called and the store_previous_model_for_#{column} method will never be executed.

Member

rmm5t commented Mar 20, 2013

@aklaiber Thanks. I'm reopening this PR for now so I can remember to come back to it.

@rmm5t rmm5t reopened this Mar 20, 2013

@aklaiber thanks for figuring out a spec test for this bug.

Member

rmm5t commented Apr 2, 2013

Update: I just took a stab at trying to debug the underlying issue here. I time-boxed myself, hit a dead-end, and then ran out of time. If anyone else has the time to look into a fix for this, I'd really appreciate it. Otherwise, I'll do my best to investigate it again in the near future.

Meanwhile, I updated this PR with @aklaiber's failing spec.

+1 on this issue.

I'm having the same problem. I have a model like this:

class Article
  # 1. Include mongoid stuff
  include Mongoid::Document
  include Mongoid::Timestamps
  include Mongoid::Slug

  # 2. Define fields 
  field :title, type: String
  field :lead, type: String
  field :body, type: String

  # 3. Set attributes accesible
  attr_accessible :title, :lead, :body, :article_image_attributes

  # 4. Set slug
  slug :title

  # 5. Set associations
  belongs_to :issue
  embeds_one :article_image, :as => :imageable, :class_name => 'Image', cascade_callbacks: true

  # 6. Accepting nested attributes
  accepts_nested_attributes_for :article_image, :allow_destroy => true
end

Everything should be in order and it works great when creating the article (and image), but when I update the article, with a new image, then the image doesn't update. It has all the new information in the params, but it doesn't update.

For now I'm updating the image "manually", by doing this:

def update
  article_update = @article.update_attributes(params[:article])
  article_image_update = @article.article_image.update_attributes(params[:article][:article_image_attributes])
  if article_update && article_image_update
    redirect_to magazine_issue_path(@magazine, @issue), notice: 'Article was successfully updated.'
  else
    render action: "edit", alert: add_errors(@article)
  end
end

It works, but it's not very pretty.

Any suggestions to what I could try?

Member

rmm5t commented Apr 4, 2013

@holgersindbaek Thanks for posting a workaround. I haven't yet figured out why update_attributes isn't working with nested attributes. If anyone has the time to debug this and at least find the underlying reason that it's not working, that would be a huge help.

edejong commented Apr 15, 2013

Saving the embedded document in a before_save callback also seems to work as a workaround. Maybe that provides a clue somehow.

before_save :save_article 
def save_article
  article.try(:save) unless self.new_record?
end
Member

rmm5t commented Apr 15, 2013

Thanks @edejong. Does anyone have an idea about why cascade_callbacks isn't triggering the before_save during an update_attributes?

hi @rmm5t, I do have an idea.
In my case, if I try to modify directly a embedded document without touching its parent, calling the save method (or any methods calling save) on the parent won't work. Basically, the changes on the embedded document are not popped up into the parent, so instance_of_parent.changes will return an empty hash which stops mongoid to save it.
I hope to come up with a patch quickly.

[UPDATE] the main problem is that if you don't set a file when you save for the first time the parent document, the next time, you'll save it, it won't work because the changes method from mongoid will return nothing because the old value of the field used to store the filename is nil. See here: https://github.com/mongoid/mongoid/blob/3.1.0-stable/lib/mongoid/dirty.rb#L68.

So, in this file: https://github.com/jnicklas/carrierwave-mongoid/blob/master/lib/carrierwave/mongoid.rb#L37, I added

write_uploader(column, '_old_') if persisted? && read_uploader(column).nil?

and my test passes.

I'm going to run the full tests suite if it breaks anything.

Member

rmm5t commented May 3, 2013

@locomotivehosting That's great. Your scenario does sound a bit different than the scenario behind this issue though. Am I misunderstanding it?

If it is different, please include a failing test with your pull-request so I can more easily confirm there was an actual problem initially.

hi @rmm5t, that's right. Actually, I'm now focused on solving that issue described here because my application happens to have all the "symptoms": embedded documents + carrierwave + accepts_nested_attributes_for.
However, I may have an idea about what it's going on here. Take a look at this statement here: https://github.com/jnicklas/carrierwave/blob/master/lib/carrierwave/mount.rb#L318
record.class does not seem to return the same result depending on where it is used. Since the carrierwave-mongoid gem uses the singleton class to override methods (like #{column}=), record.class returns the singleton and not the class itself. Which also means that we lost the "right" uploader at a step of the saving process.
I'm not sure if I'm being clear here.
Anyway, I need to continue my investigation :-)

[UPDATE] not sure if what I wrote is exact. What I know for sure is that, one way or another, we lost the uploader which stores the new file.

[UPDATE 2] forget what I said. Actually, I was confused because in the spec tests, the embedded document uses the same column name for the mount_uploader.

[UPDATE 3] I feel super stupid, the tests mentioned in this PR pass. The problem was an extra "@doc.reload". I do feel dumb. Sorry for having polluted this thread.

Member

rmm5t commented May 13, 2013

Rebased. Still looking for help resolving this issue if anyone has some extra time.

pechkin commented Jul 16, 2013

The problem caused by in_callback_state? method:

    # Is the document currently in a state that could potentially require
    # callbacks to be executed?
    #
    # @example Is the document in a callback state?
    #   document.in_callback_state?(:update)
    #
    # @param [ Symbol ] kind The callback kind.
    #
    # @return [ true, false ] If the document is in a callback state.
    #
    # @since 3.1.0
    def in_callback_state?(kind)
      [ :create, :destroy ].include?(kind) || new_record? || flagged_for_destroy? || changed?
    end

I found a workaround for this issue:

class Image

  include Mongoid::Document

  embedded_in :profile

  mount_uploader :image, ImageUploader

  def in_callback_state? kind
    true
  end

end

@did did referenced this pull request in locomotivecms/engine Aug 12, 2013

Closed

Can't update editable_file twice #769

did added a commit to did/carrierwave-mongoid that referenced this pull request Aug 12, 2013

the call to the default attribute_will_change method was not enough w…
…hen uploading a new file in an embedded document (#64)
Member

rmm5t commented Aug 13, 2013

#109 fixes this issue, but I'm still going to merge in these extra tests to make sure we have everything covered. Once the build passes on the master branch, I'll release a new version of carrierwave-mongoid. Thanks to @did for the fix!

rmm5t added a commit that referenced this pull request Aug 13, 2013

Merge pull request #64 from rmm5t/nested_attributes
Mongoid 3: cascade_callbacks works for save but not update_attributes

@rmm5t rmm5t merged commit 50bc75a into carrierwaveuploader:master Aug 13, 2013

1 check failed

default The Travis CI build failed
Details

@rmm5t rmm5t deleted the rmm5t:nested_attributes branch Aug 13, 2013

pbougie commented Aug 30, 2013

Pull request #109 overrides Mongoid's dirty tracking which I need in my app (replaces dirty attribute with new). Is there another way to get at this information? I don't think CarrierWave should be overriding default Mongoid behaviors which risks breaking many applications.

FYI: I use the info to track whether this is a new image being uploaded or I'm replacing an image.

Member

rmm5t commented Aug 30, 2013

@pbougie I tend to agree, but it also seems like Mongoid's dirty tracking is inherently flawed. I haven't researched this enough to fully grok why, but if we can get Mongoid to better cooperate with this (i.e. attribute_will_change method properly triggering a dirty field), I'd be happy to undo what we currently have overridden in carrierwave-mongoid.

Contributor

did commented Sep 1, 2013

@pbougie, I'm not satisfied at all by my PR but this was the only way to make Carrierwave work with Mongoid without forcing people update the code of their Mongoid models.
However, for very custom case purpose, you should still have the ability to override the setter for your field. Could you move the logic of your "track" method to the uploader itself which has a reference to the model ?

pbougie commented Sep 3, 2013

@did I've already moved the logic to the model itself. I hadn't thought of adding it to the uploader. At what point is the previous attribute value overridden (the dirty value) and how would I access it in the uploader?

On 2013-09-01, at 15:40 , Didier Lafforgue notifications@github.com wrote:

@pbougie, I'm not satisfied at all by my PR but this was the only way to make Carrierwave work with Mongoid without forcing people update the code of their Mongoid models.
However, for very custom case purpose, you should still have the ability to override the setter for your field. Could you move the logic of your "track" method to the uploader itself which has a reference to the model ?


Reply to this email directly or view it on GitHub.

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