Peter Marklund

Peter Marklund's Home

Thu November 18, 2010
Programming

Rails 3.0.3 Backwards Incompatible for File Uploads

We upgraded from Rails 3.0.1 to Rails 3.0.3 yesterday and sadly this broke file uploads for us in production. What do we conclude from this other than that we are missing integration tests for file uploads? Well, one noteworthy thing is that the Rails 3.0.3 release is not quite as backwards compatible as announced, at least not when it comes to file uploads. In Rails file fields in multipart forms are exposed in the params hash as an ActionDispatch::Http::UploadedFile object. As of a commit by tenderlove on the 5:th of October this class no longer inherits from Tempfile but instead delegates to Tempfile (i.e. tenderlove favored composition over inheritance like the GoF prescribes). Unfortunately, the UploadedFile class only delegates a handful of methods and not two important methods that we happened to be using, namely open and path. So now, for example when getting the path of the tempfile we have to fetch the tempfile first:

  params[:my_file].tempfile.path

Comments

José Valim said over 3 years ago:

Great post Peter! Maybe you could provide a patch or a pull request that would delegate these two methods to the tempfile as well?

--------------------------------------------------------------------------------

Peter Marklund said over 3 years ago:

Thanks Jose. I would be happy to provide a patch but I'm not sure how best to resolve the issue as there are many more methods in Tempfile that other apps might be using. It turns out that I was slightly wrong about the change. In Rails 3.0.1 the file object was actually a Tempfile with UploadedFile being a module that was mixed in. For full backwards compatibiliy the file object would need to continue to behave exactly like a Tempfile.

--------------------------------------------------------------------------------

sigerello said over 3 years ago:

There is quick fix for people who use Paperclip:
https://github.com/thoughtbot/paperclip/issues/issue/346

# put this somewhere into config/initializers/

if defined? ActionDispatch::Http::UploadedFile
ActionDispatch::Http::UploadedFile.send(:include, Paperclip::Upfile)
end

--------------------------------------------------------------------------------

David Backeus said over 3 years ago:

Good catch. Had the same thing break in our app.

--------------------------------------------------------------------------------

hipertracker said over 3 years ago:

This is not only one backwards incompatible change in public API. The another one is in models. Previous working MyModel.select(:foo,:bar) does not work in 3.0.3, it has to be MyModel([:foo,:bar]) now.

--------------------------------------------------------------------------------

Aaron Patterson said over 3 years ago:

Hi,

The problem with mixing in a module on every request is that it breaks the VM's method cache causing request processing to slow down.

Using inheritance requires us to duplicate the internal structure of the tempfile to a subclass. If you look at the code before my change, that's exactly what it was doing. The main problem with this is that Ruby objects can keep data hidden away (an internal state), not accessible to the Ruby runtime. Think of it as an instance variable that you can't reach from Ruby. For the Tempfile object, MRI, and JRuby both do this.

That means that any attempts at cloning to a subclass would not actually be a clone. The previous code happened to work in most cases on MRI, but in fact, wouldn't work at all on JRuby. This is why I decided to move to composition and delegation. It lets us add functionality without breaking method cache, and will work cross VM.

I will add delegate methods that you've mentioned. I'm sorry I broke your production app, but I really believe this change was necessary. :-(

--------------------------------------------------------------------------------

Peter Marklund said over 3 years ago:

Aaron,
thanks for being so responsive about this and for the deep technical explanation, I really appreciate it. Your change makes a lot of sense, I guess it just shouldn't have been done on a stable branch. In our case, no users were affected as we discovered this issue early internally.

--------------------------------------------------------------------------------

Trevor Turk said over 3 years ago:

FYI - this is fixed in this commit: https://github.com/rails/rails/commit/c52e2cf4b3ef22fea6989df906a53188e632b9a4

--------------------------------------------------------------------------------