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

Fix rails 6 save loop callback stack too deep on File upload in admin #892

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JeremiahChurch
Copy link

Summary

When CMS is included in a rails 6 parent app the after_save callback on the file model causes an endless callback loop that runs until a stack level too deep error is returned. And the file is not successfully uploaded/saved.

I reviewed the change logs for active storage and didn't see anything that seemed relevant. I suspect there is a change in the behavior in there somewhere and I just failed to find it.

Moving the callback to a before_save resolves the issue. tests continue to pass on 5.2 with the change.

the repeating stack is:

  /home/tongboy/comfortable-mexican-sofa/app/models/comfy/cms/file.rb:54:in `process_attachment'
activesupport (6.0.0.rc2) lib/active_support/callbacks.rb:429:in `block in make_lambda'
activesupport (6.0.0.rc2) lib/active_support/callbacks.rb:239:in `block in halting_and_conditional'
activesupport (6.0.0.rc2) lib/active_support/callbacks.rb:518:in `block in invoke_after'
activesupport (6.0.0.rc2) lib/active_support/callbacks.rb:518:in `each'
activesupport (6.0.0.rc2) lib/active_support/callbacks.rb:518:in `invoke_after'
activesupport (6.0.0.rc2) lib/active_support/callbacks.rb:136:in `run_callbacks'
activesupport (6.0.0.rc2) lib/active_support/callbacks.rb:827:in `_run_save_callbacks'
activerecord (6.0.0.rc2) lib/active_record/callbacks.rb:328:in `create_or_update'
activerecord (6.0.0.rc2) lib/active_record/timestamp.rb:129:in `create_or_update'
activerecord (6.0.0.rc2) lib/active_record/persistence.rb:470:in `save'
activerecord (6.0.0.rc2) lib/active_record/validations.rb:46:in `save'
activerecord (6.0.0.rc2) lib/active_record/transactions.rb:315:in `block in save'
activerecord (6.0.0.rc2) lib/active_record/transactions.rb:375:in `block in with_transaction_returning_status'
activerecord (6.0.0.rc2) lib/active_record/connection_adapters/abstract/database_statements.rb:275:in `transaction'
activerecord (6.0.0.rc2) lib/active_record/transactions.rb:212:in `transaction'
activerecord (6.0.0.rc2) lib/active_record/transactions.rb:366:in `with_transaction_returning_status'
activerecord (6.0.0.rc2) lib/active_record/transactions.rb:315:in `save'
activerecord (6.0.0.rc2) lib/active_record/suppressor.rb:44:in `save'
activerecord (6.0.0.rc2) lib/active_record/persistence.rb:621:in `block in update'
activerecord (6.0.0.rc2) lib/active_record/transactions.rb:375:in `block in with_transaction_returning_status'
activerecord (6.0.0.rc2) lib/active_record/connection_adapters/abstract/database_statements.rb:275:in `transaction'
activerecord (6.0.0.rc2) lib/active_record/transactions.rb:212:in `transaction'
activerecord (6.0.0.rc2) lib/active_record/transactions.rb:366:in `with_transaction_returning_status'
activerecord (6.0.0.rc2) lib/active_record/persistence.rb:619:in `update'
activestorage (6.0.0.rc2) lib/active_storage/attached/one.rb:32:in `attach'

Please let me know if I can add any further tests or anything else. Thanks for the great gem!

@JeremiahChurch JeremiahChurch mentioned this pull request Aug 12, 2019
2 tasks
@gr8bit
Copy link
Contributor

gr8bit commented Aug 12, 2019

That's a little strange imho: if you save the attachment before the comfy File model, the attachment shouldn't be able to attach itself to anything (as the File model doesn't have an id yet).
I thought that's the reason for the after_save hook.

@JeremiahChurch
Copy link
Author

That's a little strange imho: if you save the attachment before the comfy File model, the attachment shouldn't be able to attach itself to anything (as the File model doesn't have an id yet).
I thought that's the reason for the after_save hook.

Totally agree it's strange - I'm used to funny callback chaining behavior problem cases but I've never seen behavior changes in rails version changes like this.

The behavior of active storage #attach as described here https://github.com/rails/rails/blob/master/activestorage/lib/active_storage/attached/one.rb#L22

If the record is persisted and unchanged, the attachment is saved to the database immediately. Otherwise, it'll be saved to the DB when the record is next saved.

lines up with the behavior I'm seeing - looks like if #attach is called before the parent record is saved then the save operation for the file happens when the parent is saved. The order of operations before and after the before/after callback change remains the same - the Comfy::Cms::File is created before the ActiveStorage::Blob and ActiveStorage::Attachment.

@gr8bit
Copy link
Contributor

gr8bit commented Aug 13, 2019

In fact, Rails 6's behaviour is better imho, but your PR also changed how the attachment save code behaves on 5.2... What do you think about conditionally adapting the hook for Rails <6, >=6?

@JeremiahChurch
Copy link
Author

In fact, Rails 6's behaviour is better imho, but your PR also changed how the attachment save code behaves on 5.2... What do you think about conditionally adapting the hook for Rails <6, >=6?

Happy to add backwards compatibility - just pushed a commit for that. I didn't see any existing standards to use as far as styling/methodology for version checking, please let me know if you'd like me to make any adjustments to how I did it.

@morgant
Copy link
Contributor

morgant commented Sep 18, 2019

@tongboy Thanks for this fix! This needs to be applied to Comfy::Cms::Fragment as well.

@JeremiahChurch
Copy link
Author

@morgant PR updated!

app/models/comfy/cms/file.rb Outdated Show resolved Hide resolved
@nitsujri
Copy link
Contributor

Hi guys, any thoughts on how to get the test/test-migration to pass? Really looking forward to this PR merged in!

@jtmkrueger
Copy link

The test failures all appear to be a gem issue, maybe it just needs to be rebased?

GBH pushed a commit that referenced this pull request Dec 9, 2019
* fix rails 6 save loop callback stack

* update to support backward compatibility

* use proc

* add same change to fragment

* code review

* Upgrading sqlite3 to 1.4
@morgant
Copy link
Contributor

morgant commented Dec 9, 2019

PR #899, which was a duplicate of this, has been merged, so this can probably be closed.

@mattpolito
Copy link

I don't believe this is actually fixed, at least for a record that is being updated.

The before_save hook calling attach on a persisted record ends up calling update once it gets inside attach which calls the hook... so on and so on.

Since it is a before_save, just setting the attachment should resolve this.

def process_attachment
  return if @file.blank?
  self.attachment = @file
end

@Fodoj
Copy link

Fodoj commented Nov 17, 2021

I can confirm it's not fixed for updating, which can be verified with export/import of seeds

Fodoj added a commit to mkdev-me/comfortable-mexican-sofa that referenced this pull request Nov 17, 2021
gr8bit added a commit to bichinger/comfortable-mexican-sofa that referenced this pull request Mar 31, 2022
…a into bichinger

* 'master' of github.com:bichinger/comfortable-mexican-sofa:
  fix seeds exporter (comfy#911)
  Bump comfortable_mexican_sofa to 2.0.19
  fixing issue with rack 2.0.8
  will this works with sprokets 4?
  syncing locale files
  feat(I18n): added arabic locale (comfy#897)
  bumping puma
  bumping sqlite gem
  rails 6 needs ruby 2.5+
  wip getting things not crash in for rails 6
  Duplicate comfy#892 - Rails 6 save stack too deep on File upload (comfy#899)
  bug with when there are no pages
  updating test matrix

# Conflicts:
#	Gemfile
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

Successfully merging this pull request may close these issues.

8 participants