Should not mutate options[:file_name]

[dustMason]

The combination of Proc#curry and the is_a?(Proc) conditional create a bug that causes the file name to be set once (the first time a file upload is handled by the Rails process) and then never updated again.

Proc#curry's docs say:

If a sufficient number of arguments are supplied, it passes the supplied arguments to the original proc and returns the result.

In other words, curry does not always return a Proc. Because the Proc's arity is enforced as 1, this means that it will get set to a String after the first run.

This also causes the curious bug of always firing the file_name deprecation warning on all but the first request handled by this code.

A better solution is to simply call the proc on demand.

[y9v]

@dustMason thank you for your PR.

This is a breaking change, and it has to go to 2.5.0 (there's even an open issue for that)

#50

What I don't understand is the expected behaviour. You write that the file name of the upload does not change after being set once. Do you expect the file to be renamed?

Also regarding a weird bug with deprecation warning on the first call: can you please give me more info on that?

[dustMason]

@lebedev-yury I think I did a poor job explaining the issue. It was a challenge to track down – I'll take another try at describing it.

Looking at your code and the example in the readme, I think the expected behavior is to have the file_name Proc run every time the #{attribute}= method is called.

In other words, given a model class:

class Item < ApplicationRecord
  mount_base64_uploader :file, ItemFileUploader, file_name: -> (item) { item.safe_name }

  def safe_name
    name.parameterize
  end
end

and two instances of that class:

one = Item.new name: "item one"
two = Item.new name: "item two"

I would expect the file uploads of each to have different file names. However the bug makes it so they each have the same name:

one.file = "#{base_64_encoded_file}"
one.save
puts one.file.file.filename # => "item-one.ext"
two.file = "#{base_64_encoded_file}"
two.save
puts two.file.file.filename # => "item-one.ext"

The reason this happens is that the body of the #{attribute}= method mutates the file_name option, setting it to a String. I think you intended to set it to a Proc, but as mentioned before, curry returns a String in this case.

curry only returns another Proc if there are still more arguments left on the given Proc. Since you make sure the given Proc has an arity of 1, this code will never return a Proc.

This is what causes the deprecation warning issue as well. The first call does not produce the warning, because the Proc has not yet been curried. The second and further calls do produce the warning, because the file_name option has been set to a String.

I hope this makes sense!

EDIT: Looking again, I see that I did not fix the problem of mutating options[:file_name] 🤦‍♂️. I will make another commit.

I see now that this change prevents String file_name values from working, so it needs an additional fix for that. Once we agree on the fix I can create another commit. I did not intend this to be a breaking change. EDIT: My latest commit restores the ability to give a String for options[:file_name].

[dustMason]

@lebedev-yury let me know if there's anything more I can do to resolve this

[y9v]

Well, the build is failing, rubocop is complaining.

On top of that a test case replicating the issue is missing.

Maybe you need some help with that?

[dustMason]

Done.

I tried to add a spec initially, but gave up when I ran into the gem dependency conflict. Turns out it was an easy fix.

I've added the spec describing the issue although at this point it's just a regression test against what ended up being an obscure bug. Up to you on whether or not you want to keep in the test suite. It does, at least, show the failure when run against the previous version of adapter.rb.

I also used #curry in a way that illustrates the behavior explained in my other comments. Feel free to change that too if you think it's silly.

Added specs to assert the deprecation warning, too, since that turned out to be somewhat related.

[y9v]

@dustMason great job! I'm merging this and releasing a new tiny version.

Thank you for your contribution!

[y9v]

bb6b141