Add support for multiple file uploads

[jnicklas]

This adds pretty effortless support for multiple file uploads, via a new mount_uploaders method (note the plural-s).

All the tests pass when run against fog 1.23.0, but #1480 causes failure against 1.24.0 which is why this is red on Travis.

I've tried to not change any existing behaviour, but this makes one backwards incompatible change. The behaviour of to_json has been changed when serializing an uploader which is not mounted on a model. This removes the need to override serializable_hash on records, which is super, super hacky and shouldn't have been done in the first place IMO. This drastically simplifies the implementation.

This is a huge change which touches a lot of files and refactors and changes a lot of very old code. As such I'm a bit apprehensive about it, but as a feature this is totally amazing. Drop it in with multiple file upload and it just works.

@bensie et al, please take a look and let me know what you think.

[thiagofm]

I think this is a good change. We can write about that change(to_json behavior) and also release it as a major(1.0+ anyone?).

[bensie]

@jnicklas Awesome...merged. There are two specs that this breaks (from recent PRs I merged yesterday). I marked them pending in f88ebde.

For one of them, it seems after_commit doesn't work as expected with something that came from the multiple uploaders branch. The other ensures that we only delete a file once during the removal process, but it was part of the mounter code you refactored.

If you have a bit to take a peek, I'd appreciate it, otherwise I'll try to dig in next week. Thanks!

[bensie]

@jnicklas Also it looks like this doesn't work on Rails 3.2 :(

https://travis-ci.org/carrierwaveuploader/carrierwave/builds/41521188

Quick fix or going to be brutal?

[jnicklas]

Looks like Rails 3.2 doesn't support the :json column type. The way this is implemented now, I think doesn't mesh well with ActiveRecord's serialize, since both will want to override the getter/setters, so we'll probably have to build in some way of serializing/deserializing the data to make it work on 3.2.

[ardecvz]

Hi, just some notes about the introduced API from random guy:

@jnicklas Have you considered to add a new "multiple" option to the existing mount_uploader method instead of adding the new one ending with S?

My rationale:

1. mount_uploader :Users, AvatarUploader, multiple: true seems more consistent with input type="file" multiple (and corresponding form generation's code) so it should be more familiar to any web developer.
2. mount_uploaders :Users, AvatarUploader Two words in a row that ends with S may distract people's attention and definitely would be a source of small but annoying errors related with typos. And also think about people who quick skimming over the code. With multiple: true developer's intentions seems more clear.
3. You already know it but rubists love KISS' principle for removing possible future burden of code maintaining. I've made just some very raw glances at the code and there seems to be unnecessary code's deduplication in similar methods. For example, this and this or this and this. Finally, from developer's perspective it would be great to have just one entrypoint for foreign API in his models (there's already a bunch of new methods to remember in uploaders).

Until this new and undoubtedly great function gets into stable version, what do you think about multiple: true?

Thank you!

[jnicklas]

I think it would be nice, I was going back and forth between those two options as well. Unfortunately it wouldn't really help with the duplication you pointed out in (3), while all of this code is very similar, if you look closely it is not the exact same. I tried to dry up and extract as much as possible.

Putting both in the same method would make the code quite messy, I think, but just because they are implemented in separate methods doesn't mean they can't have a common API. multiple: true is probably more intuitive, and easier to distinguish.

[ardecvz]

You're absolutely right about the proper code separation, I'm reviewing only from the perspective of API consumers and I just mean that maybe you could win a few keystrokes if you've chosen the option variant (so it was an additional but not important point). No claims or worries about the code, it's great for me.

[ersinakinci]

For the benefit of future generations who are upgrading from < 1.0.0, I want to point out that @jnicklas' comment about how "The behaviour of to_json has been changed when serializing an uploader which is not mounted on a model" is not entirely accurate. This statement is true, but also the changes affect the behavior of to_json when serializing an uploader which is mounted on a model. In other words, to_json's behavior (or more accurately, as_json's behavior) is changed no matter what you do.

The changelog states that this PR introduces a breaking change, but the scope of the breakage is much broader than the original comment would lead one to believe.

I confirmed that my comment is actually incorrect. When mounted, the output of model_instance.to_json is the same. But why? This PR changed as_json's behavior for every uploader. 🤔 ❓ ❓ ❓