When deleting a file, delete all transformations of the file

Summary:
Fixes T3143. When a user deletes a file, delete all transforms of the file too. In particular, this means that deleting an image deletes all the thumbnails of the image.

In most cases, this aligns with user expectations. The only sort of weird case I can come up with is that memes are transformations of the source macro image, so deleting grumpycat will delete all the hilarious grumpycat memes. This seems not-too-unreasonable, though, and desirable if someone accidentally uploads an inappropriate image which is promptly turned into a meme.

Test Plan:
Added a unit test which covers both inbound and outbound transformations.

Uploaded a file and deleted it, verified its thumbnail was also deleted.

Reviewers: chad, btrahan, joseph.kampf

Reviewed By: btrahan

CC: aran, joseph.kampf

Maniphest Tasks: T3143

Differential Revision: https://secure.phabricator.com/D5879

src/applications/files/query/PhabricatorFileQuery.php

@@ -24,6 +24,38 @@ final class PhabricatorFileQuery
     return $this;
   }
 
+  /**
+   * Select files which are transformations of some other file. For example,
+   * you can use this query to find previously generated thumbnails of an image
+   * file.
+   *
+   * As a parameter, provide a list of transformation specifications. Each
+   * specification is a dictionary with the keys `originalPHID` and `transform`.
+   * The `originalPHID` is the PHID of the original file (the file which was
+   * transformed) and the `transform` is the name of the transform to query
+   * for. If you pass `true` as the `transform`, all transformations of the
+   * file will be selected.
+   *
+   * For example:
+   *
+   *   array(
+   *     array(
+   *       'originalPHID' => 'PHID-FILE-aaaa',
+   *       'transform'    => 'sepia',
+   *     ),
+   *     array(
+   *       'originalPHID' => 'PHID-FILE-bbbb',
+   *       'transform'    => true,
+   *     ),
+   *   )
+   *
+   * This selects the `"sepia"` transformation of the file with PHID
+   * `PHID-FILE-aaaa` and all transformations of the file with PHID
+   * `PHID-FILE-bbbb`.
+   *
+   * @param list<dict>  List of transform specifications, described above.
+   * @return this
+   */
   public function withTransforms(array $specs) {
     foreach ($specs as $spec) {
       if (!is_array($spec) ||
@@ -50,7 +82,7 @@ final class PhabricatorFileQuery
 
     $data = queryfx_all(
       $conn_r,
-      'SELECT * FROM %T f %Q %Q %Q %Q',
+      'SELECT f.* FROM %T f %Q %Q %Q %Q',
       $table->getTableName(),
       $this->buildJoinClause($conn_r),
       $this->buildWhereClause($conn_r),
@@ -108,12 +140,19 @@ final class PhabricatorFileQuery
     if ($this->transforms) {
       $clauses = array();
       foreach ($this->transforms as $transform) {
+        if ($transform['transform'] === true) {
+          $clauses[] = qsprintf(
+            $conn_r,
+            '(t.originalPHID = %s)',
+            $transform['originalPHID']);
+        } else {
           $clauses[] = qsprintf(
             $conn_r,
             '(t.originalPHID = %s AND t.transform = %s)',
             $transform['originalPHID'],
             $transform['transform']);
         }
+      }
       $where[] = qsprintf($conn_r, '(%Q)', implode(') OR (', $clauses));
     }
 

src/applications/files/storage/PhabricatorFile.php

@@ -384,13 +384,35 @@ final class PhabricatorFile extends PhabricatorFileDAO
   }
 
   public function delete() {
-    // delete all records of this file in transformedfile
-    $trans_files = id(new PhabricatorTransformedFile())->loadAllWhere(
-      'TransformedPHID = %s', $this->getPHID());
+
+    // We want to delete all the rows which mark this file as the transformation
+    // of some other file (since we're getting rid of it). We also delete all
+    // the transformations of this file, so that a user who deletes an image
+    // doesn't need to separately hunt down and delete a bunch of thumbnails and
+    // resizes of it.
+
+    $outbound_xforms = id(new PhabricatorFileQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withTransforms(
+        array(
+          array(
+            'originalPHID' => $this->getPHID(),
+            'transform'    => true,
+          ),
+        ))
+      ->execute();
+
+    foreach ($outbound_xforms as $outbound_xform) {
+      $outbound_xform->delete();
+    }
+
+    $inbound_xforms = id(new PhabricatorTransformedFile())->loadAllWhere(
+      'transformedPHID = %s',
+      $this->getPHID());
 
     $this->openTransaction();
-    foreach ($trans_files as $trans_file) {
-      $trans_file->delete();
+      foreach ($inbound_xforms as $inbound_xform) {
+        $inbound_xform->delete();
       }
       $ret = parent::delete();
     $this->saveTransaction();
@@ -398,15 +420,18 @@ final class PhabricatorFile extends PhabricatorFileDAO
     // Check to see if other files are using storage
     $other_file = id(new PhabricatorFile())->loadAllWhere(
       'storageEngine = %s AND storageHandle = %s AND
-      storageFormat = %s AND id != %d LIMIT 1', $this->getStorageEngine(),
-      $this->getStorageHandle(), $this->getStorageFormat(),
+      storageFormat = %s AND id != %d LIMIT 1',
+      $this->getStorageEngine(),
+      $this->getStorageHandle(),
+      $this->getStorageFormat(),
       $this->getID());
 
     // If this is the only file using the storage, delete storage
-    if (count($other_file) == 0) {
+    if (!$other_file) {
       $engine = $this->instantiateStorageEngine();
       $engine->deleteFile($this->getStorageHandle());
     }
+
     return $ret;
   }
 

src/applications/files/storage/__tests__/PhabricatorFileTestCase.php

@@ -145,4 +145,107 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase {
     $this->assertEqual($ttl, $file->getTTL());
   }
 
+  public function testFileTransformDelete() {
+    // We want to test that a file deletes all its inbound transformation
+    // records and outbound transformed derivatives when it is deleted.
+
+    // First, we create a chain of transforms, A -> B -> C.
+
+    $engine = new PhabricatorTestStorageEngine();
+
+    $params = array(
+      'name' => 'test.txt',
+      'storageEngines' => array(
+        $engine,
+      ),
+    );
+
+    $a = PhabricatorFile::newFromFileData('a', $params);
+    $b = PhabricatorFile::newFromFileData('b', $params);
+    $c = PhabricatorFile::newFromFileData('c', $params);
+
+    id(new PhabricatorTransformedFile())
+      ->setOriginalPHID($a->getPHID())
+      ->setTransform('test:a->b')
+      ->setTransformedPHID($b->getPHID())
+      ->save();
+
+    id(new PhabricatorTransformedFile())
+      ->setOriginalPHID($b->getPHID())
+      ->setTransform('test:b->c')
+      ->setTransformedPHID($c->getPHID())
+      ->save();
+
+    // Now, verify that A -> B and B -> C exist.
+
+    $xform_a = id(new PhabricatorFileQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withTransforms(
+        array(
+          array(
+            'originalPHID' => $a->getPHID(),
+            'transform'    => true,
+          ),
+        ))
+      ->execute();
+
+    $this->assertEqual(1, count($xform_a));
+    $this->assertEqual($b->getPHID(), head($xform_a)->getPHID());
+
+    $xform_b = id(new PhabricatorFileQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withTransforms(
+        array(
+          array(
+            'originalPHID' => $b->getPHID(),
+            'transform'    => true,
+          ),
+        ))
+      ->execute();
+
+    $this->assertEqual(1, count($xform_b));
+    $this->assertEqual($c->getPHID(), head($xform_b)->getPHID());
+
+    // Delete "B".
+
+    $b->delete();
+
+    // Now, verify that the A -> B and B -> C links are gone.
+
+    $xform_a = id(new PhabricatorFileQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withTransforms(
+        array(
+          array(
+            'originalPHID' => $a->getPHID(),
+            'transform'    => true,
+          ),
+        ))
+      ->execute();
+
+    $this->assertEqual(0, count($xform_a));
+
+    $xform_b = id(new PhabricatorFileQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withTransforms(
+        array(
+          array(
+            'originalPHID' => $b->getPHID(),
+            'transform'    => true,
+          ),
+        ))
+      ->execute();
+
+    $this->assertEqual(0, count($xform_b));
+
+    // Also verify that C has been deleted.
+
+    $alternate_c = id(new PhabricatorFileQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withPHIDs(array($c->getPHID()))
+      ->execute();
+
+    $this->assertEqual(array(), $alternate_c);
+  }
+
 }