Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Document problems with not using mount_on option in the wiki #249

Closed
clyfe opened this Issue · 18 comments

4 participants

@clyfe
class Document < ActiveRecord::Base
  mount_uploader :file, FileUploader
end

# obtain a reference to a document without loading from db
doc = some_parent.instance_variable_get("@document")

doc.is_a? Array

For some reason the "is_a?" call passes and executes
a chain of "method_missing" before it reaches to Object ancestor
one of these "method_missings" is AR/association/association_proxy.rb +208
this proxy is responsible of loading from the db
and a few lines down the call-stack it reacher AR/base.rb +1557
and on line 1558 it assigns to the uploader the value from the db

respond_to?(:"#{k}=") ? send(:"#{k}=", v) : raise(UnknownAttributeError, "unknown attribute: #{k}")

In our case the uploader accesor is called with the DB-fetched value

# equivalent call
self.file= 'string/from.db'

that renders the error

CarrierWave::FormNotMultipart (You tried to assign a String or a Pathname to an uploader, for security reasons, this is not allowed.

  If this is a file upload, please check that your upload form is multipart encoded.):

This can easily be fixed obviously by mounting on a different key that the DB attribute.
Nevertheless, the general case is to be considered.

@trevorturk
Owner

I'm sorry, but can you please take a moment to carefully describe the what the problem you are having is? What are you trying to accomplish? What's getting in your way? I can't make sense of this as you've written it.

@clyfe

Unfortunately there is no easy way to understand the long version unless one does make some effort.

Short:
I'm using ActiveScaffold + Carrierwave and when I update an existing document by uploading a new file I get the error:

CarrierWave::FormNotMultipart (You tried to assign a String or a Pathname to an uploader, for security reasons, this is not allowed.

Long:
ActiveScaffold is a generic CRUD plugin.
ManagementProcess has many Documents and Document mount_uploader :file, FileUploader
ActiveScaffold does some juggling to allow mass assignment of associated objects in a generic way (that means no mather how they're named):

association_proxy = instance_variable_get("@#{association.name}")
# meaning in my case:
# management_process.instance_variable_get("@document")

And the function doing this calls allways neds to return an array that's either association_proxy if it's already an array or [association_proxy] if it's just a single object.

When it check's if it's already an array via association_proxy.is_a? Array the above described behavior happens wich sumarizes to the fact that the uploader is assigned with the string from database because both have the same name ie.: document.file = 'string/from.db'.

  1. association_proxy.is_a? Array
  2. method_missing is called
  3. method_missing loads and populates from DB
  4. calls self.uploader_name = "value/from/db.column"
  5. => error

As I said this can be avoided by using "mount_on" to mount on a different key that the DB column name.

I wonder if anything can be done in carrierwave to avoid this assignment (uploader = db value) to improve the project.

@trevorturk
Owner

Have you tried this with carrierwave master branch? I did something to help alleviate FormNotMultipart errors, but I have no idea if it would help in your case.

I'm really not sure about how else we might change Carrierwave to accommodate your situation. Maybe someone else will chime in, otherwise it might be smart for you to work up a pull request with a proposed change because you might be the only person that's bothered. Failing that, maybe just document your workaround in the wiki.

@clyfe

The problem stems from doing r1.attributes = r2.attributes on two records.

We could override the attributes method on our model like:

  def attributes
    attrs = {}
    attribute_names.each { |name| 
      attrs[name] = (name == :file) ? self.file : read_attribute(name) 
    }
    attrs
  end

I'm not sure if that breaks anything.

Anyway, I think it's a "best practice" to mount on a different name, there seems to be quite a few cross-cuts if one mounts on the same key name.

@trevorturk
Owner

Umm... so I'm not sure what you're suggesting...?

@clyfe

ActiveRecord has the attributes getter and setter to work with attributes all at once, the problem is that the getter uses read_attribute[:attr] and the setter uses the AR-dynamically-generated-setters attr=.
If one overrides the setter for the file-column (the accessor that used to set-get DB values now set-gets the uploader) the AR API contract might break (like in the attributes case).

Mounting the uploader on a different key like:

mount_uploader :file, FileUploader, :mount_on => :name
# instead mount_uploader :file, FileUploader

ensures that we don't interfere with AR's usual attribute logic.

@trevorturk
Owner

I'm sorry, but I'm still not clear on what action you are suggesting we take.

@clyfe

I guess either of these 3:
1. File a bug report in AR for using for attributes accessor read_attribute[:attr] in the getter and attr= in the setter, they should both be the same I think, don't know the inner details of AR
2. Monkey-patch AR attributes accessor from carrierwave adapter when mounting
3. Raise 'mount overriding error' if trying to mount on the same key

@trevorturk
Owner

Honestly, none of those sounds lik every appealing options.

  1. I'd guess that AR is doing that on purpose, but maybe not
  2. I'd prefer not to monkey-patch AR if at all possible
  3. Is this suggesting to always use the mount_on option, or just that if you use mount_on you must use a different key?
@clyfe

regarding 3: both

  • always use mount_on
  • always mount on different key
@trevorturk
Owner

Hmm... I think this sounds like something that jnicklas probably should look at. Maybe he'll have some ideas.

@elabs

I've been thinking for a long time, that we should probably change CarrierWave to default to using mount_column_filename or something instead of just mount_column, it's a hugely breaking change though, essentially breaking 95% of applications using CarrierWave. It should be done, but I'm reluctant to do it. What do you guys think?

In that case we could raise an error when trying to mount on a column with the same name as the uploader.

@elabs

Just realized that technically it's not the filename we're storing, but rather what we've been calling "identifier" in CarrierWave. mount_column_identifier sounds a bit weird, and might trip some people up. Hmmm.

@trevorturk
Owner

I'm of the opinion that we should make the change if it's smart. CarrierWave is not yet 1.0, and asking everyone to run a migration / rename a column perhaps isn't too much to ask. If they think so, they can continue using an older version.

I'd just suggest waiting until after we release 0.5.3 with fog support. Then we can move on to an 0.6 branch and note that there are breaking changes.

So, what if it defaulted like so:

add_column :users, :avatar_uploader, :string
mount_uploader :avatar, AvatarUploader

So, the column would get _uploader added to it...?

Oh, also -- we could suggest that people use

mount_uploader :avatar, AvatarUploader, :mount_on => :avatar

...to retain the old behavior (and not raise an error) if they don't want to change their column names.

That way, we change the default, note that there was a breaking change, provide a simple fix (change your db column name), and provide an alternative (change the mount_on to use the old default) -- sounds fine to me.

@andriusch

Just thought I'd mention to anyone running into this bug: the workaround is to rename field in database and specify :mount_on option when mounting uploader.

For me this happened when using form with nested models. For example I had Avatar model accepting nested attributes for AvatarImages (AvatarImage has uploader mounted). The error happened every time when form with invalid data is submitted. I think it's strange that noone noticed this before, as what I'm doing seems pretty common to me.

@trevorturk
Owner

@sinsiliux it's not that nobody noticed, it's just that it's a major change, so we've been hesitant to make it. I think we may have to, though.

@trevorturk
Owner

I spoke with Jonas about this in depth, and we agreed that this is a shitty situation. On the one hand, we know there's a problem and want to fix it. On the other hand, we don't want to break everyone's applications by making a huge change.

So, I'd like to ask you guys to work up a new section on the wiki that explains the situation and suggests that people use the mount_on option if/when they run into issues like this.

Would one of you like to volunteer for that? Jonas and I aren't having the problem, so I don't think we're the best people to do a write up.

Thanks so much -- I'm sorry about this messy situation.

@trevorturk
Owner

I've added a quick section stub about this to the wiki. Please do consider adding more detail if possible!

@trevorturk trevorturk closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.