Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added errors on file size #1026

Merged
merged 1 commit into from
Nov 19, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/carrierwave/locale/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ en:
rmagick_processing_error: "Failed to manipulate with rmagick, maybe it is not an image? Original Error: %{e}"
mime_types_processing_error: "Failed to process file with MIME::Types, maybe not valid content-type? Original Error: %{e}"
mini_magick_processing_error: "Failed to manipulate with MiniMagick, maybe it is not an image? Original Error: %{e}"
min_size_error: "File size should be greater than %{min_size}"
max_size_error: "File size should be less than %{max_size}"
2 changes: 2 additions & 0 deletions lib/carrierwave/uploader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
require "carrierwave/uploader/remove"
require "carrierwave/uploader/extension_whitelist"
require "carrierwave/uploader/extension_blacklist"
require "carrierwave/uploader/file_size"
require "carrierwave/uploader/processing"
require "carrierwave/uploader/versions"
require "carrierwave/uploader/default_url"
Expand Down Expand Up @@ -53,6 +54,7 @@ class Base
include CarrierWave::Uploader::Remove
include CarrierWave::Uploader::ExtensionWhitelist
include CarrierWave::Uploader::ExtensionBlacklist
include CarrierWave::Uploader::FileSize
include CarrierWave::Uploader::Processing
include CarrierWave::Uploader::Versions
include CarrierWave::Uploader::DefaultUrl
Expand Down
43 changes: 43 additions & 0 deletions lib/carrierwave/uploader/file_size.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# encoding: utf-8

module CarrierWave
module Uploader
module FileSize
extend ActiveSupport::Concern

included do
before :cache, :check_size!
end

##
# Override this method in your uploader to provide a Range of Size which
# are allowed to be uploaded.
# === Returns
#
# [NilClass, Range] a size range which are permitted to be uploaded
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In kilobytes? Note units.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that here it should be in kilobytes, but it should be converted when showing the errors messages

ps: maybe i misunderstood your question

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the conversion for error messages also. The more I think about it the more I'm into this pull, btw.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@ibrahima ibrahima Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this is 10 years old but this PR is linked from the changelog, and it's not clearly documented in https://github.com/carrierwaveuploader/carrierwave/blob/master/lib/carrierwave/uploader/file_size.rb so I felt like it would be helpful to comment here.

It feels to me like if it's comparing to new_file.size isn't it comparing bytes? When I set a size_range the validation messages convert to human friendly units and it does seem like it's using bytes. I set the limit to 1024 and it allowed a file smaller than 1KB and when I tried a 4KB file it rejected it, with the error message "Validation failed: File File size should be less than 1 KB". (It does seem to go for the closest human-friendly unit, so if the limit is in MB or GB it reflects that.) So I think that the value here is in fact interpreted as bytes.

Submitted a tiny PR to update the doc comment here: #2662

ah yeah this is implicitly documented in #2199, but it would be nice to make it explicit.

#
# === Examples
#
# def size_range
# 3256....5748
# end
#
def size_range; end

private

def check_size!(new_file)
size = new_file.size
expected_size_range = size_range
if expected_size_range.is_a?(::Range)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposal is to just return if expected_size_range.nil? as early in the method as we can (before loading the size, since it's possible .size could require io), and let duck typing handle the rest. Explicitly checking that it's a Range smells funny to me.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Duck type all the things! 😉

if size < expected_size_range.min
raise CarrierWave::IntegrityError, I18n.translate(:"errors.messages.min_size_error", :min_size => expected_size_range.min)
elsif size > expected_size_range.max
raise CarrierWave::IntegrityError, I18n.translate(:"errors.messages.max_size_error", :max_size => expected_size_range.max)
end
end
end

end # FileSize
end # Uploader
end # CarrierWave
6 changes: 3 additions & 3 deletions spec/uploader/callback_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,16 @@
@uploader_class_1 = Class.new(CarrierWave::Uploader::Base)

# First Uploader only has default before-callback
@uploader_class_1._before_callbacks[:cache].should == [:check_whitelist!, :check_blacklist!]
@uploader_class_1._before_callbacks[:cache].should == [:check_whitelist!, :check_blacklist!, :check_size!]

@uploader_class_2 = Class.new(CarrierWave::Uploader::Base)
@uploader_class_2.before :cache, :before_cache_callback

# Second Uploader defined with another callback
@uploader_class_2._before_callbacks[:cache].should == [:check_whitelist!, :check_blacklist!, :before_cache_callback]
@uploader_class_2._before_callbacks[:cache].should == [:check_whitelist!, :check_blacklist!, :check_size!, :before_cache_callback]

# Make sure the first Uploader doesn't inherit the same callback
@uploader_class_1._before_callbacks[:cache].should == [:check_whitelist!, :check_blacklist!]
@uploader_class_1._before_callbacks[:cache].should == [:check_whitelist!, :check_blacklist!, :check_size!]
end


Expand Down
49 changes: 49 additions & 0 deletions spec/uploader/file_size_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# encoding: utf-8

require 'spec_helper'

describe CarrierWave::Uploader do
before do
@uploader_class = Class.new(CarrierWave::Uploader::Base)
@uploader = @uploader_class.new
end

after do
FileUtils.rm_rf(public_path)
end

describe '#cache!' do

before do
CarrierWave.stub!(:generate_cache_id).and_return('20071201-1234-345-2255')
end

it "should not raise an integrity error if there is no range specified" do
@uploader.stub!(:size_range).and_return(nil)
running {
@uploader.cache!(File.open(file_path('test.jpg')))
}.should_not raise_error(CarrierWave::IntegrityError)
end

it "should raise an integrity error if there is a size range and file has size less than minimum" do
@uploader.stub!(:size_range).and_return(2097152..4194304)
running {
@uploader.cache!(File.open(file_path('test.jpg')))
}.should raise_error(CarrierWave::IntegrityError)
end

it "should raise an integrity error if there is a size range and file has size more than maximum" do
@uploader.stub!(:size_range).and_return(0..10)
running {
@uploader.cache!(File.open(file_path('test.jpg')))
}.should raise_error(CarrierWave::IntegrityError)
end

it "should not raise an integrity error if there is a size range the file is not on it" do
@uploader.stub!(:size_range).and_return(0..50)
running {
@uploader.cache!(File.open(file_path('test.jpg')))
}.should_not raise_error(CarrierWave::IntegrityError)
end
end
end