From 4f933d34f5ea4f38efa64dec081e262360b035a9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 30 Dec 2012 13:44:37 -0800
Subject: [PATCH] Improve S3 integration

Summary:
  - Fixes T2257. We wrote a 0-length file (erroneously?) and currently throw when retrieving it. This also happens if you intentionally upload an empty file. I'm not sure what happened with the image: we check for errors during the write, so its existence implies S3 told us the write was successful and then lost the data. Since this is a one-off, I'm not too worried about it. The indistinguishable case of an actually empty file is fixed, at least.
  - Writes to a directory like "phabricator/ab/cd/efgh" instead of "phabricator/abcdefgh". When I had to go look for the file on S3 it took a few minutes of scrolling since the web interface isn't very fast. Make it so a file can be located by navigating through pieces of the hash.

Test Plan: Viewed an empty file, no fatal. Viewed the file from T2257 locally, no fatal (no data either, but it's gone). Uploaded a file, saw a nice path.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2257

Differential Revision: https://secure.phabricator.com/D4303
---
 .../engine/PhabricatorS3FileStorageEngine.php | 22 ++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/applications/files/engine/PhabricatorS3FileStorageEngine.php b/src/applications/files/engine/PhabricatorS3FileStorageEngine.php
index bad2d34a2e..b1ca579394 100644
--- a/src/applications/files/engine/PhabricatorS3FileStorageEngine.php
+++ b/src/applications/files/engine/PhabricatorS3FileStorageEngine.php
@@ -31,7 +31,17 @@ final class PhabricatorS3FileStorageEngine
   public function writeFile($data, array $params) {
     $s3 = $this->newS3API();
 
-    $name = 'phabricator/'.Filesystem::readRandomCharacters(20);
+    // Generate a random name for this file. We add some directories to it
+    // (e.g. 'abcdef123456' becomes 'ab/cd/ef123456') to make large numbers of
+    // files more browsable with web/debugging tools like the S3 administration
+    // tool.
+    $seed = Filesystem::readRandomCharacters(20);
+    $parts = array(
+      substr($seed, 0, 2),
+      substr($seed, 2, 2),
+      substr($seed, 4),
+    );
+    $name = 'phabricator/'.implode('/', $parts);
 
     AphrontWriteGuard::willWrite();
     $s3->putObject(
@@ -52,7 +62,14 @@ final class PhabricatorS3FileStorageEngine
     $result = $this->newS3API()->getObject(
       $this->getBucketName(),
       $handle);
-    return $result->body;
+
+    // NOTE: The implementation of the API that we're using may respond with
+    // a successful result that has length 0 and no body property.
+    if (isset($result->body)) {
+      return $result->body;
+    } else {
+      return '';
+    }
   }
 
 
@@ -61,7 +78,6 @@ final class PhabricatorS3FileStorageEngine
    * @task impl
    */
   public function deleteFile($handle) {
-
     AphrontWriteGuard::willWrite();
     $this->newS3API()->deleteObject(
       $this->getBucketName(),