Bug 490930: Always store attachments locally if they are over X size (and below some threshold!), don't ever display "Big File" checkbox

r=mkanat a=LpSolit

9 files changed, 57 insertions, 141 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index ddce1f593..39afe5df1 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -59,6 +59,9 @@ use Bugzilla::Util;
 use Bugzilla::Field;
 use Bugzilla::Hook;
 
+use File::Copy;
+use List::Util qw(max);
+
 use base qw(Bugzilla::Object);
 
 ###############################
@@ -108,7 +111,6 @@ use constant VALIDATORS => {
     isprivate     => \&_check_is_private,
     isurl         => \&_check_is_url,
     mimetype      => \&_check_content_type,
-    store_in_file => \&_check_store_in_file,
 };
 
 use constant UPDATE_VALIDATORS => {
@@ -538,52 +540,29 @@ sub _check_content_type {
 sub _check_data {
     my ($invocant, $params) = @_;
 
-    my $data;
+    my $data = $params->{data};
     if ($params->{isurl}) {
-        $data = $params->{data};
         ($data && $data =~ m#^(http|https|ftp)://\S+#)
           || ThrowUserError('attachment_illegal_url', { url => $data });
 
         $params->{mimetype} = 'text/plain';
         $params->{ispatch} = 0;
-        $params->{store_in_file} = 0;
+        $params->{filesize} = length($data);
     }
     else {
-        if ($params->{store_in_file} || !ref $params->{data}) {
-            # If it's a filehandle, just store it, not the content of the file
-            # itself as the file may be quite large. If it's not a filehandle,
-            # it already contains the content of the file.
-            $data = $params->{data};
-        }
-        else {
-            # The file will be stored in the DB. We need the content of the file.
-            local $/;
-            my $fh = $params->{data};
-            $data = <$fh>;
-        }
+        $params->{filesize} = ref $data ? -s $data : length($data);
     }
     Bugzilla::Hook::process('attachment_process_data', { data       => \$data,
                                                          attributes => $params });
 
-    # Do not validate the size if we have a filehandle. It will be checked later.
-    return $data if ref $data;
-
-    $data || ThrowUserError('zero_length_file');
+    $params->{filesize} || ThrowUserError('zero_length_file');
     # Make sure the attachment does not exceed the maximum permitted size.
-    my $len = length($data);
-    my $max_size = $params->{store_in_file} ? Bugzilla->params->{'maxlocalattachment'} * 1048576
-                                            : Bugzilla->params->{'maxattachmentsize'} * 1024;
-    if ($len > $max_size) {
-        my $vars = { filesize => sprintf("%.0f", $len/1024) };
-        if ($params->{ispatch}) {
-            ThrowUserError('patch_too_large', $vars);
-        }
-        elsif ($params->{store_in_file}) {
-            ThrowUserError('local_file_too_large');
-        }
-        else {
-            ThrowUserError('file_too_large', $vars);
-        }
+    my $max_size = max(Bugzilla->params->{'maxlocalattachment'} * 1048576,
+                       Bugzilla->params->{'maxattachmentsize'} * 1024);
+
+    if ($params->{filesize} > $max_size) {
+        my $vars = { filesize => sprintf("%.0f", $params->{filesize}/1024) };
+        ThrowUserError('file_too_large', $vars);
     }
     return $data;
 }
@@ -642,15 +621,6 @@ sub _check_is_url {
     return $is_url ? 1 : 0;
 }
 
-sub _check_store_in_file {
-    my ($invocant, $store_in_file) = @_;
-
-    if ($store_in_file && !Bugzilla->params->{'maxlocalattachment'}) {
-        ThrowCodeError('attachment_local_storage_disabled');
-    }
-    return $store_in_file ? 1 : 0;
-}
-
 =pod
 
 =head2 Class Methods
@@ -815,9 +785,6 @@ Params:     takes a hashref with the following keys:
             the attachment is private.
             C<isurl> - boolean (optional, default false) - true if the
             attachment is a URL pointing to some external ressource.
-            C<store_in_file> - boolean (optional, default false) - true
-            if the attachment must be stored in data/attachments/ instead
-            of in the DB.
 
 Returns:    The new attachment object.
 
@@ -833,53 +800,44 @@ sub create {
     # Extract everything which is not a valid column name.
     my $bug = delete $params->{bug};
     $params->{bug_id} = $bug->id;
-    my $fh = delete $params->{data};
-    my $store_in_file = delete $params->{store_in_file};
+    my $data = delete $params->{data};
+    my $size = delete $params->{filesize};
 
     my $attachment = $class->insert_create_data($params);
     my $attachid = $attachment->id;
 
-    # We only use $data here in this INSERT with a placeholder,
-    # so it's safe.
-    my $sth = $dbh->prepare("INSERT INTO attach_data
-                             (id, thedata) VALUES ($attachid, ?)");
-
-    my $data = $store_in_file ? "" : $fh;
-    trick_taint($data);
-    $sth->bind_param(1, $data, $dbh->BLOB_TYPE);
-    $sth->execute();
-
-    # If the file is to be stored locally, stream the file from the web server
-    # to the local file without reading it into a local variable.
-    if ($store_in_file) {
+    # The file is too large to be stored in the DB, so we store it locally.
+    if ($size > Bugzilla->params->{'maxattachmentsize'} * 1024) {
         my $attachdir = bz_locations()->{'attachdir'};
         my $hash = ($attachid % 100) + 100;
         $hash =~ s/.*(\d\d)$/group.$1/;
         mkdir "$attachdir/$hash", 0770;
         chmod 0770, "$attachdir/$hash";
-        open(AH, '>', "$attachdir/$hash/attachment.$attachid");
-        binmode AH;
-        if (ref $fh) {
-            my $limit = Bugzilla->params->{"maxlocalattachment"} * 1048576;
-            my $sizecount = 0;
-            while (<$fh>) {
-                print AH $_;
-                $sizecount += length($_);
-                if ($sizecount > $limit) {
-                    close AH;
-                    close $fh;
-                    unlink "$attachdir/$hash/attachment.$attachid";
-                    ThrowUserError("local_file_too_large");
-                }
-            }
-            close $fh;
+        if (ref $data) {
+            copy($data, "$attachdir/$hash/attachment.$attachid");
+            close $data;
         }
         else {
-            print AH $fh;
+            open(AH, '>', "$attachdir/$hash/attachment.$attachid");
+            binmode AH;
+            print AH $data;
+            close AH;
         }
-        close AH;
+        $data = ''; # Will be stored in the DB.
+    }
+    # If we have a filehandle, we need its content to store it in the DB.
+    elsif (ref $data) {
+        local $/;
+        $data = <$data>;
     }
 
+    my $sth = $dbh->prepare("INSERT INTO attach_data
+                             (id, thedata) VALUES ($attachid, ?)");
+
+    trick_taint($data);
+    $sth->bind_param(1, $data, $dbh->BLOB_TYPE);
+    $sth->execute();
+
     # Return the new attachment object.
     return $attachment;
 }
diff --git a/attachment.cgi b/attachment.cgi
index cdfcc6bf7..ca82bc463 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -485,7 +485,6 @@ sub insert {
          isprivate     => scalar $cgi->param('isprivate'),
          isurl         => scalar $cgi->param('attachurl'),
          mimetype      => $content_type,
-         store_in_file => scalar $cgi->param('bigfile'),
          });
 
     foreach my $obsolete_attachment (@obsolete_attachments) {
diff --git a/docs/en/xml/using.xml b/docs/en/xml/using.xml
index 101a9d131..daa0720fa 100644
--- a/docs/en/xml/using.xml
+++ b/docs/en/xml/using.xml
@@ -332,9 +332,7 @@
         <para>
         <emphasis>Attachments:</emphasis>
           You can attach files (e.g. testcases or patches) to bugs. If there
-          are any attachments, they are listed in this section.  Attachments are
-          normally stored in the Bugzilla database, unless they are marked as
-          Big Files, which are stored directly on disk.
+          are any attachments, they are listed in this section.
         </para>
       </listitem>
 
@@ -862,20 +860,6 @@
     </para>
 
     <para>
-      If you have a really large attachment, something that does not need to
-      be recorded forever (as most attachments are), or something that is too
-      big for your database, you can mark your attachment as a
-      <quote>Big File</quote>, assuming the administrator of the installation
-      has enabled this feature.  Big Files are stored directly on disk instead
-      of in the database.  The maximum size of a <quote>Big File</quote> is
-      normally larger than the maximum size of a regular attachment. Independently
-      of the storage system used, an administrator can delete these attachments
-      at any time. Nevertheless, if these files are stored in the database, the
-      <quote>allow_attachment_deletion</quote> parameter (which is turned off
-      by default) must be enabled in order to delete them.
-    </para>
-
-    <para>
       Also, if the administrator turned on the <quote>allow_attach_url</quote>
       parameter, you can enter the URL pointing to the attachment instead of
       uploading the attachment itself. For example, this is useful if you want to
diff --git a/js/attachment.js b/js/attachment.js
index d759248cd..314e4acb1 100644
--- a/js/attachment.js
+++ b/js/attachment.js
@@ -56,8 +56,7 @@ function setContentTypeDisabledState(form)
 function URLFieldHandler() {
     var field_attachurl = document.getElementById("attachurl");
     var greyfields = new Array("data", "ispatch", "autodetect",
-                               "list", "manual", "bigfile",
-                               "contenttypeselection",
+                               "list", "manual", "contenttypeselection",
                                "contenttypeentry");
     var i, thisfield;
     if (field_attachurl.value.match(/^\s*$/)) {
@@ -103,8 +102,6 @@ function clearAttachmentFields() {
 
     document.getElementById('data').value = '';
     DataFieldHandler();
-    if ((element = document.getElementById('bigfile')))
-        element.checked = '';
     if ((element = document.getElementById('attachurl'))) {
         element.value = '';
         URLFieldHandler();
diff --git a/post_bug.cgi b/post_bug.cgi
index c097a96ce..956856b12 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -205,7 +205,6 @@ if (defined($cgi->upload('data')) || $cgi->param('attachurl')) {
              isprivate     => scalar $cgi->param('isprivate'),
              isurl         => scalar $cgi->param('attachurl'),
              mimetype      => $content_type,
-             store_in_file => scalar $cgi->param('bigfile'),
             });
     };
     Bugzilla->error_mode($error_mode_cache);
diff --git a/template/en/default/admin/params/attachment.html.tmpl b/template/en/default/admin/params/attachment.html.tmpl
index 12fd491fc..159588625 100644
--- a/template/en/default/admin/params/attachment.html.tmpl
+++ b/template/en/default/admin/params/attachment.html.tmpl
@@ -64,13 +64,16 @@
                       "specify a URL when creating an attachment and " _
                       "treat the URL itself as if it were an attachment.",
 
-  maxattachmentsize => "The maximum size (in kilobytes) of attachments. " _
-                       "$terms.Bugzilla will not accept attachments greater than this number " _
-                       "of kilobytes in size. Setting this parameter to 0 will prevent " _
-                       "attaching files to ${terms.bugs}.",
+  maxattachmentsize => "The maximum size (in kilobytes) of attachments to be stored " _
+                       "in the database. If a file larger than this size is attached " _
+                       "to ${terms.abug}, $terms.Bugzilla will look at the " _
+                       "<a href='#maxlocalattachment'><tt>maxlocalattachment</tt> parameter</a> " _
+                       "to determine if the file can be stored locally on the web server. " _
+                       "If the file size exceeds both limits, then the attachment is rejected. " _
+                       "Settings both parameters to 0 will prevent attaching files to ${terms.bugs}.",
 
-  maxlocalattachment => "The maximum size (in megabytes) of attachments identified by " _
-                        "the user as 'Big Files' to be stored locally on the webserver. " _
-                        "If set to zero, attachments will never be kept on the local " _
-                        "filesystem." }
+  maxlocalattachment => "The maximum size (in megabytes) of attachments to be stored " _
+                        "locally on the web server. If set to a value lower than the " _
+                        "<a href='#maxattachmentsize'><tt>maxattachmentsize</tt> parameter</a>, " _
+                        "attachments will never be kept on the local filesystem." }
 %]
diff --git a/template/en/default/attachment/createformcontents.html.tmpl b/template/en/default/attachment/createformcontents.html.tmpl
index 2cef632d1..ad3a25bc6 100644
--- a/template/en/default/attachment/createformcontents.html.tmpl
+++ b/template/en/default/attachment/createformcontents.html.tmpl
@@ -32,18 +32,6 @@
     >
   </td>
 </tr>
-[% IF Param("maxlocalattachment") %]
-<tr class="expert_fields">
-  <th>BigFile:</th>
-  <td>
-    <input type="checkbox" id="bigfile"
-           name="bigfile" value="bigfile">
-    <label for="bigfile">
-      Big File - Stored locally and may be purged
-    </label>
-  </td>
-</tr>
-[% END %]
 [% IF Param("allow_attach_url") %]
 <tr class="expert_fields">
   <th><label for="attachurl">AttachURL</label>:</th>
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index 177d47621..43644f703 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -37,11 +37,7 @@
 [% DEFAULT title = "Internal Error" %]
 
 [% error_message = BLOCK %]
-  [% IF error == "attachment_local_storage_disabled" %]
-    [% title = "Local Storage Disabled" %]
-    You cannot store attachments locally. This feature is disabled.
-
-  [% ELSIF error == "attachment_url_disabled" %]
+  [% IF error == "attachment_url_disabled" %]
     [% title = "Attachment URL Disabled" %]
     You cannot attach a URL. This feature is currently disabled.
 
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index b57792fd2..490941807 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -574,9 +574,13 @@
 
   [% ELSIF error == "file_too_large" %]
     [% title = "File Too Large" %]
+    [%# Convert maxlocalattachment from Mb to Kb %]
+    [% max_local = Param('maxlocalattachment') * 1024 %]
+    [% max_limit = [Param('maxattachmentsize'), max_local] %]
     The file you are trying to attach is [% filesize FILTER html %] 
-    kilobytes (KB) in size. Non-patch attachments cannot be more than
-    [%+ Param('maxattachmentsize') %] KB. <br>
+    kilobytes (KB) in size. Attachments cannot be more than
+    [%# Hack to get the max value between both limits %]
+    [%+ max_limit.nsort.last FILTER html %] KB. <br>
     We recommend that you store your attachment elsewhere
     [% IF Param("allow_attach_url") %]
       and then specify the URL to this file on the attachment
@@ -1023,11 +1027,6 @@
     [% title = "Invalid Keyword Name" %]
     You may not use commas or whitespace in a keyword name.
      
-  [% ELSIF error == "local_file_too_large" %]
-    [% title = "Local File Too Large" %]
-    Local file uploads must not exceed 
-    [% Param('maxlocalattachment') %] MB in size.
-
   [% ELSIF error == "login_needed_for_password_change" %]
     [% title = "Login Name Required" %]
     You must enter a login name when requesting to change your password.
@@ -1312,13 +1311,6 @@
     The password must be at least
     [%+ constants.USER_PASSWORD_MIN_LENGTH FILTER html %] characters long.
 
-  [% ELSIF error == "patch_too_large" %]
-    [% title = "File Too Large" %]
-    The file you are trying to attach is [% filesize FILTER html %] 
-    kilobytes (KB) in size.
-    Patches cannot be more than [% Param('maxattachmentsize') %] KB in size.
-    Try splitting your patch into several pieces.
-
   [% ELSIF error == "product_access_denied" %]
     Either the product 
     [%+ IF id.defined %]