Allow an ActiveStorage attachment to be removed via a form post

[natematykiewicz]

Motivation / Background

We use ActiveStorage attachments via Direct Uploads, which set the attachment value to an ActiveStorage::Blob signed id. We also have the ability to remove an attachment (such as an avatar) in the UI via a "clear" button. This button simply sets the attachment to an empty string (via a hidden field tag), since a form cannot send a nil param. However, we found that this causes an ActiveSupport::MessageVerifier::InvalidSignature: mismatched digest error to be raised, because it's being treated as a signed blob id.

Detail

This PR changes the #{attachment}= ActiveRecord method to treat an empty string just like it treats a nil, and use ActiveStorage::Attached::Changes::DeleteOne instead of ActiveStorage::Attached::Changes::CreateOne.

Now these two are treated the same:

User.first.update!(avatar: nil)
User.first.update!(avatar: "")

Additional information

Without this change, you have to do things like this in all of your controllers that accept direct uploads, which is quite tedious:

def user_params
  params
    .require(:user)
    .permit(:first_name, :last_name, :avatar).tap do |permitted|
      permitted[:avatar] = nil if permitted[:avatar] == ''
    end
end

When adding tests, I simply copy/pasted the tests that test assigning nil, and modified them to be "" tests.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[byroot]

One one hand I don't like that we have to deal with HTML form concerns on the model layer, but on the other I'm forced to admit we already do a lot of that stuff (typically how Active Record converts booleans).

So I'd tend to approve this. I'll see if I can find a second opinion, if not I'll merge in a few days.

[natematykiewicz]

@byroot Agreed, but I'm not sure how to handle it otherwise. The controller layer (where I'd prefer to handle just converting "" to nil) doesn't know that this is an ActiveStorage field and that an empty string means nil.

This does get into me really never being a fan of empty strings, and always wanting string columns to either be present? or nil?, but that's another can of worms. I feel like empty strings always cause performance issues because developers litter their codebase with present? / presence checks because they didn't .presence the strings on write. A nil is a lot faster to coalesce to a default value than a string that's empty / only whitespace (blank?), as it doesn't require a regex scan or even a string allocation.

All that to say, if the ActionController::Parameters just converted blank? string params to nil, this would all "just work". But obviously that has some pretty major backwards compatibility issues.

[guilleiguaran]

I'm approving this since I can't imagine better solutions to the problem but still open for discussion if someone has a better proposal or other suggestions.

[simi]

🤔 Any reason to not use blank??

[natematykiewicz]

I simply wanted to add support for empty strings, since that’s how HTML form fields work when they have no value. The previous code checked for nil? instead of being falsely, which I assumed was intentional.

Therefore, I feel like whitespace-only strings and false should still raise an ActiveSupport::MessageVerifier::InvalidSignature: mismatched digest.

Additionally, blank? is about 2.17x slower than what I've got here, when given a signed id:

# frozen_string_literal: true

require 'benchmark/ips'
require 'active_support'

str = +'BAh7CEkiCGdpZAY6BkVUSSIbZ2lkOi8vZHdlbGwtd2ViL1VzZXIvMQY7AFRJIgxwdXJwb3NlBjsAVEkiDGRlZmF1bHQGOwBUSSIPZXhwaXJlc19hdAY7AFRJIh0yMDIzLTA3LTAyVDEzOjUwOjE2LjM4MVoGOwBU--4335d744fdc394d750210728675cb8ef9e7e0ef0'

Benchmark.ips do |x|
  x.report('==') { str.nil? || str == '' }
  x.report('blank?') { str.blank? }

  x.compare!
end

Warming up --------------------------------------
                  ==     1.750M i/100ms
              blank?   802.528k i/100ms
Calculating -------------------------------------
                  ==     17.498M (± 0.5%) i/s -     87.495M in   5.000509s
              blank?      8.052M (± 0.9%) i/s -     40.929M in   5.083819s

Comparison:
                  ==: 17497713.1 i/s
              blank?:  8051557.4 i/s - 2.17x  slower

blank? would solve Jean's concerns of mixing HTML form concerns in the model layer, but it would come at the expense of allowing more values to not raise an error (which feels incorrect), and being slower.

[rafaeldev]

@natematykiewicz that exception is raising for nested model form submit.

Do you have any clue where it could be produced?

parameters = params.require(:post).permit(:name, comments_attributes: [:text, :image])

applying your tip for skip (looking for each comment and set to nil) I fixed it.

[natematykiewicz]

This PR is a feature of Rails 7.1. If you're on Rails 7.0 or lower and submitting "" as the param, you'll get this error. Are you on Rails 7.1 or something older?

[rafaeldev]

Ouch, I'm on 7.0.x 🤡
I will prepare to upgrade my app!
Thanks for your quick answer 🫂

[natematykiewicz]

I'm still on 7.0, running running this backport in config/initializers/active_storage_empty_string_backport.rb.

raise 'Remove this backport' if Rails.version.to_f != 7.0

# https://github.com/rails/rails/pull/48339

# Method copy/pasted from here, added ` || attachable == ""`, to match PR 48339's change.
# https://github.com/rails/rails/blob/v7.0.5/activestorage/lib/active_storage/attached/model.rb

ActiveStorage::Attached::Model::ClassMethods.class_eval do
  def has_one_attached(name, dependent: :purge_later, service: nil, strict_loading: false)
    validate_service_configuration(name, service)

    generated_association_methods.class_eval <<-CODE, __FILE__, __LINE__ + 1
      # frozen_string_literal: true
      def #{name}
        @active_storage_attached ||= {}
        @active_storage_attached[:#{name}] ||= ActiveStorage::Attached::One.new("#{name}", self)
      end

      def #{name}=(attachable)
        attachment_changes["#{name}"] =
          if attachable.nil? || attachable == ""
            ActiveStorage::Attached::Changes::DeleteOne.new("#{name}", self)
          else
            ActiveStorage::Attached::Changes::CreateOne.new("#{name}", self, attachable)
          end
      end
    CODE

    has_one :"#{name}_attachment", -> { where(name: name) }, class_name: "ActiveStorage::Attachment", as: :record, inverse_of: :record, dependent: :destroy, strict_loading: strict_loading
    has_one :"#{name}_blob", through: :"#{name}_attachment", class_name: "ActiveStorage::Blob", source: :blob, strict_loading: strict_loading

    scope :"with_attached_#{name}", -> { includes("#{name}_attachment": :blob) }

    after_save { attachment_changes[name.to_s]&.save }

    after_commit(on: %i[ create update ]) { attachment_changes.delete(name.to_s).try(:upload) }

    reflection = ActiveRecord::Reflection.create(
      :has_one_attached,
      name,
      nil,
      { dependent: dependent, service_name: service },
      self
    )
    yield reflection if block_given?
    ActiveRecord::Reflection.add_attachment_reflection(self, name, reflection)
  end
end

Whenever I upgrade to 7.1, I'll just delete that file and things will continue to work the same.

[natematykiewicz]

I don't think that method has changed between 7.0.5 and 7.0.8, but you'll want to double-check. But all it does is add || attachable == "" over the top of the pre-existing method definition.