Factor out photo storage into PhotoStorage::FileSystem backend

11 files changed, 203 insertions, 59 deletions

diff --git a/bin/fixmystreet.com/fixture b/bin/fixmystreet.com/fixture
index 091fcab9d..2f882381a 100755
--- a/bin/fixmystreet.com/fixture
+++ b/bin/fixmystreet.com/fixture
@@ -19,6 +19,7 @@ use List::Util qw(shuffle);
 use Path::Tiny;
 use FixMyStreet;
 use FixMyStreet::Cobrand;
+use FixMyStreet::PhotoStorage;
 use FixMyStreet::DB::Factories;
 use Getopt::Long::Descriptive;
 
@@ -111,8 +112,7 @@ foreach (FixMyStreet::Cobrand->available_cobrand_classes) {
     }
 }
 
-my $cache_dir = path(FixMyStreet->config('UPLOAD_DIR'));
-$cache_dir->mkpath;
+FixMyStreet::PhotoStorage::backend->init()
 
 my $user = $users{'user@example.org'};
 my $num = 20;
diff --git a/conf/general.yml-example b/conf/general.yml-example
index 1f85b6fe7..11fe654ff 100644
--- a/conf/general.yml-example
+++ b/conf/general.yml-example
@@ -68,11 +68,24 @@ LANGUAGES:
 # from the server, you can set the time zone here (standard time zone string)
 TIME_ZONE: ""
 
-# File locations for uploaded photos and cached geocoding results.
-# Absolute paths, or relative to the project's root directory
-UPLOAD_DIR: '../upload/'
+# File locations for cached geocoding results.
 GEO_CACHE: '../cache/'
 
+# Photo storage options.
+# Which storage backend to use. Options are 'FileSystem' and 'S3'.
+PHOTO_STORAGE_BACKEND: 'FileSystem'
+
+# FileSystem-specific options
+PHOTO_STORAGE_OPTIONS:
+  # Where uploaded photos will be stored
+  UPLOAD_DIR: '../upload/'
+
+# If using the S3 backend, you'll need to set the following options instead:
+#PHOTO_STORAGE_OPTIONS:
+#  BUCKET: 'fixmystreet-photos'
+#  ACCESS_KEY: ''
+#  SECRET_KEY: ''
+
 # Location of MapIt, to map points to administrative areas, and what types of
 # area from it you want to use. If left blank, a default area will be used
 # everywhere (a URL needs to be given for non-web things, like sending of
diff --git a/perllib/FixMyStreet/App.pm b/perllib/FixMyStreet/App.pm
index 160d2851e..afc8bd918 100644
--- a/perllib/FixMyStreet/App.pm
+++ b/perllib/FixMyStreet/App.pm
@@ -129,10 +129,18 @@ __PACKAGE__->log->disable('debug')    #
   unless __PACKAGE__->debug;
 
 # Check upload_dir
-my $cache_dir = path(FixMyStreet->config('UPLOAD_DIR'))->absolute(FixMyStreet->path_to());
-$cache_dir->mkpath;
-unless ( -d $cache_dir && -w $cache_dir ) {
-    warn "\x1b[31mCan't find/write to photo cache directory '$cache_dir'\x1b[0m\n";
+# TODO: Should this check be part of PhotoStorage::FileSystem?
+if (
+    FixMyStreet->config('PHOTO_STORAGE_BACKEND') eq 'FileSystem' ||
+    !defined FixMyStreet->config('PHOTO_STORAGE_BACKEND') # Backwards compatibility
+   ) {
+    my $cache_dir = FixMyStreet->config('PHOTO_STORAGE_OPTIONS')->{UPLOAD_DIR}
+                    || FixMyStreet->config('UPLOAD_DIR');
+    $cache_dir = path($cache_dir)->absolute(FixMyStreet->path_to());
+    $cache_dir->mkpath;
+    unless ( -d $cache_dir && -w $cache_dir ) {
+        warn "\x1b[31mCan't find/write to photo cache directory '$cache_dir'\x1b[0m\n";
+    }
 }
 
 =head1 NAME
diff --git a/perllib/FixMyStreet/App/Model/PhotoSet.pm b/perllib/FixMyStreet/App/Model/PhotoSet.pm
index 8fcc1700e..58e1a135d 100644
--- a/perllib/FixMyStreet/App/Model/PhotoSet.pm
+++ b/perllib/FixMyStreet/App/Model/PhotoSet.pm
@@ -3,7 +3,6 @@ package FixMyStreet::App::Model::PhotoSet;
 # TODO this isn't a Cat model, rename to something else
 
 use Moose;
-use Path::Tiny 'path';
 
 my $IM = eval {
     require Image::Magick;
@@ -12,7 +11,6 @@ my $IM = eval {
 };
 
 use Scalar::Util 'openhandle', 'blessed';
-use Digest::SHA qw(sha1_hex);
 use Image::Size;
 use IPC::Cmd qw(can_run);
 use IPC::Open3;
@@ -57,28 +55,23 @@ has data_items => ( # either a) split from db_data or b) provided by photo uploa
         my $self = shift;
         my $data = $self->db_data or return [];
 
-        return [$data] if (detect_type($data));
+        return [$data] if ($self->storage->detect_type($data));
 
         return [ split ',' => $data ];
     },
 );
 
-has upload_dir => (
+has storage => (
     is => 'ro',
     lazy => 1,
     default => sub {
-        path(FixMyStreet->config('UPLOAD_DIR'))->absolute(FixMyStreet->path_to());
-    },
+        my $class = 'FixMyStreet::PhotoStorage::';
+        $class .= FixMyStreet->config('PHOTO_STORAGE_BACKEND') || 'FileSystem';
+        eval "use $class";
+        return $class->new();
+    }
 );
 
-sub detect_type {
-    return 'jpeg' if $_[0] =~ /^\x{ff}\x{d8}/;
-    return 'png' if $_[0] =~ /^\x{89}\x{50}/;
-    return 'tiff' if $_[0] =~ /^II/;
-    return 'gif' if $_[0] =~ /^GIF/;
-    return '';
-}
-
 =head2 C<ids>, C<num_images>, C<get_id>, C<all_ids>
 
 C<$photoset-E<GT>ids> is an arrayref containing the fileid data.
@@ -166,25 +159,20 @@ has ids => ( #  Arrayref of $fileid tuples (always, so post upload/raw data proc
                     return ();
                 }
 
-                # we have an image we can use - save it to the upload dir for storage
-                my $fileid = $self->get_fileid($photo_blob);
-                my $file = $self->get_file($fileid, $type);
-                $upload->copy_to( $file );
-                return $file->basename;
-
+                # we have an image we can use - save it to storage
+                return $self->storage->store_photo($photo_blob);
             }
-            if (my $type = detect_type($part)) {
+
+            # It might be a raw file stored in the DB column...
+            if (my $type = $self->storage->detect_type($part)) {
                 my $photo_blob = $part;
-                my $fileid = $self->get_fileid($photo_blob);
-                my $file = $self->get_file($fileid, $type);
-                $file->spew_raw($photo_blob);
-                return $file->basename;
+                return $self->storage->store_photo($photo_blob);
+                # TODO: Should this update the DB record with a pointer to the
+                # newly-stored file, instead of leaving it in the DB?
             }
-            my ($fileid, $type) = split /\./, $part;
-            $type ||= 'jpeg';
-            if ($fileid && length($fileid) == 40) {
-                my $file = $self->get_file($fileid, $type);
-                $file->basename;
+
+            if (my $key = $self->storage->validate_key($part)) {
+                $key;
             } else {
                 # A bad hash, probably a bot spamming with bad data.
                 ();
@@ -194,24 +182,11 @@ has ids => ( #  Arrayref of $fileid tuples (always, so post upload/raw data proc
     },
 );
 
-sub get_fileid {
-    my ($self, $photo_blob) = @_;
-    return sha1_hex($photo_blob);
-}
-
-sub get_file {
-    my ($self, $fileid, $type) = @_;
-    my $cache_dir = $self->upload_dir;
-    return path( $cache_dir, "$fileid.$type" );
-}
-
 sub get_raw_image {
     my ($self, $index) = @_;
     my $filename = $self->get_id($index);
-    my ($fileid, $type) = split /\./, $filename;
-    my $file = $self->get_file($fileid, $type);
-    if ($file->exists) {
-        my $photo = $file->slurp_raw;
+    my ($photo, $type) = $self->storage->retrieve_photo($filename);
+    if ($photo) {
         return {
             data => $photo,
             content_type => "image/$type",
diff --git a/perllib/FixMyStreet/PhotoStorage.pm b/perllib/FixMyStreet/PhotoStorage.pm
new file mode 100644
index 000000000..99f0bdab6
--- /dev/null
+++ b/perllib/FixMyStreet/PhotoStorage.pm
@@ -0,0 +1,30 @@
+package FixMyStreet::PhotoStorage;
+
+use Moose;
+use Digest::SHA qw(sha1_hex);
+
+
+sub detect_type {
+    my ($self, $photo) = @_;
+    return 'jpeg' if $photo =~ /^\x{ff}\x{d8}/;
+    return 'png' if $photo =~ /^\x{89}\x{50}/;
+    return 'tiff' if $photo =~ /^II/;
+    return 'gif' if $photo =~ /^GIF/;
+    return '';
+}
+
+=head2 get_fileid
+
+Calculates an identifier for a binary blob of photo data.
+This is just the SHA1 hash of the blob currently.
+
+=cut
+
+sub get_fileid {
+    my ($self, $photo_blob) = @_;
+    return sha1_hex($photo_blob);
+}
+
+
+
+1;
diff --git a/perllib/FixMyStreet/PhotoStorage/FileSystem.pm b/perllib/FixMyStreet/PhotoStorage/FileSystem.pm
new file mode 100644
index 000000000..0600f867d
--- /dev/null
+++ b/perllib/FixMyStreet/PhotoStorage/FileSystem.pm
@@ -0,0 +1,95 @@
+package FixMyStreet::PhotoStorage::FileSystem;
+
+use Moose;
+use parent 'FixMyStreet::PhotoStorage';
+
+use Path::Tiny 'path';
+
+
+has upload_dir => (
+    is => 'ro',
+    lazy => 1,
+    default => sub {
+        my $dir = FixMyStreet->config('PHOTO_STORAGE_OPTIONS')->{UPLOAD_DIR} ||
+                  FixMyStreet->config('UPLOAD_DIR');
+        return path($dir)->absolute(FixMyStreet->path_to());
+    },
+);
+
+
+=head2 get_file
+
+Returns a Path::Tiny path to a file on disk identified by an ID and type.
+File may or may not exist. This handle is then used to read photo data or
+write to disk.
+
+=cut
+
+sub get_file {
+    my ($self, $fileid, $type) = @_;
+    my $cache_dir = $self->upload_dir;
+    return path( $cache_dir, "$fileid.$type" );
+}
+
+
+=head2 store_photo
+
+Stores a blob of binary data representing a photo on disk.
+Returns a key which is used in the future to get the contents of the file.
+
+=cut
+
+sub store_photo {
+    my ($self, $photo_blob) = @_;
+
+    my $type = $self->detect_type($photo_blob) || 'jpeg';
+    my $fileid = $self->get_fileid($photo_blob);
+    my $file = $self->get_file($fileid, $type);
+    $file->spew_raw($photo_blob);
+
+    return $file->basename;
+}
+
+
+=head2 retrieve_photo
+
+Fetches the file content of a particular photo from storage.
+Returns the binary blob and the filetype, if the photo exists in storage.
+
+=cut
+
+sub retrieve_photo {
+    my ($self, $filename) = @_;
+
+    my ($fileid, $type) = split /\./, $filename;
+    my $file = $self->get_file($fileid, $type);
+    if ($file->exists) {
+        my $photo = $file->slurp_raw;
+        return ($photo, $type);
+    }
+}
+
+
+=head2 validate_key
+
+A long-running FMS instance might have reports whose photo IDs in the DB
+don't include the file extension. This function takes a value from the DB and
+returns a 'tidied' version that can be used when calling photo_exists
+or retrieve_photo.
+
+If the passed key doesn't seem like it'll result in a valid filename (i.e.
+it's not a 40-char SHA1 hash) returns undef.
+
+=cut
+
+sub validate_key {
+    my ($self, $key) = @_;
+
+    my ($fileid, $type) = split /\./, $key;
+    $type ||= 'jpeg';
+    if ($fileid && length($fileid) == 40) {
+        return "$fileid.$type";
+    }
+}
+
+1;
diff --git a/t/app/controller/photo.t b/t/app/controller/photo.t
index e9183836b..e28ee1946 100644
--- a/t/app/controller/photo.t
+++ b/t/app/controller/photo.t
@@ -24,7 +24,10 @@ subtest "Check multiple upload worked" => sub {
     FixMyStreet::override_config {
         ALLOWED_COBRANDS => [ { fixmystreet => '.' } ],
         MAPIT_URL => 'http://mapit.uk/',
-        UPLOAD_DIR => $UPLOAD_DIR,
+        PHOTO_STORAGE_BACKEND => 'FileSystem',
+        PHOTO_STORAGE_OPTIONS => {
+            UPLOAD_DIR => $UPLOAD_DIR,
+        },
     }, sub {
 
         $mech->log_in_ok('test@example.com');
@@ -77,7 +80,10 @@ subtest "Check photo uploading URL works" => sub {
 
     # submit initial pc form
     FixMyStreet::override_config {
-        UPLOAD_DIR => $UPLOAD_DIR,
+        PHOTO_STORAGE_BACKEND => 'FileSystem',
+        PHOTO_STORAGE_OPTIONS => {
+            UPLOAD_DIR => $UPLOAD_DIR,
+        },
     }, sub {
         $mech->post( '/photo/upload',
             Content_Type => 'form-data',
diff --git a/t/app/model/photoset.t b/t/app/model/photoset.t
index d171ba88b..708bda891 100644
--- a/t/app/model/photoset.t
+++ b/t/app/model/photoset.t
@@ -15,7 +15,10 @@ my $db = FixMyStreet::DB->schema;
 my $user = $db->resultset('User')->find_or_create({ name => 'Bob', email => 'bob@example.com' });
 
 FixMyStreet::override_config {
-    UPLOAD_DIR => $UPLOAD_DIR,
+    PHOTO_STORAGE_BACKEND => 'FileSystem',
+    PHOTO_STORAGE_OPTIONS => {
+        UPLOAD_DIR => $UPLOAD_DIR,
+    },
 }, sub {
 
 my $image_path = path('t/app/controller/sample.jpg');
diff --git a/t/cobrand/zurich.t b/t/cobrand/zurich.t
index eccb0c8eb..dbd6ec882 100644
--- a/t/cobrand/zurich.t
+++ b/t/cobrand/zurich.t
@@ -416,7 +416,10 @@ FixMyStreet::override_config {
     ALLOWED_COBRANDS => [ 'zurich' ],
     MAPIT_URL => 'http://mapit.zurich/',
     MAP_TYPE => 'Zurich,OSM',
-    UPLOAD_DIR => $UPLOAD_DIR,
+    PHOTO_STORAGE_BACKEND => 'FileSystem',
+    PHOTO_STORAGE_OPTIONS => {
+        UPLOAD_DIR => $UPLOAD_DIR,
+    },
 }, sub {
     # Photo publishing
     $mech->get_ok( '/admin/report_edit/' . $report->id );
diff --git a/t/open311.t b/t/open311.t
index 4dc1b2959..e1ad578d7 100644
--- a/t/open311.t
+++ b/t/open311.t
@@ -322,7 +322,10 @@ subtest 'check media url set' => sub {
     $comment->cobrand('fixmystreet');
 
     FixMyStreet::override_config {
-        UPLOAD_DIR => $UPLOAD_DIR,
+        PHOTO_STORAGE_BACKEND => 'FileSystem',
+        PHOTO_STORAGE_OPTIONS => {
+            UPLOAD_DIR => $UPLOAD_DIR,
+        },
     }, sub {
         my $results = make_update_req( $comment, '<?xml version="1.0" encoding="utf-8"?><service_request_updates><request_update><update_id>248</update_id></request_update></service_request_updates>' );
 
diff --git a/t/sendreport/open311.t b/t/sendreport/open311.t
index 26764dc19..e68a0aa3c 100644
--- a/t/sendreport/open311.t
+++ b/t/sendreport/open311.t
@@ -74,6 +74,10 @@ subtest 'test report with multiple photos only sends one', sub {
         STAGING_FLAGS => { send_reports => 1 },
         ALLOWED_COBRANDS => [ 'fixmystreet' ],
         MAPIT_URL => 'http://mapit.uk/',
+        PHOTO_STORAGE_BACKEND => 'FileSystem',
+        PHOTO_STORAGE_OPTIONS => {
+            UPLOAD_DIR => $UPLOAD_DIR,
+        },
     }, sub {
         $test_data = FixMyStreet::Script::Reports::send();
     };
@@ -107,6 +111,10 @@ subtest 'test sending multiple photos', sub {
         STAGING_FLAGS => { send_reports => 1 },
         ALLOWED_COBRANDS => [ 'tester' ],
         MAPIT_URL => 'http://mapit.uk/',
+        PHOTO_STORAGE_BACKEND => 'FileSystem',
+        PHOTO_STORAGE_OPTIONS => {
+            UPLOAD_DIR => $UPLOAD_DIR,
+        },
     }, sub {
         $test_data = FixMyStreet::Script::Reports::send();
     };