FIX: Show gif upload size limit error straight away (#21633)

When uploading images via direct to S3 upload, we were
assuming that we could not pre-emptively check the file
size because the client may do preprocessing to reduce
the size, and UploadCreator could also further reduce the
size.

This, however, is not true of gifs, so we would have an
issue where you upload a gif > the max_image_size_kb
setting and had to wait until the upload completed for
this error to show.

Now, instead, when we direct upload gifs to S3, we check
the size straight away and present a file size error to
the user rather than making them wait. This will increase
meme efficiency by approximately 1000%.
This commit is contained in:
Martin Brennan 2023-05-18 10:36:34 +02:00 committed by GitHub
parent 20a7192b4e
commit 341f87efb7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 31 additions and 3 deletions

View File

@ -221,7 +221,7 @@ class UploadsController < ApplicationController
def validate_file_size(file_name:, file_size:) def validate_file_size(file_name:, file_size:)
raise ExternalUploadValidationError.new(I18n.t("upload.size_zero_failure")) if file_size.zero? raise ExternalUploadValidationError.new(I18n.t("upload.size_zero_failure")) if file_size.zero?
if file_size_too_big?(file_name, file_size) if attachment_too_big?(file_name, file_size)
raise ExternalUploadValidationError.new( raise ExternalUploadValidationError.new(
I18n.t( I18n.t(
"upload.attachments.too_large_humanized", "upload.attachments.too_large_humanized",
@ -232,6 +232,18 @@ class UploadsController < ApplicationController
), ),
) )
end end
if image_too_big?(file_name, file_size)
raise ExternalUploadValidationError.new(
I18n.t(
"upload.images.too_large_humanized",
max_size:
ActiveSupport::NumberHelper.number_to_human_size(
SiteSetting.max_image_size_kb.kilobytes,
),
),
)
end
end end
def force_download? def force_download?
@ -306,14 +318,20 @@ class UploadsController < ApplicationController
private private
# We can preemptively check size for attachments, but not for images # We can preemptively check size for attachments, but not for (most) images
# as they may be further reduced in size by UploadCreator (at this point # as they may be further reduced in size by UploadCreator (at this point
# they may have already been reduced in size by preprocessors) # they may have already been reduced in size by preprocessors)
def file_size_too_big?(file_name, file_size) def attachment_too_big?(file_name, file_size)
!FileHelper.is_supported_image?(file_name) && !FileHelper.is_supported_image?(file_name) &&
file_size >= SiteSetting.max_attachment_size_kb.kilobytes file_size >= SiteSetting.max_attachment_size_kb.kilobytes
end end
# Gifs are not resized on the client and not reduced in size by UploadCreator
def image_too_big?(file_name, file_size)
FileHelper.is_supported_image?(file_name) && File.extname(file_name) == ".gif" &&
file_size >= SiteSetting.max_image_size_kb.kilobytes
end
def send_file_local_upload(upload) def send_file_local_upload(upload)
opts = { opts = {
filename: upload.original_filename, filename: upload.original_filename,

View File

@ -853,6 +853,16 @@ RSpec.describe UploadsController do
) )
end end
it "returns 422 when the file is an gif and it's too big, since gifs cannot be resized on client" do
SiteSetting.max_image_size_kb = 1024
post "/uploads/create-multipart.json",
**{ params: { file_name: "test.gif", file_size: 9_999_999, upload_type: "composer" } }
expect(response.status).to eq(422)
expect(response.body).to include(
I18n.t("upload.images.too_large_humanized", max_size: "1 MB"),
)
end
it "returns a sensible error if the file size is 0 bytes" do it "returns a sensible error if the file size is 0 bytes" do
SiteSetting.authorized_extensions = "*" SiteSetting.authorized_extensions = "*"
stub_create_multipart_request stub_create_multipart_request