Not marked as changed if validation failed

[wata727]

While testing CarrierWave v3, we encountered an issue where saving files with invalid extensions did not fail correctly.
After digging into the details, it seems that due to #2658, it is not marked as changed if validation fails.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "activerecord", "7.1.3.2"
  gem "sqlite3"
  # gem "carrierwave", "2.2.5"
  gem "carrierwave", "3.0.5"
end

require "active_record"
require "carrierwave"
require "carrierwave/orm/activerecord"
require "minitest/autorun"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
    t.string :name
  end

  create_table :profiles, force: true do |t|
    t.integer :user_id
    t.string :avatar
  end
end

class AvatarUploader < CarrierWave::Uploader::Base
  storage :file

  def extension_allowlist
    %w(jpg jpeg gif png)
  end
end

class User < ActiveRecord::Base
  has_one :profile, autosave: true
end

class Profile < ActiveRecord::Base
  mount_uploader :avatar, AvatarUploader
end

class BugTest < Minitest::Test
  def test_validation
    user = User.create!(name: "Taro", profile: Profile.new)
    user.profile.avatar = Tempfile.new(["test", ".txt"])
    
    assert user.invalid?
  end

  def test_changed
    user = User.create!(name: "Taro", profile: Profile.new)
    user.profile.avatar = Tempfile.new(["test", ".txt"])
    
    assert user.profile.changed?
  end
end

# Running:

FF

Finished in 0.016239s, 123.1603 runs/s, 123.1603 assertions/s.

  1) Failure:
BugTest#test_validation [test.rb:53]:
Expected false to be truthy.

  2) Failure:
BugTest#test_changed [test.rb:60]:
Expected false to be truthy.

2 runs, 2 assertions, 2 failures, 0 errors, 0 skips

v2.2.5 works correctly:

# Running:

..

Finished in 0.024955s, 80.1443 runs/s, 80.1443 assertions/s.

2 runs, 2 assertions, 0 failures, 0 errors, 0 skips

A temporary identifier is generated based on the cached file, but if the save fails like this, the identifier will not be generated and I think this issue will occur.
https://github.com/carrierwaveuploader/carrierwave/blob/v3.0.5/lib/carrierwave/mounter.rb#L100
https://github.com/carrierwaveuploader/carrierwave/blob/v3.0.5/lib/carrierwave/mounter.rb#L98

P.S. Thank you for maintaining CarrierWave! We are always helped by your wonderful work :)

[mshibuya]

Sorry for the late response 🙇
You're right. #extension_allowlist raises CarrierWave::IntegrityError then skips #cache, resulting in the attribute value unchanged.
How important is this behavior of being able to detect a change in validation failure in your usecase? I mean, there can be an explanation that the attribute value was not changed because assigning an invalid value was rejected. But if this behavior is crucial we may need to find a way to keep it.

By the way, this single-file snippet you used is very useful for reproducing issues. Let me use this way in other places 👍

[wata727]

Thank you for your reply. For example, if you have models like the one above, you may run into issues when using nested attributes to submit images from a form. Imagine params like below are submitted from the form:

{
  id: 1,
  profile_attributes: {
    id: 2,
    avatar: <...> # image
  }
}

The controller handles it as follows:

def update
  @user = User.find(params[:id])
  if @user.update(params.permit(:profile_attributes))
    redirect_to(@user)
  else
    render :edit, status: :unprocessable_entity
  end
end

In this case, if the passed image is invalid, the update should fail and the user should be given feedback on the cause of the error. However, since user.invalid? does not return true, the error is ignored and the update succeeds.

Intuitively, similar to Active Model, I believe it's better to keep the invalid value assigned rather than rejecting it. That's why I recommend changing the identifier even on assignment of invalid values. What do you think?

By the way, this single-file snippet you used is very useful for reproducing issues. Let me use this way in other places 👍

👍
This snippet was inspired by Rails. It's good practice to be able to reproduce the issue in a single file. You might want to start with an issue template.