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

ActiveStorage changes in nested model not detected during update #37701

Closed
camiloforero opened this issue Nov 13, 2019 · 11 comments · Fixed by #37786
Closed

ActiveStorage changes in nested model not detected during update #37701

camiloforero opened this issue Nov 13, 2019 · 11 comments · Fixed by #37786

Comments

@camiloforero
Copy link

camiloforero commented Nov 13, 2019

Steps to reproduce

Having two models:

class Parent < ApplicationRecord
  has_many: :children
  accepts_nested_attributes_for :children, allow_destroy: true
end

and

class Child < ApplicationRecord
  # Assume this Child class has a "name" column in the DB
  has_one_attached :example_file
end

And updating the Child model through a very vanilla Parent's controller:

  def update
    respond_to do |format|
      if @parent.update(parent_params)
        format.html { redirect_to @parent, notice: 'Parent was successfully updated.' }
        format.json { render :show, status: :ok, location: @parent }
      else
        format.html { render :edit }
        format.json { render json: @parent.errors, status: :unprocessable_entity }
      end
    end
  end

  def parent_params
      params.require(:parent).permit(
        :name, children_attributes: [:id, :name, :example_file, :_destroy],
      )
    end
  1. Create a new Parent with some Children in it, and save
  2. Edit the parent, change the example_file and the name of a Child, save, check the results
  3. Edit the parent, change only the example_file of a Child, save, check the results

Expected behavior

  1. In both cases the example_file should be updated. (in the first one the name is updated as well)

Actual behavior

The first case will work as expected. But in the second case, the Child with the new example_file will not be detected as changed, and the uploaded file will not be saved

System configuration

Rails version: 6.0.0

Ruby version: 2.6.2

@camiloforero camiloforero changed the title ActiveRecord changes in nested model not detected during save ActiveStorage changes in nested model not detected during update Nov 13, 2019
@pixeltrix
Copy link
Contributor

@camiloforero can you try with has_many :children, autosave: true - by default only new records are saved, not updated ones:

:autosave
    If true, always save the associated objects or destroy them if marked
    for destruction, when saving the parent object. If false, never save or
    destroy the associated objects. By default, only save associated
    objects that are new records. This option is implemented as a before_save
    callback. Because callbacks are run in the order they are defined,
    associated objects may need to be explicitly saved in any user-defined
    before_save callbacks.

@camiloforero
Copy link
Author

@pixeltrix I just tried and it didn't work either.

Also, keep in mind that Rails will still detect the changes in the nested model when I update a regular database field, without needing to set the autosave option. So there must be something off with the way Rails is doing this change detection

@pixeltrix
Copy link
Contributor

Hi @camiloforero - thanks for trying that.

The has_one_attached uses a has_one and has_one :through association underneath the hood and assignments to them happen immediately so unsure what's happening - can you create a single file example or a repo containing a simple rails app that demonstrates the problem, thanks.

@willtcarey
Copy link
Contributor

willtcarey commented Nov 22, 2019

I wanted to chime in and say I'm hitting this same issue. I'm the same setup as @camiloforero: A Parent record which has_many children and accepts_nested_attributes_for children. The suggestion earlier in this thread to try putting autosave: true on the relationship isn't needed because having the accepts_nested_attributes_for call turns the association into an autosave association.

I was able to trace it back to where the autosave association checks all the collection records to see if they need to be saved and the Child record returns false even though the ActiveStorage attachment has changed (checked with example_file.changed_for_autosave? on the Child record).

It seems as though the record is not properly checking ActiveStorage attachments for their changes.

My temporary workaround has been to override the changed_for_autosave? method in my child model with

class Child < ApplicationRecord
  ...
  
  def changed_for_autosave?
    super || example_file.changed_for_autosave?
  end
end

@pixeltrix I created a repository to reproduce the issue here.

Let me know if you need any other help in reproducing this issue!

@pixeltrix
Copy link
Contributor

@willtcarey thanks for the repo - helped track it down.

First thing I thought was the associations that Active Storage creates aren't defined with autosave: true, however turns out this is irrelevant because the actual association isn't updated until an after_save callback so changed? on the child wouldn't return true anyway. Looks to be a fairly simple fix to override changed_for_autosave? in ActiveStorage::Attached::Model to check for attachment changes, e.g:

def changed_for_autosave?() #:nodoc:
  super || !attachment_changes.empty?
end

Anyone want to have a go a fixing it?

@willtcarey
Copy link
Contributor

@pixeltrix I created a pull request for a fix. This is my first pull request on rails so I hope I got everything right. I wrote a test to go along with the fix but let me know if I need anything else!

@georgeclaghorn georgeclaghorn modified the milestones: 6.0.2, 6.0.3 Dec 3, 2019
@willtcarey
Copy link
Contributor

@pixeltrix Sorry to bother, but I'm checking to see if you need anything else from me to push this through?

@otaviogaiao
Copy link

def changed_for_autosave?
super || example_file.changed_for_autosave?
end

Thank you very much. Your workaround works perfectly. I was having the same issue.

@r-obeen
Copy link

r-obeen commented Mar 25, 2020

class Child < ApplicationRecord
def changed_for_autosave?
super || example_file.changed_for_autosave?
end
end

This did the trick for me as well.

Anyone knows what version for Rails will have a fix ? And when will it be released ?

@stevejc
Copy link

stevejc commented May 28, 2020

I'm running into a similar problem on any field in the nested attributes, not just an attached file. I can saved_changes on create, but nothing on the update. I upgraded to 6.0.3.1 and still have the issue. I'm assuming this fix is only for attached files. Should I submit a separate issue?

@jonathanhefner
Copy link
Member

@stevejc 6.0.3.1 was a security patch release. It only included CVE fixes. The fix from #37786 will be included in Rails 6.1 (since it was committed to master branch) and Rails 6.0.4 (since it was backported to 6-0-stable branch).

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

Successfully merging a pull request may close this issue.

8 participants