carrierwaveuploader / carrierwave

Classier solution for file uploads for Rails, Sinatra and other Ruby web frameworks
https://github.com/carrierwaveuploader/carrierwave
8.78k stars 1.66k forks source link

3.0.0 Breaking change with enable_processing behavior of versions #2676

Closed rajyan closed 1 year ago

rajyan commented 1 year ago

Summary

This change https://github.com/carrierwaveuploader/carrierwave/commit/1531a67366f0e25e3d298133a72c81b6c9c0dc83 led to a breaking change with enable_processing configuration.

Before this change we could change versions' enable_processing by calling the mounted column uploader's config like in the README.

MyUploader.enable_processing = true

Although, 3.0.0 has a complex change with this behavior. The above configuration sometimes works, but sometimes not.

Minimum reproducible example

class TestImageUploader < CarrierWave::Uploader::Base
  version :test do
  end
end

class TestImage < ActiveRecord::Base
  mount_uploader :image, TestImageUploader
end

i=TestImage.new
i.image.class.instance_variables
i.image.enable_processing
i.image.class.instance_variables # instance_variable `@enable_processing` is set

i.image.test.class.instance_variables
i.image.test.enable_processing
i.image.test.class.instance_variables # instance_variable `@enable_processing` is not set before 3.0.0 but set in 3.0.0

Problem

We noticed this change with our test suite. If we have called the mounted column somewhere in our test, we cannot change the enable_processing configuration of the versions afterwards in other tests.

RSpec.describe TestImageUploader do
  describe 'test1' do
    let(:test_image) { build(:test_image) }

    it 'run something that invokes enable_processing' do
      test_image.image = File.open('foo.jpg', 'rb')
    end
  end

  describe 'test2' do
    let(:test_image) { build(:test_image) }

    before do
      TestImageUploader.enable_processing = true
    end

    after do
      TestImageUploader.enable_processing = false
    end

    it 'can change enable_processing' do
      expect(test_image.image.test.enable_processing).to be true
    end
  end
end

test2 succeeds if you run it individually, but fails if you run after test1.

I don't have a valid use-case in production, but if someone is changing enable_processing dynamically in their application, the same issue might happen.

I believe restoring these lines https://github.com/carrierwaveuploader/carrierwave/commit/1531a67366f0e25e3d298133a72c81b6c9c0dc83#diff-c3e7243412c35dfefcaf2569736474b74402cc34cb550e472aacef867d91da18L90-L97 in the Builder can fix this issue, but want some opinions before proceeding.

rajyan commented 1 year ago

There is a workaround

test_image.image.test.class.enable_processing = true
mshibuya commented 1 year ago

My intention was that the inheritance structure change in https://github.com/carrierwaveuploader/carrierwave/commit/1531a67366f0e25e3d298133a72c81b6c9c0dc83 will make the #enable_processing patch unnecessary, but actually it wasn't as you mentioned. I've restored the #enable_processing patch back and now this is fixed.

Thank you for reporting 👍

rajyan commented 1 year ago

@mshibuya

Thank you for your swift replies!