Skip to content

Commit

Permalink
Preserve existing attachment assignment behavior for upgraded apps
Browse files Browse the repository at this point in the history
Assigning to a collection of attachments appends rather than replacing, as in 5.2. Existing 5.2 apps that rely on this behavior will no longer break when they're upgraded to 6.0.

For apps generated on 6.0 or newer, assigning replaces the existing attachments in the collection. #attach should be used to add new attachments to the collection without removing existing ones.

I expect that we'll deprecate the old behavior in 6.1.

Closes #36374.
  • Loading branch information
georgeclaghorn committed Jul 20, 2019
1 parent 876548a commit 400b210
Show file tree
Hide file tree
Showing 9 changed files with 117 additions and 15 deletions.
18 changes: 13 additions & 5 deletions activestorage/lib/active_storage.rb
Expand Up @@ -43,18 +43,26 @@ module ActiveStorage

mattr_accessor :logger
mattr_accessor :verifier
mattr_accessor :variant_processor, default: :mini_magick

mattr_accessor :queues, default: {}

mattr_accessor :previewers, default: []
mattr_accessor :analyzers, default: []
mattr_accessor :variant_processor, default: :mini_magick
mattr_accessor :analyzers, default: []

mattr_accessor :paths, default: {}
mattr_accessor :variable_content_types, default: []

mattr_accessor :variable_content_types, default: []
mattr_accessor :binary_content_type, default: "application/octet-stream"
mattr_accessor :content_types_to_serve_as_binary, default: []
mattr_accessor :content_types_allowed_inline, default: []
mattr_accessor :binary_content_type, default: "application/octet-stream"
mattr_accessor :content_types_allowed_inline, default: []

mattr_accessor :service_urls_expire_in, default: 5.minutes

mattr_accessor :routes_prefix, default: "/rails/active_storage"

mattr_accessor :replace_on_assign_to_many, default: false

module Transformers
extend ActiveSupport::Autoload

Expand Down
17 changes: 12 additions & 5 deletions activestorage/lib/active_storage/attached/model.rb
Expand Up @@ -93,12 +93,19 @@ def #{name}
end
def #{name}=(attachables)
attachment_changes["#{name}"] =
if attachables.nil? || Array(attachables).none?
ActiveStorage::Attached::Changes::DeleteMany.new("#{name}", self)
else
ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, attachables)
if ActiveStorage.replace_on_assign_to_many
attachment_changes["#{name}"] =
if attachables.nil? || Array(attachables).none?
ActiveStorage::Attached::Changes::DeleteMany.new("#{name}", self)
else
ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, attachables)
end
else
if !attachables.nil? || Array(attachables).any?
attachment_changes["#{name}"] =
ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, #{name}.blobs + attachables)
end
end
end
CODE

Expand Down
2 changes: 2 additions & 0 deletions activestorage/lib/active_storage/engine.rb
Expand Up @@ -79,6 +79,8 @@ class Engine < Rails::Engine # :nodoc:
ActiveStorage.service_urls_expire_in = app.config.active_storage.service_urls_expire_in || 5.minutes
ActiveStorage.content_types_allowed_inline = app.config.active_storage.content_types_allowed_inline || []
ActiveStorage.binary_content_type = app.config.active_storage.binary_content_type || "application/octet-stream"

ActiveStorage.replace_on_assign_to_many = app.config.active_storage.replace_on_assign_to_many || false
end
end

Expand Down
26 changes: 26 additions & 0 deletions activestorage/test/models/attached/many_test.rb
Expand Up @@ -269,6 +269,24 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
end
end

test "updating an existing record with attachments when appending on assign" do
append_on_assign do
@user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg")

assert_difference -> { @user.reload.highlights.count }, +2 do
@user.update! highlights: [ create_blob(filename: "whenever.jpg"), create_blob(filename: "wherever.jpg") ]
end

assert_no_difference -> { @user.reload.highlights.count } do
@user.update! highlights: [ ]
end

assert_no_difference -> { @user.reload.highlights.count } do
@user.update! highlights: nil
end
end
end

test "attaching existing blobs to a new record" do
User.new(name: "Jason").tap do |user|
user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg")
Expand Down Expand Up @@ -538,4 +556,12 @@ def highlights
User.remove_method :highlights
end
end

private
def append_on_assign
ActiveStorage.replace_on_assign_to_many, previous = false, ActiveStorage.replace_on_assign_to_many
yield
ensure
ActiveStorage.replace_on_assign_to_many = previous
end
end
10 changes: 6 additions & 4 deletions guides/source/6_0_release_notes.md
Expand Up @@ -672,6 +672,12 @@ Please refer to the [Changelog][active-storage] for detailed changes.
is saved instead of immediately.
([Pull Request](https://github.com/rails/rails/pull/33303))

* Optionally replace existing files instead of adding to them when assigning to
a collection of attachments (as in `@user.update!(images: [ … ])`). Use
`config.active_storage.replace_on_assign_to_many` to control this behavior.
([Pull Request](https://github.com/rails/rails/pull/33303),
[Pull Request](https://github.com/rails/rails/pull/36716))

* Add the ability to reflect on defined attachments using the existing
Active Record reflection mechanism.
([Pull Request](https://github.com/rails/rails/pull/33018))
Expand All @@ -688,10 +694,6 @@ Please refer to the [Changelog][active-storage] for detailed changes.
`mini_magick` directly.
([Pull Request](https://github.com/rails/rails/pull/32471))

* Replace existing images instead of adding to them when updating an
attached model via `update` or `update!` with, say, `@user.update!(images: [ … ])`.
([Pull Request](https://github.com/rails/rails/pull/33303))

Active Model
------------

Expand Down
5 changes: 4 additions & 1 deletion guides/source/configuring.md
Expand Up @@ -881,7 +881,9 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla
config.active_storage.routes_prefix = '/files'
```

The default is `/rails/active_storage`
The default is `/rails/active_storage`.

* `config.active_storage.replace_on_assign_to_many` determines whether assigning to a collection of attachments declared with `has_many_attached` replaces any existing attachments or appends to them. The default is `true`.

### Results of `load_defaults`

Expand Down Expand Up @@ -917,6 +919,7 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla
- `config.active_job.return_false_on_aborted_enqueue`: `true`
- `config.active_storage.queues.analysis`: `:active_storage_analysis`
- `config.active_storage.queues.purge`: `:active_storage_purge`
- `config.active_storage.replace_on_assign_to_many`: `true`
- `config.active_record.collection_cache_versioning`: `true`

### Configuring a Database
Expand Down
48 changes: 48 additions & 0 deletions guides/source/upgrading_ruby_on_rails.md
Expand Up @@ -398,6 +398,54 @@ config.load_defaults "6.0"
config.autoloader = :classic
```

### Active Storage assignment behavior change

In Rails 5.2, assigning to a collection of attachments declared with `has_many_attached` appended new files:

```ruby
class User < ApplicationRecord
has_many_attached :highlights
end

user.highlights.attach(filename: "funky.jpg", ...)
user.higlights.count # => 1

blob = ActiveStorage::Blob.create_after_upload!(filename: "town.jpg", ...)
user.update!(highlights: [ blob ])

user.highlights.count # => 2
user.highlights.first.filename # => "funky.jpg"
user.highlights.second.filename # => "town.jpg"
```

With the default configuration for Rails 6.0, assigning to a collection of attachments replaces existing files
instead of appending to them. This matches Active Record behavior when assigning to a collection association:

```ruby
user.highlights.attach(filename: "funky.jpg", ...)
user.highlights.count # => 1

blob = ActiveStorage::Blob.create_after_upload!(filename: "town.jpg", ...)
user.update!(highlights: [ blob ])

user.highlights.count # => 1
user.highlights.first.filename # => "town.jpg"
```

`#attach` can be used to add new attachments without removing the existing ones:

```ruby
blob = ActiveStorage::Blob.create_after_upload!(filename: "town.jpg", ...)
user.highlights.attach(blob)

user.highlights.count # => 2
user.highlights.first.filename # => "funky.jpg"
user.highlights.second.filename # => "town.jpg"
```

Opt in to the new default behavior by setting `config.active_storage.replace_on_assign_to_many` to `true`.
The old behavior will be deprecated in Rails 6.1 and removed in a subsequent release.

Upgrading from Rails 5.1 to Rails 5.2
-------------------------------------

Expand Down
2 changes: 2 additions & 0 deletions railties/lib/rails/application/configuration.rb
Expand Up @@ -145,6 +145,8 @@ def load_defaults(target_version)
if respond_to?(:active_storage)
active_storage.queues.analysis = :active_storage_analysis
active_storage.queues.purge = :active_storage_purge

active_storage.replace_on_assign_to_many = true
end

if respond_to?(:active_record)
Expand Down
Expand Up @@ -26,6 +26,10 @@
# Rails.application.config.active_storage.queues.analysis = :active_storage_analysis
# Rails.application.config.active_storage.queues.purge = :active_storage_purge

# When assigning to a collection of attachments declared via `has_many_attached`, replace existing
# attachments instead of appending. Use #attach to add new attachments without replacing existing ones.
# Rails.application.config.active_storage.replace_on_assign_to_many = true

# Use ActionMailer::MailDeliveryJob for sending parameterized and normal mail.
#
# The default delivery jobs (ActionMailer::Parameterized::DeliveryJob, ActionMailer::DeliveryJob),
Expand Down

0 comments on commit 400b210

Please sign in to comment.