Bug 1180572 - create attachment_storage parameter

9 files changed, 289 insertions, 154 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 95e6b2977..07fafb11e 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -333,27 +333,7 @@ the content of the attachment
 
 sub data {
     my $self = shift;
-    return $self->{data} if exists $self->{data};
-
-    # First try to get the attachment data from the database.
-    ($self->{data}) = Bugzilla->dbh->selectrow_array("SELECT thedata
-                                                      FROM attach_data
-                                                      WHERE id = ?",
-                                                     undef,
-                                                     $self->id);
-
-    # If there's no attachment data in the database, the attachment is stored
-    # in a local file, so retrieve it from there.
-    if (length($self->{data}) == 0) {
-        if (open(AH, '<', $self->_get_local_filename())) {
-            local $/;
-            binmode AH;
-            $self->{data} = <AH>;
-            close(AH);
-        }
-    }
-
-    return $self->{data};
+    return $self->{data} //= current_storage()->retrieve($self->id);
 }
 
 =over
@@ -372,60 +352,6 @@ sub datasize {
 
 =over
 
-=item C<linecount>
-
-the number of lines of the attachment content
-
-=back
-
-=cut
-
-# linecount allows for getting the number of lines of an attachment
-# from the database directly if the data has not yet been loaded for
-# performance reasons.
-
-sub linecount {
-    my ($self) = @_;
-
-    return $self->{linecount} if exists $self->{linecount};
-
-    # Limit this to just text/* attachments as this could
-    # cause strange results for binary attachments.
-    return if $self->contenttype !~ /^text\//;
-
-    # If the data has already been loaded, we can just determine
-    # line count from the data directly. 
-    if ($self->{data}) {
-        $self->{linecount} = $self->{data} =~ tr/\n/\n/;
-    }
-    else {
-        $self->{linecount} = 
-            int(Bugzilla->dbh->selectrow_array("
-                SELECT LENGTH(attach_data.thedata)-LENGTH(REPLACE(attach_data.thedata,'\n',''))/LENGTH('\n') 
-                FROM attach_data WHERE id = ?", undef, $self->id));
-    
-    }
-
-    # If we still do not have a linecount either the attachment
-    # is stored in a local file or has been deleted. If the former,
-    # we call self->data to force a load from the filesystem and
-    # then do a split on newlines and count again.
-    unless ($self->{linecount}) {
-        $self->{linecount} = $self->data =~ tr/\n/\n/;
-    }
-
-    return $self->{linecount};
-}
-
-sub _get_local_filename {
-    my $self = shift;
-    my $hash = ($self->id % 100) + 100;
-    $hash =~ s/.*(\d\d)$/group.$1/;
-    return bz_locations()->{'attachdir'} . "/$hash/attachment." . $self->id;
-}
-
-=over
-
 =item C<flags>
 
 flags that have been set on the attachment
@@ -543,13 +469,10 @@ sub _check_data {
 
     $params->{attach_size} || ThrowUserError('zero_length_file');
     # Make sure the attachment does not exceed the maximum permitted size.
-    my $max_size = max(Bugzilla->params->{'maxlocalattachment'} * 1048576,
-                       Bugzilla->params->{'maxattachmentsize'} * 1024);
-
-    if ($params->{attach_size} > $max_size) {
-        my $vars = { filesize => sprintf("%.0f", $params->{attach_size}/1024) };
-        ThrowUserError('file_too_large', $vars);
+    if ($params->{attach_size} > Bugzilla->params->{'maxattachmentsize'} * 1024) {
+        ThrowUserError('file_too_large', { filesize => sprintf("%.0f", $params->{attach_size}/1024) });
     }
+
     return $data;
 }
 
@@ -804,46 +727,18 @@ sub create {
     my $data = delete $params->{data};
 
     my $attachment = $class->insert_create_data($params);
-    my $attachid = $attachment->id;
+    $attachment->{bug} = $bug;
 
-    # The file is too large to be stored in the DB, so we store it locally.
-    if ($params->{attach_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";
-        if (ref $data) {
-            copy($data, "$attachdir/$hash/attachment.$attachid");
-            close $data;
-        }
-        else {
-            open(AH, '>', "$attachdir/$hash/attachment.$attachid");
-            binmode AH;
-            print AH $data;
-            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) {
+    # store attachment data
+    if (ref($data)) {
         local $/;
-        # Store the content in a temp variable while we close the FH.
         my $tmp = <$data>;
-        close $data;
+        close($data);
         $data = $tmp;
     }
+    current_storage()->store($attachment->id, $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();
-
-    $attachment->{bug} = $bug;
-
-    # Return the new attachment object.
+    # Return the new attachment object
     return $attachment;
 }
 
@@ -924,10 +819,10 @@ sub remove_from_db {
         'SELECT id FROM flags WHERE attach_id = ?', undef, $self->id);
     $dbh->do('DELETE FROM flags WHERE ' . $dbh->sql_in('id', $flag_ids))
         if @$flag_ids;
-    $dbh->do('DELETE FROM attach_data WHERE id = ?', undef, $self->id);
     $dbh->do('UPDATE attachments SET mimetype = ?, ispatch = ?, isobsolete = ?, attach_size = ?
               WHERE attach_id = ?', undef, ('text/plain', 0, 1, 0, $self->id));
     $dbh->bz_commit_transaction();
+    current_storage->remove($self->id);
 
     # As we don't call SUPER->remove_from_db we need to manually clear
     # memcached here.
@@ -989,5 +884,33 @@ sub get_content_type {
     return $content_type;
 }
 
+sub current_storage {
+    return state $storage //= get_storage_by_name(Bugzilla->params->{attachment_storage});
+}
+
+sub get_storage_names {
+    require Bugzilla::Config::Attachment;
+    foreach my $param (Bugzilla::Config::Attachment->get_param_list) {
+        next unless $param->{name} eq 'attachment_storage';
+        return @{ $param->{choices} };
+    }
+    return [];
+}
+
+sub get_storage_by_name {
+    my ($name) = @_;
+    # all options for attachment_storage need to be handled here
+    if ($name eq 'database') {
+        require Bugzilla::Attachment::Database;
+        return Bugzilla::Attachment::Database->new();
+    }
+    elsif ($name eq 'filesystem') {
+        require Bugzilla::Attachment::FileSystem;
+        return Bugzilla::Attachment::FileSystem->new();
+    }
+    else {
+        return undef;
+    }
+}
 
 1;
diff --git a/Bugzilla/Attachment/Database.pm b/Bugzilla/Attachment/Database.pm
new file mode 100644
index 000000000..24617cacc
--- /dev/null
+++ b/Bugzilla/Attachment/Database.pm
@@ -0,0 +1,59 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+
+package Bugzilla::Attachment::Database;
+use strict;
+use warnings;
+
+use Bugzilla::Util qw(trick_taint);
+
+sub new {
+    return bless({}, shift);
+}
+
+sub store {
+    my ($self, $attach_id, $data) = @_;
+    my $dbh = Bugzilla->dbh;
+    my $sth = $dbh->prepare("INSERT INTO attach_data (id, thedata) VALUES ($attach_id, ?)");
+    trick_taint($data);
+    $sth->bind_param(1, $data, $dbh->BLOB_TYPE);
+    $sth->execute();
+}
+
+sub retrieve {
+    my ($self, $attach_id) = @_;
+    my $dbh = Bugzilla->dbh;
+    my ($data) = $dbh->selectrow_array(
+        "SELECT thedata FROM attach_data WHERE id = ?",
+        undef,
+        $attach_id
+    );
+    return $data;
+}
+
+sub remove {
+    my ($self, $attach_id) = @_;
+    my $dbh = Bugzilla->dbh;
+    $dbh->do(
+        "DELETE FROM attach_data WHERE id = ?",
+        undef,
+        $attach_id
+    );
+}
+
+sub exists {
+    my ($self, $attach_id) = @_;
+    my $dbh = Bugzilla->dbh;
+    my ($exists) = $dbh->selectrow_array(
+        "SELECT 1 FROM attach_data WHERE id = ?",
+        undef,
+        $attach_id
+    );
+    return !!$exists;
+}
+
+1;
diff --git a/Bugzilla/Attachment/FileSystem.pm b/Bugzilla/Attachment/FileSystem.pm
new file mode 100644
index 000000000..675184c49
--- /dev/null
+++ b/Bugzilla/Attachment/FileSystem.pm
@@ -0,0 +1,61 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+
+package Bugzilla::Attachment::FileSystem;
+use strict;
+use warnings;
+
+use Bugzilla::Constants qw(bz_locations);
+
+sub new {
+    return bless({}, shift);
+}
+
+sub store {
+    my ($self, $attach_id, $data) = @_;
+    my $path = _local_path($attach_id);
+    mkdir($path, 0770) unless -d $path;
+    open(my $fh, '>', _local_file($attach_id));
+    binmode($fh);
+    print $fh $data;
+    close($fh);
+}
+
+sub retrieve {
+    my ($self, $attach_id) = @_;
+    if (open(my $fh, '<', _local_file($attach_id))) {
+        local $/;
+        binmode($fh);
+        my $data = <$fh>;
+        close($fh);
+        return $data;
+    }
+    return undef;
+}
+
+sub remove {
+    my ($self, $attach_id) = @_;
+    unlink(_local_file($attach_id));
+}
+
+sub exists {
+    my ($self, $attach_id) = @_;
+    return -e _local_file($attach_id);
+}
+
+sub _local_path {
+    my ($attach_id) = @_;
+    my $hash = sprintf('group.%03d', $attach_id % 1000);
+    return bz_locations()->{attachdir} . '/' . $hash;
+}
+
+sub _local_file {
+    my ($attach_id) = @_;
+    return _local_path($attach_id) . '/attachment.' . $attach_id;
+}
+
+1;
diff --git a/Bugzilla/Config/Attachment.pm b/Bugzilla/Config/Attachment.pm
index e6e3b7f3d..cf87f4c89 100644
--- a/Bugzilla/Config/Attachment.pm
+++ b/Bugzilla/Config/Attachment.pm
@@ -66,19 +66,12 @@ sub get_param_list {
    checker => \&check_maxattachmentsize
   },
 
-  # The maximum size (in bytes) for patches and non-patch attachments.
-  # The default limit is 1000KB, which is 24KB less than mysql's default
-  # maximum packet size (which determines how much data can be sent in a
-  # single mysql packet and thus how much data can be inserted into the
-  # database) to provide breathing space for the data in other fields of
-  # the attachment record as well as any mysql packet overhead (I don't
-  # know of any, but I suspect there may be some.)
-
   {
-   name => 'maxlocalattachment',
-   type => 't',
-   default => '0',
-   checker => \&check_numeric
+   name => 'attachment_storage',
+   type => 's',
+   choices => ['database', 'filesystem'],
+   default => 'database',
+   checker => \&check_multi
   } );
   return @param_list;
 }
diff --git a/scripts/migrate-attachments.pl b/scripts/migrate-attachments.pl
new file mode 100755
index 000000000..40342c179
--- /dev/null
+++ b/scripts/migrate-attachments.pl
@@ -0,0 +1,111 @@
+#!/usr/bin/perl
+
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+
+use strict;
+use warnings;
+
+use FindBin qw($RealBin);
+use lib "$RealBin/..", "$RealBin/../lib";
+
+use Bugzilla;
+use Bugzilla::Attachment;
+use Bugzilla::Install::Util qw(indicate_progress);
+use Getopt::Long qw(GetOptions);
+
+my @storage_names = Bugzilla::Attachment->get_storage_names();
+
+my %options;
+GetOptions(\%options, 'mirror=s@{2}', 'delete=s') or exit(1);
+unless ($options{mirror} || $options{delete}) {
+    die <<EOF;
+Syntax:
+    migrate-attachments.pl --mirror source destination
+    migrate-attachments.pl --delete source
+
+'mirror'
+    Copies all attachments from the specified source to the destination.
+    Attachments which already exist at the destination will not be copied
+    again.  Attachments deleted on the source will be deleted from the
+    destination.
+
+'delete'
+    Deletes all attachments in the specified location.  This operation cannot
+    be undone.
+
+Valid locations:
+    @storage_names
+
+EOF
+}
+
+my $dbh = Bugzilla->dbh;
+my ($total) = $dbh->selectrow_array("SELECT COUNT(*) FROM attachments");
+
+if ($options{mirror}) {
+    if ($options{mirror}->[0] eq $options{mirror}->[1]) {
+        die "Source and destination must be different\n";
+    }
+    my ($source, $dest) = map { storage($_) } @{ $options{mirror} };
+    confirm(sprintf('Mirror %s attachments from %s to %s?', $total, @{ $options{mirror} }));
+
+    my $sth = $dbh->prepare("SELECT attach_id, attach_size FROM attachments ORDER BY attach_id");
+    $sth->execute();
+    my ($count, $deleted, $stored) = (0, 0, 0);
+    while (my ($attach_id, $attach_size) = $sth->fetchrow_array()) {
+        indicate_progress({ total => $total, current => ++$count });
+
+        # remove deleted attachments
+        if ($attach_size == 0 && $dest->exists($attach_id)) {
+            $dest->remove($attach_id);
+            $deleted++;
+        }
+
+        # store attachments that don't already exist
+        elsif ($attach_size != 0 && !$dest->exists($attach_id)) {
+            if (my $data = $source->retrieve($attach_id)) {
+                $dest->store($attach_id, $data);
+                $stored++;
+            }
+        }
+    }
+    print "\n";
+    print "Attachments stored: $stored\n";
+    print "Attachments deleted: $deleted\n" if $deleted;
+}
+
+elsif ($options{delete}) {
+    my $storage = storage($options{delete});
+    confirm(sprintf('DELETE %s attachments from %s?', $total, $options{delete}));
+
+    my $sth = $dbh->prepare("SELECT attach_id FROM attachments ORDER BY attach_id");
+    $sth->execute();
+    my ($count, $deleted) = (0, 0);
+    while (my ($attach_id) = $sth->fetchrow_array()) {
+        indicate_progress({ total => $total, current => ++$count });
+        if ($storage->exists($attach_id)) {
+            $storage->remove($attach_id);
+            $deleted++;
+        }
+    }
+    print "\n";
+    print "Attachments deleted: $deleted\n";
+}
+
+sub storage {
+    my ($name) = @_;
+    my $storage = Bugzilla::Attachment::get_storage_by_name($name)
+        or die "Invalid attachment location: $name\n";
+    return $storage;
+}
+
+sub confirm {
+    my ($prompt) = @_;
+    print $prompt, "\n\nPress <Ctrl-C> to stop or <Enter> to continue..\n";
+    getc();
+}
diff --git a/template/en/default/admin/params/attachment.html.tmpl b/template/en/default/admin/params/attachment.html.tmpl
index 4075374bc..bdd20c676 100644
--- a/template/en/default/admin/params/attachment.html.tmpl
+++ b/template/en/default/admin/params/attachment.html.tmpl
@@ -57,19 +57,17 @@
     _ " (such as <tt>1234.your.domain.com</tt>) must point to this same"
     _ " $terms.Bugzilla instance.",
 
-  allow_attachment_deletion => "If this option is on, administrators will be able to delete " _
-                               "the content of attachments.",
+  allow_attachment_deletion =>
+    "If this option is on, administrators will be able to delete " _
+    "the content of attachments.",
 
-  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}.",
+  maxattachmentsize =>
+    "The maximum size (in kilobytes) of attachments.",
 
-  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." }
+  attachment_storage =>
+    "Where attachment data should be stored.  If this value is changed you " _
+    "must use <tt>scripts/mmigrate-attachments</tt> to migrate existing " _
+    "attachments.",
+
+  }
 %]
diff --git a/template/en/default/attachment/list.html.tmpl b/template/en/default/attachment/list.html.tmpl
index 2b0b31584..4aae3abef 100644
--- a/template/en/default/attachment/list.html.tmpl
+++ b/template/en/default/attachment/list.html.tmpl
@@ -19,8 +19,6 @@
   #                 Frédéric Buclin <LpSolit@gmail.com>
   #%]
 
-[% RETURN UNLESS attachments.size || Param("maxattachmentsize") || Param("maxlocalattachment") %]
-
 <script type="text/javascript">
 <!--
 function toggle_display(link) {
@@ -168,10 +166,8 @@ function toggle_display(link) {
           [% END %]
         </span>
       [% END %]
-      [% IF Param("maxattachmentsize") || Param("maxlocalattachment") %]
-        <a href="attachment.cgi?bugid=[% bugid %]&amp;action=enter">Add an attachment</a>
-        (proposed patch, testcase, etc.)
-      [% END %]
+      <a href="attachment.cgi?bugid=[% bugid %]&amp;action=enter">Add an attachment</a>
+      (proposed patch, testcase, etc.)
     </td>
   </tr>
 
diff --git a/template/en/default/bug/create/create.html.tmpl b/template/en/default/bug/create/create.html.tmpl
index 98290f1c2..ba5d08273 100644
--- a/template/en/default/bug/create/create.html.tmpl
+++ b/template/en/default/bug/create/create.html.tmpl
@@ -559,7 +559,6 @@ TUI_hide_default('attachment_text_field');
   </tr>
 </tbody>
 
-  [% IF Param("maxattachmentsize") || Param("maxlocalattachment") %]
   <tr>
     <th>Attachment:</th>
     <td colspan="3">
@@ -592,7 +591,6 @@ TUI_hide_default('attachment_text_field');
       </div>
     </td>
   </tr>
-  [% END %]
 </tbody>
 
 <tbody class="expert_fields">
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index f8d8985f9..e8510b134 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -688,13 +688,9 @@
 
   [% 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. Attachments cannot be more than
-    [%# Hack to get the max value between both limits %]
-    [%+ max_limit.nsort.last FILTER html %] KB. <br>
+    [%+ Param('maxattachmentsize') FILTER html %] KB. <br>
     We recommend that you store your attachment elsewhere
     and then paste the URL to this file on the attachment
     creation page in the appropriate text field.