Defect #13949

Handling of attachment uploads when 'Maximum attachment size' is set to 0

Added by Hongjoo Lee over 4 years ago. Updated over 4 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:Attachments
Target version:2.4.0
Resolution:Fixed Affected version:2.3.0

Description

After setting maximum attachment size to 0,

with expecting unlimited size of attachment, (expected behavior)

updating issue with attachment seems to upload file, but actually it doesn't make a link to the file.

Here's steps to reproduce.

  1. Administration > Settings > General > Maximum Attachment Size : set to 0
  2. Issue > Update (or New Issue) > choose files
  3. choose any file and click 'choose'
  4. progress bar shows for awhile (seems file is uploaded)
  5. after all, "not found" message comes out other than description input field.

shows progress file

not found message

redmine1.png - shows progress file (23.5 KB) Hongjoo Lee, 2013-05-05 10:42

redmine2.png - "not found" message (28.5 KB) Hongjoo Lee, 2013-05-05 10:42


Related issues

Related to Redmine - Feature #14022: provide a way to set unlimited attachment size New

Associated revisions

Revision 11841
Added by Jean-Philippe Lang over 4 years ago

Fixed that JS warning is not displayed when attachment maximum size is 0 (#13949).

Revision 11842
Added by Jean-Philippe Lang over 4 years ago

Display error messages when attachment validation fails (#13949).

History

#1 Updated by Mischa The Evil over 4 years ago

Hongjoo Lee wrote:

After setting maximum attachment size to 0, with expecting unlimited size of attachment (expected behavior), updating issue with attachment seems to upload file, but actually it doesn't make a link to the file.

This is the expected behavior. When setting "Maximum attachment size" is set to 0 (which is actually 0 KB) you deny any bits and bytes to be uploaded.

Here's steps to reproduce.

[...]
4. progress bar shows for awhile (seems file is uploaded)
[...]

The files are not only not-linked but also not uploaded in this case. The progress bar shows only while it is requesting permission to upload (store) the file. When this is denied, as in this case due to setting "Maximum attachment size" to 0, file upload attempt is canceled and Not Found message is rendered.

#2 Updated by Hongjoo Lee over 4 years ago

Mischa The Evil wrote:

Hongjoo Lee wrote:

After setting maximum attachment size to 0, with expecting unlimited size of attachment (expected behavior), updating issue with attachment seems to upload file, but actually it doesn't make a link to the file.

This is the expected behavior. When setting "Maximum attachment size" is set to 0 (which is actually 0 KB) you deny any bits and bytes to be uploaded.

No problem with how expected behavior defined in redmine. However, set to 0 means unlimited in general and there are many cases in the same sense. In this point of view, my expectation was unlimited size of attachment.

Here's steps to reproduce.

[...]
4. progress bar shows for awhile (seems file is uploaded)
[...]

The files are not only not-linked but also not uploaded in this case. The progress bar shows only while it is requesting permission to upload (store) the file. When this is denied, as in this case due to setting "Maximum attachment size" to 0, file upload attempt is canceled and Not Found message is rendered.

presenting clip icon and file name looks as if the file uploaded. Also "NOT FOUND" message after showing process bar causes some misapprehension.

In my opinion, a message showing size limit exceeded should come out in this case and keep consistency. It would be better than unusual behavior.

#3 Updated by Mischa The Evil over 4 years ago

Hongjoo Lee wrote:

presenting clip icon and file name looks as if the file uploaded. Also "NOT FOUND" message after showing process bar causes some misapprehension.

In my opinion, a message showing size limit exceeded should come out in this case and keep consistency. It would be better than unusual behavior.

You are right. With setting "Maximum attachment size" set to 0 the regular notice of exceeding the max size isn't displayed and file upload is triggered with the following error stack as a result:

Started POST "/uploads.js?attachment_id=1&filename=CPtoRM-List.txt" for xxx.xxx.xxx.xxx at 2013-05-06 05:05:46 +0200
Processing by AttachmentsController#upload as JS
  Parameters: {"attachment_id"=>"1", "filename"=>"CPtoRM-List.txt"}
  Current user: Mischa (id=1)
  Rendered attachments/upload.js.erb (3.9ms)
Completed 500 Internal Server Error in 65ms

ActionController::RoutingError (No route matches {:action=>"show", :controller=>"attachments", :attachment_id=>"1", :format=>"js", :id=>#<Attachment id: nil, container_id: nil, container_type: nil, filename: "CPtoRM-List.txt", disk_filename: "", filesize: 2578, content_type: "", digest: "", downloads: 0, author_id: 1, created_on: nil, description: nil, disk_directory: nil>}):
  app/views/attachments/upload.js.erb:7:in `_app_views_attachments_upload_js_erb__187147715116926503_58328920'
  app/controllers/attachments_controller.rb:90:in `upload'

So, I think your issue can be actually split into two parts:
  1. Fix handling of attachment uploads when "Maximum attachment size" is set to 0, and/or
  2. Provide a way to set "Maximum attachment size" to unlimited

Regarding 1: I've tried to find the cause of this but haven't succeeded. It seems the filesize-validation should cover the 0 value correctly looking at source:/trunk/app/models/attachment.rb#L66 unless the .to_i.kilobytes is not handled correctly with a 0 value1:

  def validate_max_file_size
    if @temp_file && self.filesize > Setting.attachment_max_size.to_i.kilobytes
      errors.add(:base, l(:error_attachment_too_big, :max_size => Setting.attachment_max_size.to_i.kilobytes))
    end
  end

Regarding 2: I don't know if it's worth the effort that goes into implementing such an unlimited option while 1) it can be easily configured - virtually - using a high value for "Maximum attachment size" and 2) it is generally better to set some limit to file uploads instead of letting it unlimited which could potentially do harm to systems/networks.

1 though, these methods are provided by the Rails framework instead of by Redmine

#4 Updated by Hongjoo Lee over 4 years ago

I totally agree with you at the point of splitting this issue into two parts.

And I just tried to do it, but seems configuration have change since the last time I made an my old report and cannot produce any sub-issues or related-issues anymore.

Anyway, "fix handling of attachment uploads when "Maximum attachment size" is set to 0" as bug, and "provide a way to set unlimited attachment size" as a new feature will be made.

#5 Updated by Hongjoo Lee over 4 years ago

Just created a new issue #14022 for new feature,
I cannot modify the title of this issue but let's deal this is as a matter of "fix handling of attachment uploads when 'Maximum attachment size' is set to 0".

#6 Updated by Jean-Philippe Lang over 4 years ago

  • Subject changed from 0 max attachment size causes unexpected behavior to Handling of attachment uploads when 'Maximum attachment size' is set to 0
  • Status changed from New to Resolved
  • Assignee set to Jean-Philippe Lang
  • Target version set to 2.4.0
  • Resolution set to Fixed

This is now fixed by r11841 and r11842.

#7 Updated by Jean-Philippe Lang over 4 years ago

BTW, the upload field should not even be displayed if the maximum size is set to 0, what do you think?

#8 Updated by Jean-Baptiste Barth over 4 years ago

Jean-Philippe: it depends on #14022, which basically says "0 should mean unlimited size". In this case we shouldn't hide the field. For now 0 equals 0, so we should probably hide the field.

#9 Updated by Jean-Philippe Lang over 4 years ago

Jean-Baptiste Barth wrote:

Jean-Philippe: it depends on #14022, which basically says "0 should mean unlimited size". In this case we shouldn't hide the field. For now 0 equals 0, so we should probably hide the field.

I'd like to keep the ability to disable uploads by setting the max size to 0. -1 would be an option for unlimited size.

#10 Updated by Mischa The Evil over 4 years ago

Jean-Philippe Lang wrote:

This is now fixed by r11841 and r11842.

Confirmed. Thanks.

BTW, the upload field should not even be displayed if the maximum size is set to 0, what do you think?

I agree.

I'd like to keep the ability to disable uploads by setting the max size to 0. -1 would be an option for unlimited size.

I think that that is indeed the best way to go when an the unlimited size option is getting implemented (as per #14022).

#11 Updated by Etienne Massip over 4 years ago

Jean-Philippe Lang wrote:

BTW, the upload field should not even be displayed if the maximum size is set to 0, what do you think?

I think it is common use in software configuration to set 0 to deactivate a limit, as Hongjoo actually thought it was, -1 is more often used behind the scenes by programmers.

Also, I don't think there exists an use case for totally disabling upload and if there was, this should be done using permissions. IMHO, it's not worth it dealing with extra code for this purpose.

#12 Updated by Hongjoo Lee over 4 years ago

Jean-Baptiste Barth wrote:

Jean-Philippe: it depends on #14022, which basically says "0 should mean unlimited size". In this case we shouldn't hide the field. For now 0 equals 0, so we should probably hide the field.

As I mentioned (thanks for Etienne's agreement), 0 is more generally used for meaning unlimited and -1 means sort of exceptional case having broad meaning comparing to 0. Actually, both cases are not often shown on UI setting but appear in text configuration such as INI or XML files. However, "0 means unlimited" sometimes have seen in UI configuration, having size input form with short guide, "set to 0 for unlimited".

We can take it in this way. The feature, "file attachment" may firstly divided into two cases, activated and deactivated. Then, activated branch can have sub-branches of limited and unlimited, and may have authorization/permission branch in future for the case of deactivating file attachment. And now we are discussing about activated-unlimited-fileattachment case and I claimed that the feature #14022 is suitable for that case. Hence, I think we don't need to deal with deactivating file attachment case here and that should be dealt separately. Of course deactivation can be easily represented by setting the field to -1 or 0 and it make us hesitate to choose better way, but sooner or later it must be conflicted to other sub cases such as permission.

In summary, let's talk about deactivation separately when it is needed and issued. And let's move on to unlimited file attachment with #14022.

#13 Updated by Jean-Philippe Lang over 4 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF