carrierwaveuploader / carrierwave

Classier solution for file uploads for Rails, Sinatra and other Ruby web frameworks
https://github.com/carrierwaveuploader/carrierwave
8.78k stars 1.66k forks source link

Version condition re-evaluated on call to asset_url(:thumb) #1315

Closed willkoehler closed 10 years ago

willkoehler commented 10 years ago

I'm using conditional version processing, similar to https://github.com/carrierwaveuploader/carrierwave/wiki/How-to%3A-Do-conditional-processing

When I call asset_url(:thumb), the version condition is re-evaluated. This can be slow because it requires pulling the asset down from asset storage in some cases.

This changed between v0.7.1 and v0.9.0. In v0.7.1, asset_url(:thumb) did not cause the version condition to be re-evaluated.

willkoehler commented 10 years ago

It looks like this is the commit where that behavior changed https://github.com/carrierwaveuploader/carrierwave/commit/2582999e83cde4b0ba3fdf4b132b1c8410502eb1

…and now I see a note at the bottom of https://github.com/carrierwaveuploader/carrierwave/wiki/How-to%3A-Do-conditional-processing suggesting a method to avoid re-evaluating the version - at least I think that what the note about "multiple checking" is solving. Also #411 appears to be related.

From my perspective, this change creates an unexpected behavior, requires additional complexity in my uploader, and I'm not sure what problem it solves. Can someone enlighten me?

willkoehler commented 10 years ago

To give more perspective here's my work-around. Hopefully it will help other people dealing with this issue.

I was fortunate enough to already have the information I needed in my model to evaluate the version condition. However I feel it's odd to evaluate the condition two different ways: one way when the the asset is first uploaded, another way when the asset exists.

class DocumentUploader < CarrierWave::Uploader::Base
  include CarrierWave::RMagick
  # Store on Amazon S3 using Fog
  storage :fog

. . .

  # Create thumbnail for images and PDFs
  version :thumb, :if => :thumbable? do
    process :resize_to_fit => [240, 240], :if => :image?
    process :pdf_preview => [240, 240], :if => :pdf?

    . . .

  end

. . .

  private

    def thumbable?(file)
      image?(file) || pdf?(file)
    end

    def image?(file)
      # Check the model first (see https://github.com/carrierwaveuploader/carrierwave/issues/1315)
      return model.is_image? if model.content_type.present?
      file.content_type.include? 'image'
    end

    def pdf?(file)
      # Check the model first (see https://github.com/carrierwaveuploader/carrierwave/issues/1315)
      return model.is_pdf? if model.content_type.present?
      file.content_type == 'application/pdf'
    end
end

See the full uploader here: https://gist.github.com/willkoehler/8936717

bensie commented 10 years ago

@willkoehler I've reverted 2582999e83cde4b0ba3fdf4b132b1c8410502eb1 in master. I agree there's no reason to check for file existence when generating a URL.

willkoehler commented 10 years ago

Awesome. Thank you.