Updating record with nested ActiveStorage attachments fails

[akz92]

Complete Description of Issue

Given a form with a nested collection where each element of the collection has an ActiveStorage attachment, calling .sync or .save fails on update without changing at least one of the attached files.

This started happening with Rails 6.1 and is possibly linked to this addition

Steps to reproduce

I have created a tiny Rails app that reproduces this in isolation, the steps to try it are in the README.

Simplified scenario that would cause the issue:

# Models
class Folder < ApplicationRecord
  has_many :documents
end

class Document < ApplicationRecord
  belongs_to :folder
  has_one_attached :file
end

# Forms
class FolderForm < Reform::Form
  collection :documents, form: DocumentForm
end

class DocumentForm < Reform::Form
  property :file
end

Expected behavior

The record should be synced/saved.

Actual behavior

Fails with Could not find or build blob: expected attachable, got ActiveStorage::Attachable::One

System configuration

Reform version: 2.6.0 Rails version: 6.1.4

Full Backtrace of Exception (if any)

activestorage (6.1.4.1) lib/active_storage/attached/changes/create_one.rb:74:in `find_or_build_blob'
activestorage (6.1.4.1) lib/active_storage/attached/changes/create_one.rb:20:in `blob'
activestorage (6.1.4.1) lib/active_storage/attached/changes/create_one.rb:12:in `initialize'
activestorage (6.1.4.1) lib/active_storage/attached/model.rb:65:in `new'
activestorage (6.1.4.1) lib/active_storage/attached/model.rb:65:in `file='
disposable (0.5.0) lib/disposable/expose.rb:33:in `block in accessors!'
disposable (0.5.0) lib/disposable/twin/sync.rb:50:in `block in sync!'
disposable (0.5.0) lib/disposable/twin/definitions.rb:27:in `block in each'
disposable (0.5.0) lib/disposable/twin/definitions.rb:27:in `each'
disposable (0.5.0) lib/disposable/twin/definitions.rb:27:in `each'
disposable (0.5.0) lib/disposable/twin/sync.rb:46:in `sync!'
disposable (0.5.0) lib/disposable/twin/sync.rb:55:in `block (2 levels) in sync!'
disposable (0.5.0) lib/disposable/twin/property_processor.rb:24:in `block in collection!'
disposable (0.5.0) lib/disposable/twin/property_processor.rb:24:in `each'
disposable (0.5.0) lib/disposable/twin/property_processor.rb:24:in `each_with_index'
disposable (0.5.0) lib/disposable/twin/property_processor.rb:24:in `each'
disposable (0.5.0) lib/disposable/twin/property_processor.rb:24:in `collect'
disposable (0.5.0) lib/disposable/twin/property_processor.rb:24:in `collection!'
disposable (0.5.0) lib/disposable/twin/property_processor.rb:16:in `call'
disposable (0.5.0) lib/disposable/twin/sync.rb:55:in `block in sync!'
disposable (0.5.0) lib/disposable/twin/definitions.rb:27:in `block in each'
disposable (0.5.0) lib/disposable/twin/definitions.rb:27:in `each'
disposable (0.5.0) lib/disposable/twin/definitions.rb:27:in `each'
disposable (0.5.0) lib/disposable/twin/sync.rb:46:in `sync!'
disposable (0.5.0) lib/disposable/twin/sync.rb:36:in `sync_models'
disposable (0.5.0) lib/disposable/twin/save.rb:5:in `save'
app/controllers/folders_controller.rb:42:in `block in update'
actionpack (6.1.4.1) lib/action_controller/metal/mime_responds.rb:205:in `respond_to'
app/controllers/folders_controller.rb:39:in `update'
actionpack (6.1.4.1) lib/action_controller/metal/basic_implicit_render.rb:6:in `send_action'
actionpack (6.1.4.1) lib/abstract_controller/base.rb:228:in `process_action'
actionpack (6.1.4.1) lib/action_controller/metal/rendering.rb:30:in `process_action'
actionpack (6.1.4.1) lib/abstract_controller/callbacks.rb:42:in `block in process_action'
activesupport (6.1.4.1) lib/active_support/callbacks.rb:117:in `block in run_callbacks'
actiontext (6.1.4.1) lib/action_text/rendering.rb:20:in `with_renderer'
actiontext (6.1.4.1) lib/action_text/engine.rb:59:in `block (4 levels) in <class:Engine>'
activesupport (6.1.4.1) lib/active_support/callbacks.rb:126:in `instance_exec'
activesupport (6.1.4.1) lib/active_support/callbacks.rb:126:in `block in run_callbacks'
activesupport (6.1.4.1) lib/active_support/callbacks.rb:137:in `run_callbacks'
actionpack (6.1.4.1) lib/abstract_controller/callbacks.rb:41:in `process_action'
actionpack (6.1.4.1) lib/action_controller/metal/rescue.rb:22:in `process_action'
actionpack (6.1.4.1) lib/action_controller/metal/instrumentation.rb:34:in `block in process_action'
activesupport (6.1.4.1) lib/active_support/notifications.rb:203:in `block in instrument'
activesupport (6.1.4.1) lib/active_support/notifications/instrumenter.rb:24:in `instrument'
activesupport (6.1.4.1) lib/active_support/notifications.rb:203:in `instrument'
actionpack (6.1.4.1) lib/action_controller/metal/instrumentation.rb:33:in `process_action'
actionpack (6.1.4.1) lib/action_controller/metal/params_wrapper.rb:249:in `process_action'
activerecord (6.1.4.1) lib/active_record/railties/controller_runtime.rb:27:in `process_action'
actionpack (6.1.4.1) lib/abstract_controller/base.rb:165:in `process'
actionview (6.1.4.1) lib/action_view/rendering.rb:39:in `process'
actionpack (6.1.4.1) lib/action_controller/metal.rb:190:in `dispatch'
actionpack (6.1.4.1) lib/action_controller/metal.rb:254:in `dispatch'
actionpack (6.1.4.1) lib/action_dispatch/routing/route_set.rb:50:in `dispatch'
actionpack (6.1.4.1) lib/action_dispatch/routing/route_set.rb:33:in `serve'
actionpack (6.1.4.1) lib/action_dispatch/journey/router.rb:50:in `block in serve'
actionpack (6.1.4.1) lib/action_dispatch/journey/router.rb:32:in `each'
actionpack (6.1.4.1) lib/action_dispatch/journey/router.rb:32:in `serve'
actionpack (6.1.4.1) lib/action_dispatch/routing/route_set.rb:842:in `call'
rack (2.2.3) lib/rack/tempfile_reaper.rb:15:in `call'
rack (2.2.3) lib/rack/etag.rb:27:in `call'
rack (2.2.3) lib/rack/conditional_get.rb:40:in `call'
rack (2.2.3) lib/rack/head.rb:12:in `call'
actionpack (6.1.4.1) lib/action_dispatch/http/permissions_policy.rb:22:in `call'
actionpack (6.1.4.1) lib/action_dispatch/http/content_security_policy.rb:18:in `call'
rack (2.2.3) lib/rack/session/abstract/id.rb:266:in `context'
rack (2.2.3) lib/rack/session/abstract/id.rb:260:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/cookies.rb:689:in `call'
activerecord (6.1.4.1) lib/active_record/migration.rb:601:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/callbacks.rb:27:in `block in call'
activesupport (6.1.4.1) lib/active_support/callbacks.rb:98:in `run_callbacks'
actionpack (6.1.4.1) lib/action_dispatch/middleware/callbacks.rb:26:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/executor.rb:14:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/actionable_exceptions.rb:18:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/debug_exceptions.rb:29:in `call'
web-console (4.1.0) lib/web_console/middleware.rb:132:in `call_app'
web-console (4.1.0) lib/web_console/middleware.rb:28:in `block in call'
web-console (4.1.0) lib/web_console/middleware.rb:17:in `catch'
web-console (4.1.0) lib/web_console/middleware.rb:17:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/show_exceptions.rb:33:in `call'
railties (6.1.4.1) lib/rails/rack/logger.rb:37:in `call_app'
railties (6.1.4.1) lib/rails/rack/logger.rb:26:in `block in call'
activesupport (6.1.4.1) lib/active_support/tagged_logging.rb:99:in `block in tagged'
activesupport (6.1.4.1) lib/active_support/tagged_logging.rb:37:in `tagged'
activesupport (6.1.4.1) lib/active_support/tagged_logging.rb:99:in `tagged'
railties (6.1.4.1) lib/rails/rack/logger.rb:26:in `call'
sprockets-rails (3.2.2) lib/sprockets/rails/quiet_assets.rb:13:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/remote_ip.rb:81:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/request_id.rb:26:in `call'
rack (2.2.3) lib/rack/method_override.rb:24:in `call'
rack (2.2.3) lib/rack/runtime.rb:22:in `call'
activesupport (6.1.4.1) lib/active_support/cache/strategy/local_cache_middleware.rb:29:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/executor.rb:14:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/static.rb:24:in `call'
rack (2.2.3) lib/rack/sendfile.rb:110:in `call'
actionpack (6.1.4.1) lib/action_dispatch/middleware/host_authorization.rb:98:in `call'
rack-mini-profiler (2.3.2) lib/mini_profiler/profiler.rb:384:in `call'
webpacker (5.4.2) lib/webpacker/dev_server_proxy.rb:25:in `perform_request'
rack-proxy (0.7.0) lib/rack/proxy.rb:63:in `call'
railties (6.1.4.1) lib/rails/engine.rb:539:in `call'
puma (5.4.0) lib/puma/configuration.rb:249:in `call'
puma (5.4.0) lib/puma/request.rb:77:in `block in handle_request'
puma (5.4.0) lib/puma/thread_pool.rb:340:in `with_force_shutdown'
puma (5.4.0) lib/puma/request.rb:76:in `handle_request'
puma (5.4.0) lib/puma/server.rb:440:in `process_client'
puma (5.4.0) lib/puma/thread_pool.rb:147:in `block in spawn_thread'

[apotonick]

Hm, this is interesting. As you might've seen in Reform's source, all #sync! does is calling model.file= <here is the File object>. So the problem must be in the attachment object. What has changed here from Rails 6.0 to 6.1? The blob.identify_without_saving is called in the constructor, so this can't be the root of the problem?!

[akz92]

I believe blob.identify_without_saving is the root of the problem as it tries to find_or_build_blob and that only accepts one of: ActiveStorage::Blob, ActionDispatch::Http::UploadedFile, Rack::Test::UploadedFile, Hash or String. For an unchanged attachment we pass in an ActiveStorage::Attachable::One.

To be honest I feel that calling model.file = model.file should not throw any errors (it does!), but that seems to be Rails's desired API as submitting the same form without Reform does work (as it does not reassign the attachment).

Am I correct to assume that the relevant #sync! comes from Disposable::Twin::Sync? Do you think it may be possible to add a guard clause here skipping if the value has not changed?

[apotonick]

Actually, I believe that Disposable only calls the setter if the value has changed, but I might be wrong. In your case, whatsoever, the problem seems to be the collection of files and here the setter gets always called. I'm pretty sure the problem wouldn't arise if it was just one property field for the upload. Let me check in the Disposable gem how we could solve this.

[apotonick]

Or, wait a minute, did you say that model.file = model.file also breaks?

[akz92]

Yes, calling model.file = model.file throws the same error I reported here. On Rails 6.0 the assignment call succeeds but the subsequent .save call throws said error. That's why I believe the addition to the constructor is what causes this issue.

[yogeshjain999]

@akz92 could you try assigning file.blob to model.file ?

[akz92]

@yogeshjain999 yes that does work as file.blob returns an instance of ActiveStorage::Blob which is one of the supported classes.

[yogeshjain999]

Cool 🍻 Closing this issue.

[akz92]

@yogeshjain999 I'm sorry if it sounded like that solves the initial issue. The discussion about model.file = model.file was tangential and that call can't be changed on the application level. This call is made by Reform's #sync! and since the scenario where this happens is when the attachment hasn't changed we can't use, lets say, a populator to fix it.

[apotonick]

@akz92 Could you try this? https://github.com/apotonick/disposable/blob/master/lib/disposable/twin/sync.rb#L133

This module included in the FileForm will prevent #sync! from writing if the property hasn't changed.

[akz92]

@apotonick good call, that does in fact solve the issue! Please let me know if/how I can help either document or fix it for good.

Thank you both very much for the prompt attention to this issue!

[akz92]

I'm afraid I jumped to conclusions too soon. Adding feature Sync::SkipUnchanged does in fact prevent that failure but at the cost of ignoring any changed files so it seems that alone isn't a viable solution.

[apotonick]

Hm, that sounds like unintended behavior (a "bug"). Can you check disposable's tests if we cover collections where only one property is changed in combo with SkipUnchanged?