Provide connection isolation to Lisk and enable it by default in tests

Summary:
Allow Lisk to be put into process-isolated mode which establishes
only isolated connections. By default, put it into this mode when running
unit tests. Build some simple unit tests around object insertion and
updating.

NOTE: The one flaw in this is that $dao->establishConnection() still
punches through the isolation layer. I need to do an API change to fix this
though so I'm holding it for now. It will probably just rename getConnection()
to establishConnection() and then rename establishConnection() to something
scary like establishLiveExternalConnection().

Test Plan:
Ran unit tests.

Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 194

src/__phutil_library_map__.php

@@ -240,6 +240,9 @@ phutil_register_library_map(array(
     'HeraldValueTypeConfig' => 'applications/herald/config/valuetype',
     'Javelin' => 'infrastructure/javelin/api',
     'LiskDAO' => 'storage/lisk/dao',
+    'LiskIsolationTestCase' => 'storage/lisk/dao/__tests__',
+    'LiskIsolationTestDAO' => 'storage/lisk/dao/__tests__',
+    'LiskIsolationTestDAOException' => 'storage/lisk/dao/__tests__',
     'ManiphestController' => 'applications/maniphest/controller/base',
     'ManiphestDAO' => 'applications/maniphest/storage/base',
     'ManiphestTask' => 'applications/maniphest/storage/task',
@@ -644,6 +647,8 @@ phutil_register_library_map(array(
     'HeraldTranscript' => 'HeraldDAO',
     'HeraldTranscriptController' => 'HeraldController',
     'HeraldTranscriptListController' => 'HeraldController',
+    'LiskIsolationTestCase' => 'PhabricatorTestCase',
+    'LiskIsolationTestDAO' => 'LiskDAO',
     'ManiphestController' => 'PhabricatorController',
     'ManiphestDAO' => 'PhabricatorLiskDAO',
     'ManiphestTask' => 'ManiphestDAO',

src/infrastructure/testing/testcase/PhabricatorTestCase.php

@@ -18,9 +18,47 @@
 
 abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
 
+
+  /**
+   * If true, put Lisk in process-isolated mode for the duration of the tests so
+   * that it will establish only isolated, side-effect-free database
+   * connections. Defaults to true.
+   *
+   * NOTE: You should disable this only in rare circumstances. Unit tests should
+   * not rely on external resources like databases, and should not produce
+   * side effects.
+   */
+  const PHABRICATOR_TESTCONFIG_ISOLATE_LISK = 'isolate-lisk';
+
+  private $configuration;
+
+  protected function getPhabricatorTestCaseConfiguration() {
+    return array();
+  }
+
+  private function getComputedConfiguration() {
+    return $this->getPhabricatorTestCaseConfiguration() + array(
+      self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK => true,
+    );
+  }
+
   protected function willRunTests() {
     $root = dirname(phutil_get_library_root('phabricator'));
     require_once $root.'/scripts/__init_env__.php';
+
+    $config = $this->getComputedConfiguration();
+
+    if ($config[self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK]) {
+      LiskDAO::beginIsolateAllLiskEffectsToCurrentProcess();
+    }
+  }
+
+  protected function didRunTests() {
+    $config = $this->getComputedConfiguration();
+
+    if ($config[self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK]) {
+      LiskDAO::endIsolateAllLiskEffectsToCurrentProcess();
+    }
   }
 
 }

src/infrastructure/testing/testcase/__init__.php

@@ -8,6 +8,8 @@
 
 phutil_require_module('arcanist', 'unit/engine/phutil/testcase');
 
+phutil_require_module('phabricator', 'storage/lisk/dao');
+
 phutil_require_module('phutil', 'moduleutils');
 
 

src/storage/connection/isolated/__tests__/AphrontIsolatedDatabaseConnectionTestCase.php

@@ -19,6 +19,15 @@
 class AphrontIsolatedDatabaseConnectionTestCase
   extends PhabricatorTestCase {
 
+  protected function getPhabricatorTestCaseConfiguration() {
+    return array(
+      // We disable this here because this test is unique (it is testing that
+      // isolation actually occurs) and must establish a live connection to the
+      // database to verify that.
+      self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK => false,
+    );
+  }
+
   public function testIsolation() {
     $conn = $this->newIsolatedConnection();
 

src/storage/lisk/dao/LiskDAO.php

@@ -140,6 +140,7 @@
  * @task   save    Writing Objects
  * @task   hook    Hooks and Callbacks
  * @task   util    Utilities
+ * @task   isolate Isolation for Unit Testing
  *
  * @group storage
  */
@@ -160,6 +161,7 @@ abstract class LiskDAO {
   const IDS_MANUAL                  = 'ids-manual';
 
   private $__connections            = array();
+  private static $processIsolationLevel = 0;
 
   /**
    *  Build an empty object.
@@ -603,6 +605,14 @@ abstract class LiskDAO {
       throw new Exception("Unknown mode '{$mode}', should be 'r' or 'w'.");
     }
 
+    if (self::shouldIsolateAllLiskEffectsToCurrentProcess()) {
+      $mode = 'isolate-'.$mode;
+      if (!isset($this->__connections[$mode])) {
+        $this->__connections[$mode] = $this->establishIsolatedConnection($mode);
+      }
+      return $this->__connections[$mode];
+    }
+
     // TODO There is currently no protection on 'r' queries against writing
     // or on 'w' queries against reading
 
@@ -1046,6 +1056,43 @@ abstract class LiskDAO {
    */
   protected function didDelete() {}
 
+
+/* -(  Isolation  )---------------------------------------------------------- */
+
+  /**
+   * @task isolate
+   */
+  public static function beginIsolateAllLiskEffectsToCurrentProcess() {
+    self::$processIsolationLevel++;
+  }
+
+  /**
+   * @task isolate
+   */
+  public static function endIsolateAllLiskEffectsToCurrentProcess() {
+    self::$processIsolationLevel--;
+    if (self::$processIsolationLevel < 0) {
+      throw new Exception(
+        "Lisk process isolation level was reduced below 0.");
+    }
+  }
+
+  /**
+   * @task isolate
+   */
+  public static function shouldIsolateAllLiskEffectsToCurrentProcess() {
+    return (bool)self::$processIsolationLevel;
+  }
+
+  /**
+   * @task isolate
+   */
+  private function establishIsolatedConnection($mode) {
+    $config = array();
+    return new AphrontIsolatedDatabaseConnection($config);
+  }
+
+
 /* -(  Utilities  )---------------------------------------------------------- */
 
 

src/storage/lisk/dao/__init__.php

@@ -6,6 +6,7 @@
 
 
 
+phutil_require_module('phabricator', 'storage/connection/isolated');
 phutil_require_module('phabricator', 'storage/exception/count');
 phutil_require_module('phabricator', 'storage/exception/objectmissing');
 phutil_require_module('phabricator', 'storage/qsprintf');

src/storage/lisk/dao/__tests__/LiskIsolationTestCase.php

@@ -0,0 +1,56 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class LiskIsolationTestCase extends PhabricatorTestCase {
+
+  public function testIsolatedWrites() {
+    $dao = new LiskIsolationTestDAO();
+
+    $this->assertEqual(null, $dao->getID(), 'Expect no ID.');
+    $this->assertEqual(null, $dao->getPHID(), 'Expect no PHID.');
+
+    $dao->save(); // Effects insert
+
+    $id = $dao->getID();
+    $phid = $dao->getPHID();
+
+    $this->assertEqual(true, (bool)$id, 'Expect ID generated.');
+    $this->assertEqual(true, (bool)$phid, 'Expect PHID generated.');
+
+    $dao->save(); // Effects update
+
+    $this->assertEqual($id, $dao->getID(), 'Expect ID unchanged.');
+    $this->assertEqual($phid, $dao->getPHID(), 'Expect PHID unchanged.');
+  }
+
+  public function testIsolationContainment() {
+    $dao = new LiskIsolationTestDAO();
+
+    try {
+      $dao->establishConnection('r');
+
+      $this->assertFailure(
+        "LiskIsolationTestDAO did not throw an exception when instructed to ".
+        "explicitly connect to an external database.");
+    } catch (LiskIsolationTestDAOException $ex) {
+      // Expected, pass.
+    }
+
+  }
+
+}

src/storage/lisk/dao/__tests__/LiskIsolationTestDAO.php

@@ -0,0 +1,44 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class LiskIsolationTestDAO extends LiskDAO {
+
+  protected $name;
+  protected $phid;
+
+  public function getConfiguration() {
+    return array(
+      self::CONFIG_AUX_PHID => true,
+    ) + parent::getConfiguration();
+  }
+
+  public function generatePHID() {
+    return PhabricatorPHID::generateNewPHID('TISO');
+  }
+
+  public function establishConnection($mode) {
+    throw new LiskIsolationTestDAOException(
+      "Isolation failure! DAO is attempting to connect to an external ".
+      "resource!");
+  }
+
+  public function getTableName() {
+    return 'test';
+  }
+
+}

src/storage/lisk/dao/__tests__/LiskIsolationTestDAOException.php

@@ -0,0 +1,19 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class LiskIsolationTestDAOException extends Exception { }

src/storage/lisk/dao/__tests__/__init__.php

@@ -0,0 +1,16 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/phid/storage/phid');
+phutil_require_module('phabricator', 'infrastructure/testing/testcase');
+phutil_require_module('phabricator', 'storage/lisk/dao');
+
+
+phutil_require_source('LiskIsolationTestCase.php');
+phutil_require_source('LiskIsolationTestDAO.php');
+phutil_require_source('LiskIsolationTestDAOException.php');