From: dklawren <dklawren@users.noreply.github.com>
Date: Mon, 9 Dec 2019 16:28:14 +0000 (-0500)
Subject: Bug 1588221 - Update current s3 storage controller to only upload attachments over... 
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=78c5c73fa539f58a98f24211e8c8371df3524a24;p=thirdparty%2Fbugzilla.git

Bug 1588221 - Update current s3 storage controller to only upload attachments over a certain size
---

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index e7b310834..f82808d58 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -316,7 +316,7 @@ the content of the attachment
 
 sub data {
   my $self = shift;
-  return $self->{data} //= current_storage()->retrieve($self->id);
+  return $self->{data} //= $self->current_storage->get_data();
 }
 
 =over
@@ -735,7 +735,8 @@ sub create {
     close($data);
     $data = $tmp;
   }
-  current_storage()->store($attachment->id, $data);
+
+  $attachment->current_storage->set_data($data)->set_class();
 
   # Return the new attachment object
   return $attachment;
@@ -827,7 +828,7 @@ sub remove_from_db {
               WHERE attach_id = ?', undef, ('text/plain', 0, 1, 0, $self->id)
   );
   $dbh->bz_commit_transaction();
-  current_storage()->remove($self->id);
+  $self->current_storage->remove_data()->remove_class();
 
   # As we don't call SUPER->remove_from_db we need to manually clear
   # memcached here.
@@ -893,8 +894,31 @@ sub get_content_type {
 }
 
 sub current_storage {
-  return state $storage
-    //= get_storage_by_name(Bugzilla->params->{attachment_storage});
+  my ($self, $override_class) = @_;
+  my $dbh  = Bugzilla->dbh;
+
+  # Sometimes we might want to copy attachment data from one
+  # storage class to another. With this param, we can override
+  # the current class, and then call ->set_data().
+  if ($override_class) {
+    return $self->{current_storage} = $self->get_storage_by_name($override_class);
+  }
+
+  if (!$self->{current_storage}) {
+    my ($current_storage)
+      = $dbh->selectrow_array(
+      "SELECT storage_class FROM attachment_storage_class WHERE id = ?",
+      undef, $self->id);
+    if (!$current_storage) {
+      $self->{current_storage}
+        = $self->get_storage_by_name(Bugzilla->params->{attachment_storage});
+    }
+    else {
+      $self->{current_storage} = $self->get_storage_by_name($current_storage);
+    }
+  }
+
+  return $self->{current_storage};
 }
 
 sub get_storage_names {
@@ -907,20 +931,20 @@ sub get_storage_names {
 }
 
 sub get_storage_by_name {
-  my ($name) = @_;
+  my ($self, $name) = @_;
 
   # all options for attachment_storage need to be handled here
   if ($name eq 'database') {
-    require Bugzilla::Attachment::Database;
-    return Bugzilla::Attachment::Database->new();
+    require Bugzilla::Attachment::Storage::Database;
+    return Bugzilla::Attachment::Storage::Database->new({attachment => $self});
   }
   elsif ($name eq 'filesystem') {
-    require Bugzilla::Attachment::FileSystem;
-    return Bugzilla::Attachment::FileSystem->new();
+    require Bugzilla::Attachment::Storage::FileSystem;
+    return Bugzilla::Attachment::Storage::FileSystem->new({attachment => $self});
   }
   elsif ($name eq 's3') {
-    require Bugzilla::Attachment::S3;
-    return Bugzilla::Attachment::S3->new();
+    require Bugzilla::Attachment::Storage::S3;
+    return Bugzilla::Attachment::Storage::S3->new({attachment => $self});
   }
   else {
     return undef;
diff --git a/Bugzilla/Attachment/Database.pm b/Bugzilla/Attachment/Database.pm
deleted file mode 100644
index fd8f5f000..000000000
--- a/Bugzilla/Attachment/Database.pm
+++ /dev/null
@@ -1,50 +0,0 @@
-# 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 5.10.1;
-use strict;
-use warnings;
-
-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, ?)");
-  $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
deleted file mode 100644
index f10a994fd..000000000
--- a/Bugzilla/Attachment/FileSystem.pm
+++ /dev/null
@@ -1,63 +0,0 @@
-# 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 5.10.1;
-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/Attachment/S3.pm b/Bugzilla/Attachment/S3.pm
deleted file mode 100644
index 7f4755720..000000000
--- a/Bugzilla/Attachment/S3.pm
+++ /dev/null
@@ -1,62 +0,0 @@
-# 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::S3;
-
-use 5.10.1;
-use strict;
-use warnings;
-
-use Bugzilla::Error;
-use Bugzilla::S3;
-
-sub new {
-  my $s3 = Bugzilla::S3->new({
-    aws_access_key_id     => Bugzilla->params->{aws_access_key_id},
-    aws_secret_access_key => Bugzilla->params->{aws_secret_access_key},
-    secure                => 1,
-  });
-  return
-    bless({s3 => $s3, bucket => $s3->bucket(Bugzilla->params->{s3_bucket}),},
-    shift);
-}
-
-sub store {
-  my ($self, $attach_id, $data) = @_;
-  unless ($self->{bucket}->add_key($attach_id, $data)) {
-    warn "Failed to add attachment $attach_id to S3: "
-      . $self->{bucket}->errstr . "\n";
-    ThrowCodeError('s3_add_failed',
-      {attach_id => $attach_id, reason => $self->{bucket}->errstr});
-  }
-}
-
-sub retrieve {
-  my ($self, $attach_id) = @_;
-  my $response = $self->{bucket}->get_key($attach_id);
-  if (!$response) {
-    warn "Failed to retrieve attachment $attach_id from S3: "
-      . $self->{bucket}->errstr . "\n";
-    ThrowCodeError('s3_get_failed',
-      {attach_id => $attach_id, reason => $self->{bucket}->errstr});
-  }
-  return $response->{value};
-}
-
-sub remove {
-  my ($self, $attach_id) = @_;
-  $self->{bucket}->delete_key($attach_id)
-    or warn "Failed to remove attachment $attach_id from S3: "
-    . $self->{bucket}->errstr . "\n";
-}
-
-sub exists {
-  my ($self, $attach_id) = @_;
-  return !!$self->{bucket}->head_key($attach_id);
-}
-
-1;
diff --git a/Bugzilla/Attachment/Storage/Base.pm b/Bugzilla/Attachment/Storage/Base.pm
new file mode 100644
index 000000000..4725f05c7
--- /dev/null
+++ b/Bugzilla/Attachment/Storage/Base.pm
@@ -0,0 +1,38 @@
+# 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::Storage::Base;
+
+use 5.10.1;
+use Moo::Role;
+
+use Type::Utils qw(class_type);
+
+requires qw(set_data get_data remove_data data_exists data_type);
+
+has 'attachment' => (
+  is       => 'ro',
+  required => 1,
+  isa      => class_type({class => 'Bugzilla::Attachment'})
+);
+
+sub set_class {
+  my ($self) = @_;
+  Bugzilla->dbh->do(
+    "REPLACE INTO attachment_storage_class (id, storage_class) VALUES (?, ?)",
+    undef, $self->attachment->id, $self->data_type);
+  return $self;
+}
+
+sub remove_class {
+  my ($self) = @_;
+  Bugzilla->dbh->do("DELETE FROM attachment_storage_class WHERE id = ?",
+    undef, $self->attachment->id);
+  return $self;
+}
+
+1;
diff --git a/Bugzilla/Attachment/Storage/Database.pm b/Bugzilla/Attachment/Storage/Database.pm
new file mode 100644
index 000000000..9da52ef6c
--- /dev/null
+++ b/Bugzilla/Attachment/Storage/Database.pm
@@ -0,0 +1,54 @@
+# 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::Storage::Database;
+
+use 5.10.1;
+use Moo;
+
+with 'Bugzilla::Attachment::Storage::Base';
+
+sub data_type { return 'database'; }
+
+sub set_data {
+  my ($self, $data) = @_;
+  my $dbh = Bugzilla->dbh;
+  my $sth
+    = $dbh->prepare(
+    "REPLACE INTO attach_data (id, thedata) VALUES (?, ?)"
+    );
+  $sth->bind_param(1, $self->attachment->id);
+  $sth->bind_param(2, $data, $dbh->BLOB_TYPE);
+  $sth->execute();
+  return $self;
+}
+
+sub get_data {
+  my ($self) = @_;
+  my $dbh = Bugzilla->dbh;
+  my ($data)
+    = $dbh->selectrow_array("SELECT thedata FROM attach_data WHERE id = ?",
+    undef, $self->attachment->id);
+  return $data;
+}
+
+sub remove_data {
+  my ($self) = @_;
+  my $dbh = Bugzilla->dbh;
+  $dbh->do("DELETE FROM attach_data WHERE id = ?", undef, $self->attachment->id);
+  return $self;
+}
+
+sub data_exists {
+  my ($self)   = @_;
+  my $dbh      = Bugzilla->dbh;
+  my ($exists) = $dbh->selectrow_array("SELECT 1 FROM attach_data WHERE id = ?",
+    undef, $self->attachment->id);
+  return !!$exists;
+}
+
+1;
diff --git a/Bugzilla/Attachment/Storage/FileSystem.pm b/Bugzilla/Attachment/Storage/FileSystem.pm
new file mode 100644
index 000000000..7a67d1b3c
--- /dev/null
+++ b/Bugzilla/Attachment/Storage/FileSystem.pm
@@ -0,0 +1,63 @@
+# 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::Storage::FileSystem;
+
+use 5.10.1;
+use Moo;
+
+use Bugzilla::Constants qw(bz_locations);
+
+with 'Bugzilla::Attachment::Storage::Base';
+
+sub data_type { return 'filesystem'; }
+
+sub set_data {
+  my ($self, $data) = @_;
+  my $path = $self->_local_path();
+  mkdir $path, 0770 unless -d $path;
+  open my $fh, '>', $self->_local_file();
+  binmode $fh;
+  print $fh $data;
+  close $fh;
+  return $self;
+}
+
+sub get_data {
+  my ($self) = @_;
+  if (open my $fh, '<', $self->_local_file()) {
+    local $/;
+    binmode $fh;
+    my $data = <$fh>;
+    close $fh;
+    return $data;
+  }
+}
+
+sub remove_data {
+  my ($self) = @_;
+  unlink $self->_local_file();
+  return $self;
+}
+
+sub data_exists {
+  my ($self) = @_;
+  return -e $self->_local_file();
+}
+
+sub _local_path {
+  my ($self) = @_;
+  my $hash = sprintf 'group.%03d', $self->attachment->id % 1000;
+  return bz_locations()->{attachdir} . '/' . $hash;
+}
+
+sub _local_file {
+  my ($self) = @_;
+  return $self->_local_path() . '/attachment.' . $self->attachment->id;
+}
+
+1;
diff --git a/Bugzilla/Attachment/Storage/S3.pm b/Bugzilla/Attachment/Storage/S3.pm
new file mode 100644
index 000000000..aea04572c
--- /dev/null
+++ b/Bugzilla/Attachment/Storage/S3.pm
@@ -0,0 +1,90 @@
+# 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::Storage::S3;
+
+use 5.10.1;
+use Moo;
+
+use Bugzilla::Error;
+use Bugzilla::S3;
+
+with 'Bugzilla::Attachment::Storage::Base';
+
+has 's3'     => (is => 'lazy');
+has 'bucket' => (is => 'lazy');
+
+sub _build_s3 {
+  my $self = shift;
+  $self->{s3} ||= Bugzilla::S3->new({
+    aws_access_key_id     => Bugzilla->params->{aws_access_key_id},
+    aws_secret_access_key => Bugzilla->params->{aws_secret_access_key},
+    secure                => 1,
+  });
+  return $self->{s3};
+}
+
+sub _build_bucket {
+  my $self = shift;
+  $self->{bucket} ||= $self->s3->bucket(Bugzilla->params->{s3_bucket});
+  return $self->bucket;
+}
+
+sub data_type { return 's3'; }
+
+sub set_data {
+  my ($self, $data) = @_;
+  my $attach_id = $self->attachment->id;
+
+  # If the attachment is larger than attachment_s3_minsize,
+  # we instead store it in the database.
+  if (Bugzilla->params->{attachment_s3_minsize}
+    && $self->attachment->datasize < Bugzilla->params->{attachment_s3_minsize})
+  {
+    require Bugzilla::Attachment::Storage::Database;
+    return Bugzilla::Attachment::Storage::Database->new({attachment => $self->attachment})
+      ->set_data($data);
+  }
+
+  unless ($self->bucket->add_key($attach_id, $data)) {
+    warn "Failed to add attachment $attach_id to S3: "
+      . $self->bucket->errstr . "\n";
+    ThrowCodeError('s3_add_failed',
+      {attach_id => $attach_id, reason => $self->bucket->errstr});
+  }
+
+  return $self;
+}
+
+sub get_data {
+  my ($self)    = @_;
+  my $attach_id = $self->attachment->id;
+  my $response  = $self->bucket->get_key($attach_id);
+  if (!$response) {
+    warn "Failed to retrieve attachment $attach_id from S3: "
+      . $self->bucket->errstr . "\n";
+    ThrowCodeError('s3_get_failed',
+      {attach_id => $attach_id, reason => $self->bucket->errstr});
+  }
+  return $response->{value};
+}
+
+sub remove_data {
+  my ($self) = @_;
+  my $attach_id = $self->attachment->id;
+  $self->bucket->delete_key($attach_id)
+    or warn "Failed to remove attachment $attach_id from S3: "
+    . $self->bucket->errstr . "\n";
+  return $self;
+}
+
+sub data_exists {
+  my ($self) = @_;
+  return !!$self->bucket->head_key($self->attachment->id);
+}
+
+1;
diff --git a/Bugzilla/Config/Attachment.pm b/Bugzilla/Config/Attachment.pm
index dc1a31d48..7aa0ca794 100644
--- a/Bugzilla/Config/Attachment.pm
+++ b/Bugzilla/Config/Attachment.pm
@@ -33,6 +33,12 @@ sub get_param_list {
       default => 'database',
       checker => \&check_storage
     },
+    {
+      name => 'attachment_s3_minsize',
+      type => 't',
+      default => '20000',
+      checker => \&check_numeric
+    },
     {name => 's3_bucket',             type => 't', default => '',},
     {name => 'aws_access_key_id',     type => 't', default => '',},
     {name => 'aws_secret_access_key', type => 't', default => '',},
diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index 185bba455..47638cccb 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -337,7 +337,7 @@ use constant ABSTRACT_SCHEMA => {
         REFERENCES => {TABLE => 'bugs', COLUMN => 'bug_id', DELETE => 'CASCADE'}
       },
       attach_id => {
-        TYPE => 'INT3',
+        TYPE => 'INT5',
         REFERENCES =>
           {TABLE => 'attachments', COLUMN => 'attach_id', DELETE => 'CASCADE'}
       },
@@ -509,7 +509,7 @@ use constant ABSTRACT_SCHEMA => {
 
   attachments => {
     FIELDS => [
-      attach_id => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1},
+      attach_id => {TYPE => 'BIGSERIAL', NOTNULL => 1, PRIMARYKEY => 1},
       bug_id    => {
         TYPE       => 'INT3',
         NOTNULL    => 1,
@@ -538,10 +538,11 @@ use constant ABSTRACT_SCHEMA => {
       attachments_ispatch_idx           => ['ispatch'],
     ],
   },
+
   attach_data => {
     FIELDS => [
       id => {
-        TYPE       => 'INT3',
+        TYPE       => 'INT5',
         NOTNULL    => 1,
         PRIMARYKEY => 1,
         REFERENCES =>
@@ -551,6 +552,20 @@ use constant ABSTRACT_SCHEMA => {
     ],
   },
 
+  attachment_storage_class => {
+    FIELDS => [
+      id => {
+        TYPE       => 'INT5',
+        NOTNULL    => 1,
+        PRIMARYKEY => 1,
+        REFERENCES =>
+          {TABLE => 'attachments', COLUMN => 'attach_id', DELETE => 'CASCADE'}
+      },
+      storage_class => {TYPE => 'varchar(64)', NOTNULL => 1},
+      extra_data    => {TYPE => 'MEDIUMTEXT'}
+    ],
+  },
+
   duplicates => {
     FIELDS => [
       dupe_of => {
@@ -654,7 +669,7 @@ use constant ABSTRACT_SCHEMA => {
         REFERENCES => {TABLE => 'bugs', COLUMN => 'bug_id', DELETE => 'CASCADE'}
       },
       attach_id => {
-        TYPE => 'INT3',
+        TYPE => 'INT5',
         REFERENCES =>
           {TABLE => 'attachments', COLUMN => 'attach_id', DELETE => 'CASCADE'}
       },
@@ -1810,7 +1825,7 @@ use constant ABSTRACT_SCHEMA => {
       user_agent  => {TYPE => 'TINYTEXT',    NOTNULL => 1},
       timestamp   => {TYPE => 'DATETIME',    NOTNULL => 1},
       bug_id      => {TYPE => 'INT3',        NOTNULL => 0},
-      attach_id   => {TYPE => 'INT4',        NOTNULL => 0},
+      attach_id   => {TYPE => 'INT5',        NOTNULL => 0},
       request_url => {TYPE => 'TINYTEXT',    NOTNULL => 1},
       method      => {TYPE => 'TINYTEXT',    NOTNULL => 1},
       action      => {TYPE => 'varchar(20)', NOTNULL => 1},
diff --git a/Bugzilla/DB/Schema/Mysql.pm b/Bugzilla/DB/Schema/Mysql.pm
index 21543b4b3..39c1fc45e 100644
--- a/Bugzilla/DB/Schema/Mysql.pm
+++ b/Bugzilla/DB/Schema/Mysql.pm
@@ -103,10 +103,12 @@ sub _initialize {
     INT2 => 'smallint',
     INT3 => 'mediumint',
     INT4 => 'integer',
+    INT5 => 'bigint(20)',
 
     SMALLSERIAL  => 'smallint auto_increment',
     MEDIUMSERIAL => 'mediumint auto_increment',
     INTSERIAL    => 'integer auto_increment',
+    BIGSERIAL    => 'bigint(20) auto_increment',
 
     TINYTEXT   => 'tinytext',
     MEDIUMTEXT => 'mediumtext',
diff --git a/Bugzilla/DB/Schema/Oracle.pm b/Bugzilla/DB/Schema/Oracle.pm
index 36f957820..751cb718f 100644
--- a/Bugzilla/DB/Schema/Oracle.pm
+++ b/Bugzilla/DB/Schema/Oracle.pm
@@ -46,10 +46,12 @@ sub _initialize {
     INT2 => 'integer',
     INT3 => 'integer',
     INT4 => 'integer',
+    INT5 => 'integer',
 
     SMALLSERIAL  => 'integer',
     MEDIUMSERIAL => 'integer',
     INTSERIAL    => 'integer',
+    BIGSERIAL    => 'integer',
 
     TINYTEXT   => 'varchar(255)',
     MEDIUMTEXT => 'varchar(4000)',
diff --git a/Bugzilla/DB/Schema/Pg.pm b/Bugzilla/DB/Schema/Pg.pm
index e4c7fae7a..b496f3886 100644
--- a/Bugzilla/DB/Schema/Pg.pm
+++ b/Bugzilla/DB/Schema/Pg.pm
@@ -55,10 +55,12 @@ sub _initialize {
     INT2 => 'integer',
     INT3 => 'integer',
     INT4 => 'integer',
+    INT5 => 'integer',
 
     SMALLSERIAL  => 'serial unique',
     MEDIUMSERIAL => 'serial unique',
     INTSERIAL    => 'serial unique',
+    BIGSERIAL    => 'serial unique',
 
     TINYTEXT   => 'varchar(255)',
     MEDIUMTEXT => 'text',
diff --git a/Bugzilla/DB/Schema/Sqlite.pm b/Bugzilla/DB/Schema/Sqlite.pm
index 1bd53515a..81cd8c2ea 100644
--- a/Bugzilla/DB/Schema/Sqlite.pm
+++ b/Bugzilla/DB/Schema/Sqlite.pm
@@ -35,10 +35,12 @@ sub _initialize {
     INT2 => 'integer',
     INT3 => 'integer',
     INT4 => 'integer',
+    INT5 => 'integer',
 
     SMALLSERIAL  => 'SERIAL',
     MEDIUMSERIAL => 'SERIAL',
     INTSERIAL    => 'SERIAL',
+    BIGSERIAL    => 'SERIAL',
 
     TINYTEXT   => 'text',
     MEDIUMTEXT => 'text',
diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index eecf2a816..91eb8f651 100644
--- a/Bugzilla/Install/DB.pm
+++ b/Bugzilla/Install/DB.pm
@@ -808,6 +808,17 @@ sub update_table_definitions {
   # Bug 1612290 - dkl@mozilla.com
   $dbh->bz_add_column('profiles', 'bounce_count', {TYPE => 'INT1', NOTNULL => 1, DEFAULT => 0});
 
+  # Bug 1588221 - dkl@mozilla.com
+  $dbh->bz_alter_column('bugs_activity', 'attach_id', {TYPE => 'INT5'});
+  $dbh->bz_alter_column('attachments', 'attach_id',
+    {TYPE => 'BIGSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
+  $dbh->bz_alter_column('attach_data', 'id',
+    {TYPE => 'INT5', NOTNULL => 1, PRIMARYKEY => 1});
+  $dbh->bz_alter_column('flags',            'attach_id', {TYPE => 'INT5'});
+  $dbh->bz_alter_column('user_request_log', 'attach_id', {TYPE => 'INT5', NOTNULL => 0});
+  _populate_attachment_storage_class();
+
+
   ################################################################
   # New --TABLE-- changes should go *** A B O V E *** this point #
   ################################################################
@@ -4329,6 +4340,19 @@ sub _populate_api_keys_creation_ts {
     {TYPE => 'DATETIME', NOTNULL => 1});
 }
 
+sub _populate_attachment_storage_class {
+  my $dbh = Bugzilla->dbh;
+
+  # Return if we have already made these changes
+  my $count = $dbh->selectrow_array('SELECT COUNT(id) FROM attachment_storage_class');
+  if (!$count) {
+    $dbh->do(
+      "INSERT INTO attachment_storage_class (id, storage_class) SELECT attachments.id, 'database' FROM attachments ORDER BY attachments.id"
+    );
+  }
+}
+
+
 1;
 
 __END__
diff --git a/extensions/Review/Extension.pm b/extensions/Review/Extension.pm
index 8fc103bd6..ca2420241 100644
--- a/extensions/Review/Extension.pm
+++ b/extensions/Review/Extension.pm
@@ -874,7 +874,7 @@ sub db_schema_abstract_schema {
       },
 
       attachment_id => {
-        TYPE => 'INT3',
+        TYPE => 'INT5',
         REFERENCES =>
           {TABLE => 'attachments', COLUMN => 'attach_id', DELETE => 'CASCADE'}
       },
@@ -946,6 +946,9 @@ sub install_update_db {
     $field->set_in_new_bugmail(1);
     $field->update();
   }
+
+  # Bug 1588221 - dkl@mozilla.com
+  $dbh->bz_alter_column('flag_state_activity', 'attachment_id', {TYPE => 'INT5'});
 }
 
 sub install_filesystem {
diff --git a/scripts/attachment-data.pl b/scripts/attachment-data.pl
index 0e89618e6..596d0844b 100755
--- a/scripts/attachment-data.pl
+++ b/scripts/attachment-data.pl
@@ -62,7 +62,7 @@ elsif ($cmd eq 'import') {
     next unless $mem->{data_len};
     next unless check_attachment($attachment, $mem->{bug_id}, $mem->{data_len});
 
-    Bugzilla::Attachment::current_storage()->store($attachment->id, $mem->{data});
+    $attachment->current_storage->set_data($mem->{data})->set_class();
   }
 }
 elsif ($cmd eq 'check') {
@@ -79,7 +79,8 @@ elsif ($cmd eq 'remove') {
   while (my $mem = $archive->read_member) {
     warn "checking $mem->{attach_id}\n";
 
-    my $attachment = Bugzilla::Attachment->new($mem->{attach_id});
+    my $attachment
+      = Bugzilla::Attachment->new({id => $mem->{attach_id}, cache => 1});
     die "bad attachment\n"
       unless check_attachment($attachment, $mem->{bug_id}, $mem->{data_len});
     $remove_ok{$mem->{attach_id}} = 1;
@@ -88,7 +89,8 @@ elsif ($cmd eq 'remove') {
     chomp $attach_id;
     if ($remove_ok{$attach_id}) {
       warn "removing $attach_id\n";
-      Bugzilla::Attachment::current_storage()->remove($attach_id);
+      my $attachment = Bugzilla::Attachment->new({id => $attach_id, cache => 1});
+      $attachment->current_storage->remove()->remove_class();
     }
     else {
       warn "Unable to remove $attach_id, as it did not occur in the archive.\n";
diff --git a/scripts/migrate-attachments.pl b/scripts/migrate-attachments.pl
index 7ef6bea78..47bcc9345 100755
--- a/scripts/migrate-attachments.pl
+++ b/scripts/migrate-attachments.pl
@@ -21,14 +21,23 @@ use Getopt::Long qw(GetOptions);
 my @storage_names = Bugzilla::Attachment->get_storage_names();
 
 my %options;
-GetOptions(\%options, 'mirror=s@{2}', 'copy=s@{2}', 'delete=s') or exit(1);
-unless ($options{mirror} || $options{copy} || $options{delete}) {
+GetOptions(\%options, 'migrate=s@{2}', 'mirror=s@{2}', 'copy=s@{2}', 'delete=s') or exit 1;
+unless ($options{migrate} || $options{mirror} || $options{copy} || $options{delete}) {
   die <<EOF;
 Syntax:
+    migrate-attachments.pl --migrate source destination
     migrate-attachments.pl --mirror source destination
     migrate-attachments.pl --copy source destination
     migrate-attachments.pl --delete source
 
+'migrate'
+    Migrates all attachments from the specified source to the destination.
+    Attachments which already exist at the destination will be overwritten.
+    This also sets the storage class so that the system loads the data from the
+    new destination.
+
+    e.g. migrate-attachments.pl --migrate database s3
+
 'mirror'
     Copies all attachments from the specified source to the destination.
     Attachments which already exist at the destination will not be copied
@@ -59,33 +68,75 @@ EOF
 
 my $dbh = Bugzilla->dbh;
 
+if ($options{migrate}) {
+  if ($options{migrate}->[0] eq $options{migrate}->[1]) {
+    die "Source and destination must be different\n";
+  }
+  my ($source, $dest) = @{$options{migrate}};
+
+  # Do not migrate from database to s3 if minsize is set
+  my $where;
+  if ($source eq 'database' && $dest eq 's3' && Bugzilla->params->{attachment_s3_minsize}) {
+    $where = 'WHERE attach_size > ' . int Bugzilla->params->{attachment_s3_minsize};
+  }
+
+  my ($total) = $dbh->selectrow_array("SELECT COUNT(*) FROM attachments $where");
+  confirm(sprintf
+    'Migrate %s attachments from %s to %s?', $total, @{$options{migrate}});
+
+  my $sth
+    = $dbh->prepare("SELECT attach_id FROM attachments $where ORDER BY attach_id DESC");
+  $sth->execute();
+  my ($count, $migrated) = (0, 0);
+  while (my ($attach_id) = $sth->fetchrow_array()) {
+    indicate_progress({total => $total, current => ++$count});
+
+    my $attachment = Bugzilla::Attachment->new({id => $attach_id, cached => 1});
+
+    # skip deleted attachments
+    next if $attachment->datasize == 0;
+
+    # migrate the attachment
+    if (my $data = $attachment->current_storage($source)->get_data()) {
+      $attachment->current_storage($dest)->set_data($data)->set_class();
+      $attachment->current_storage($source)->remove_data();
+      $migrated++;
+    }
+  }
+  print "\n";
+  print "Attachments migrated: $migrated\n";
+}
+
 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}};
+  my ($source, $dest) = @{$options{mirror}};
 
   my ($total) = $dbh->selectrow_array("SELECT COUNT(*) FROM attachments");
-  confirm(sprintf(
-    'Mirror %s attachments from %s to %s?', $total, @{$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 DESC");
+  my $sth
+    = $dbh->prepare("SELECT attach_id FROM attachments ORDER BY attach_id DESC");
   $sth->execute();
   my ($count, $deleted, $stored) = (0, 0, 0);
-  while (my ($attach_id, $attach_size) = $sth->fetchrow_array()) {
+  while (my ($attach_id) = $sth->fetchrow_array()) {
     indicate_progress({total => $total, current => ++$count});
 
+    my $attachment = Bugzilla::Attachment->new({id => $attach_id, cached => 1});
+
     # remove deleted attachments
-    if ($attach_size == 0 && $dest->exists($attach_id)) {
-      $dest->remove($attach_id);
+    if ($attachment->datasize == 0 && $attachment->current_storage($dest)->data_exists()) {
+      $attachment->current_storage($dest)->remove_data();
       $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);
+    elsif ($attachment->datasize != 0 && !$attachment->current_storage($dest)->data_exists())
+    {
+      if (my $data = $attachment->current_storage($source)->get_data()) {
+        $attachment->current_storage($dest)->set_data($data);
         $stored++;
       }
     }
@@ -99,27 +150,29 @@ elsif ($options{copy}) {
   if ($options{copy}->[0] eq $options{copy}->[1]) {
     die "Source and destination must be different\n";
   }
-  my ($source, $dest) = map { storage($_) } @{$options{copy}};
+  my ($source, $dest) = @{$options{copy}};
 
   my ($total)
     = $dbh->selectrow_array(
     "SELECT COUNT(*) FROM attachments WHERE attach_size != 0");
-  confirm(sprintf(
-    'Copy %s attachments from %s to %s?', $total, @{$options{copy}}));
+  confirm(sprintf
+    'Copy %s attachments from %s to %s?', $total, @{$options{copy}});
 
   my $sth
     = $dbh->prepare(
-    "SELECT attach_id, attach_size FROM attachments WHERE attach_size != 0 ORDER BY attach_id DESC"
+    "SELECT attach_id FROM attachments WHERE attach_size != 0 ORDER BY attach_id DESC"
     );
   $sth->execute();
   my ($count, $stored) = (0, 0);
-  while (my ($attach_id, $attach_size) = $sth->fetchrow_array()) {
+  while (my ($attach_id) = $sth->fetchrow_array()) {
     indicate_progress({total => $total, current => ++$count});
 
+    my $attachment = Bugzilla::Attachment->new({id => $attach_id, cached => 1});
+
     # store attachments that don't already exist
-    if (!$dest->exists($attach_id)) {
-      if (my $data = $source->retrieve($attach_id)) {
-        $dest->store($attach_id, $data);
+    if (!$attachment->current_storage($dest)->data_exists()) {
+      if (my $data = $attachment->current_storage($source)->get_data()) {
+        $attachment->current_storage($dest)->set_data($data);
         $stored++;
       }
     }
@@ -129,11 +182,11 @@ elsif ($options{copy}) {
 }
 
 elsif ($options{delete}) {
-  my $storage = storage($options{delete});
+  my $storage = $options{delete};
   my ($total)
     = $dbh->selectrow_array(
     "SELECT COUNT(*) FROM attachments WHERE attach_size != 0");
-  confirm(sprintf('DELETE %s attachments from %s?', $total, $options{delete}));
+  confirm(sprintf 'DELETE %s attachments from %s?', $total, $options{delete});
 
   my $sth
     = $dbh->prepare(
@@ -143,8 +196,11 @@ elsif ($options{delete}) {
   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);
+
+    my $attachment = Bugzilla::Attachment->new({id => $attach_id, cached => 1});
+
+    if ($attachment->current_storage($storage)->data_exists()) {
+      $attachment->current_storage($storage)->remove_data();
       $deleted++;
     }
   }
@@ -152,15 +208,8 @@ elsif ($options{delete}) {
   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();
+  getc;
 }
diff --git a/template/en/default/admin/params/attachment.html.tmpl b/template/en/default/admin/params/attachment.html.tmpl
index 5f0d5844e..cfabd9d62 100644
--- a/template/en/default/admin/params/attachment.html.tmpl
+++ b/template/en/default/admin/params/attachment.html.tmpl
@@ -47,5 +47,10 @@
     "must use <tt>scripts/migrate-attachments</tt> to migrate existing " _
     "attachments.",
 
+  attachment_s3_minsize =>
+    "When attachment_storage is set to S3, only attachments larger than this " _
+    "value will be uploaded to S3 for performance reasons. Smaller attachments " _
+    "will be stored in the database."
+
   }
 %]