The default :url option is very dangerous. #727

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
Contributor

tilsammans commented Feb 2, 2012

I am unsure if I am missing anything?

Without my proposed change, it will quite possibly destroy uploads already present on the system.
Simply adding the :class in there prevents this. Also I moved the :id to the front sice this is the logical hierarchy.

@tilsammans tilsammans The default :url option is very dangerous. I am unsure if I am missin…
…g anything?

Without my proposed change, it will quite possibly destroy uploads already present on the system.
Simply adding the :class in there prevents this. Also I moved the :id to the front sice this is the logical hierarchy.
4c73642
Member

jyurek commented Feb 3, 2012

Well, the ID should prevent problems, unless you have different models with the same attachment name. The biggest problem is that people currently rely on the default, and changing it would mean people would have to make changes to their code and/or environment. I'm unsure of the best way to change the default.

Contributor

Sporky023 commented Feb 3, 2012

This change makes sense to me, but it breaks backwards compatibility. Therefore it'd be good to include in a major version release.

edit: I should be more specific. There is a potential for collision problems and hence overwriting, and this is solved by using the unique tuple of :class/:attachment/:id. But this does break compatibility in an unacceptable way if it just gets merged in.

How to evolve the interface while managing compatibility is definitely an interesting question. mike-burns and I were talking about this a few days ago and he put forth the idea of an options object which has sole responsibility for getting all the options in place, knowing about defaults, and providing deprecation warnings whenever we're evolving the interface.

GBH commented Feb 8, 2012

OK, this is a horrible default that I just got burned by.

I have a few models that have a generic has_attached_file :file. So you can guess what happens when models with the same id get files uploaded to.

Adding :class will solve this problem. Yes it will break old installs. So bump a major or minor version and let people know. Keeping this broken default setting going forward is not a good idea.

nbrew commented Feb 8, 2012

I'd actually opt for the default to be :class/:attachment/:id rather than :class/:id/:attachment but either way I think the change would be good.

Contributor

tilsammans commented Feb 11, 2012

Agreed that merging this into a point release is unacceptable, but for the next major version I think it's a worthwhile thing to do. I just don't know if a new major version is planned? Having just this change in a major version seems ... overkill. Not sure if there are other big architectural changes being planned currently.

Hey guys just wanted to trow some ideas from projects that accumulate large amount of files and bring your attention to a sort of a scalability problem.

There is a point in which those suggestions and defaults above make much more seek on the hard drives and pulling files and backing up data becomes nightmare. We are talking about numbers around 10 million files which isn't that much for a project that will run couple of years on the web.

We came up with this on couple of our projects to preserve the logic and at the same time minimize the seeks. There is also an Nginx rewrite which serves for better indexing of the images in production.

Just to trow you some numbers. Backing up data with this structure went for about 30 minutes. After updating the app and reprocessing with the default the backup time changed to 10+ hours.

This is the code that was done 2 years ago and didn't introduced any problems though it is messy for my taste.

Paperclip.interpolates(:dir) do |attachment, style|
  dir = 0
  unless attachment.instance.id.blank?
    dir = (attachment.instance.id/10000).to_i
  end
  return dir
end
has_attached_file :image,
                    :styles          => { :wider  => ["240x160#", :png],
                                          :wide   => ["180x120#", :png],
                                          :large  => ["145x145#", :png],
                                          :normal => [ "95x95#",  :png],
                                          :small  => [ "45x45#",  :png] },
                    #:convert_options => { :wide   => "-depth 8 -colors 512 -type Optimize -quality 70 -channel RGB -alpha off -interlace none -strip +dither +compress +antialias",
                    #                      :large  => "-depth 8 -colors 512 -type Optimize -quality 70 -channel RGB -alpha off -interlace none -strip +dither +compress +antialias",
                    #                      :normal => "-depth 8 -colors 256 -type Optimize -quality 80 -channel RGB -alpha off -interlace none -strip +dither +compress +antialias",
                    #                      :small  => "-depth 4 -colors 128                -quality 90 -channel RGB -alpha off -interlace none -strip +dither +compress +antialias" },
                    :default_style   =>   :normal,
                    :default_url     =>   "/images/v2/story_picture/default-:style.png",
                    :url             =>   "/img_story_custom2/:dir/:url_id-:style.:extension",
                    :path            =>   ":rails_root/public/img_story_custom2/:dir/:url_id-:style.:extension"

This is what we ended up adopting which allows for the backup to be made in about 1 hour.
Also hard drives were much more happy with it.

  Paperclip.interpolates :title_url do |attachment, style|
    attachment.instance.title_url
  end
  Paperclip.interpolates :style_upcase do |attachment, style|
    style_upcase = style.to_s.upcase
    return style_upcase
  end
  Paperclip.interpolates :directory_id do |attachment, style|
    id = (attachment.instance.id).to_s.rjust(8,"0")
    directory_id = id.scan(/..../)
    directory_id = directory_id.join('/')
    return directory_id
  end
  PAPERCLIP_PATH = ":rails_root/public/attachments/:class-:attachment/:directory_id/:style_upcase-:title_url.:extension"
  if Rails.env == "production"
    PAPERCLIP_URL =                  "/attachments/:class-:attachment/:directory_id/:style/:title_url.:extension"
  else
    PAPERCLIP_URL =                  "/attachments/:class-:attachment/:directory_id/:style_upcase-:title_url.:extension"
  end
  has_attached_file :photo,
                    :styles => {
                      :small        => { :format => 'jpg', :quality => 100, :geometry => '50x50#',   :processors => [:cropper] },
                      :thumb_small  => { :format => 'jpg', :quality => 100, :geometry => '110x110#', :processors => [:cropper] },
                      :thumb_medium => { :format => 'jpg', :quality => 100, :geometry => '150x150#', :processors => [:cropper] },
                      :medium       => { :format => 'jpg', :quality => 100, :geometry => '240x160^', :processors => [:cropper] },
                      :featured     => { :format => 'jpg', :quality => 100, :geometry => '312x234^', :processors => [:cropper] },
                      :banner       => { :format => 'jpg', :quality => 100, :geometry => '300x250^', :processors => [:cropper] },
                      :homepage     => { :format => 'jpg', :quality => 100, :geometry => '660x440^', :processors => [:cropper] },
                      :large        => { :format => 'jpg', :quality => 100, :geometry => '660x550' },
                      :fullscreen   => { :format => 'jpg', :quality => 100, :geometry => '960x550' },
                      :original     => { :format => 'jpg', :quality => 100, :geometry => '' }
                    },
                    :convert_options => {
                      :small        => "-gravity Center -extent 50x50  ",
                      :thumb_small  => "-gravity Center -extent 110x110",
                      :thumb_medium => "-gravity Center -extent 150x150",
                      :medium       => "-gravity Center -extent 240x160",
                      :featured     => "-gravity Center -extent 312x234",
                      :banner       => "-gravity Center -extent 300x250",
                      :homepage     => "-gravity Center -extent 660x440"
                    },
                    :path => PAPERCLIP_PATH,
                    :url  => PAPERCLIP_URL

Hope this helps!

Contributor

tilsammans commented Feb 13, 2012

@YavorIvanov i tend to use :id_partition for every Paperclip attachment, which solves that problem I think. Also something to consider switching to, should Paperclip ship with new defaults.

mike-burns closed this in 26f4d40 Mar 9, 2012

Contributor

asanghi commented Mar 10, 2012

Just to collect some historical reference here. I'm referencing ticket #588 here. The same problem was raised 6 months ago and we added a warning then. The warning may still be worthwhile but we can look at removing it since the only way a clash will happen now is if the user will explicitly put clashing storage locations for their attachments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment