Do a better job of handling spec errors during schema adjustment

Summary:
Ref T1191. Currently if a developer forgot to specify a column type, `storage adjust` aborts explosively mid-stream. Instead:

  - Make this a formal error with an unambiugous name/description instead of something you sort of infer by seeing "<unknown>".
  - Make this error prevent generation of adjustment warnings, so we don't try to `ALTER TABLE t CHANGE COLUMN c <unknown>`, which is nonsense.
  - When schemata errors exist, surface them prominiently in `storage adjust`.

Overall:

  - Once `storage upgrade` runs `storage adjust` automatically (soon), this will make it relatively difficult to miss these errors.
  - Letting these errors slip through no longer escalates into a more severe issue.

Test Plan:
Commented out the recent `mailKey` spec and ran `storage adjust`:

```
$ ./bin/storage adjust --force
Verifying database schemata...
Found no adjustments for schemata.

Target                                            Error
phabricator2_phriction.phriction_document.mailKey Column Has No Specification

 SCHEMATA ERRORS

The schemata have serious errors (detailed above) which the adjustment
workflow can not fix.

If you are not developing Phabricator itself, report this issue to the
upstream.

If you are developing Phabricator, these errors usually indicate that your
schema specifications do not agree with the schemata your code actually
builds.
```

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1191

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

src/applications/config/schema/PhabricatorConfigColumnSchema.php

@@ -125,24 +125,30 @@ final class PhabricatorConfigColumnSchema
     PhabricatorConfigStorageSchema $expect) {
 
     $issues = array();
-    if ($this->getCharacterSet() != $expect->getCharacterSet()) {
-      $issues[] = self::ISSUE_CHARSET;
-    }
 
-    if ($this->getCollation() != $expect->getCollation()) {
+    $type_unknown = PhabricatorConfigSchemaSpec::DATATYPE_UNKNOWN;
-      $issues[] = self::ISSUE_COLLATION;
+    if ($expect->getColumnType() == $type_unknown) {
-    }
+      $issues[] = self::ISSUE_UNKNOWN;
+    } else {
+      if ($this->getCharacterSet() != $expect->getCharacterSet()) {
+        $issues[] = self::ISSUE_CHARSET;
+      }
 
-    if ($this->getColumnType() != $expect->getColumnType()) {
+      if ($this->getCollation() != $expect->getCollation()) {
-      $issues[] = self::ISSUE_COLUMNTYPE;
+        $issues[] = self::ISSUE_COLLATION;
-    }
+      }
 
-    if ($this->getNullable() !== $expect->getNullable()) {
+      if ($this->getColumnType() != $expect->getColumnType()) {
-      $issues[] = self::ISSUE_NULLABLE;
+        $issues[] = self::ISSUE_COLUMNTYPE;
-    }
+      }
 
-    if ($this->getAutoIncrement() !== $expect->getAutoIncrement()) {
+      if ($this->getNullable() !== $expect->getNullable()) {
-      $issues[] = self::ISSUE_AUTOINCREMENT;
+        $issues[] = self::ISSUE_NULLABLE;
+      }
+
+      if ($this->getAutoIncrement() !== $expect->getAutoIncrement()) {
+        $issues[] = self::ISSUE_AUTOINCREMENT;
+      }
     }
 
     return $issues;

src/applications/config/schema/PhabricatorConfigSchemaSpec.php

@@ -7,6 +7,8 @@ abstract class PhabricatorConfigSchemaSpec extends Phobject {
   private $utf8BinaryCollation;
   private $utf8SortingCollation;
 
+  const DATATYPE_UNKNOWN = '<unknown>';
+
   public function setUTF8SortingCollation($utf8_sorting_collation) {
     $this->utf8SortingCollation = $utf8_sorting_collation;
     return $this;
@@ -324,9 +326,9 @@ abstract class PhabricatorConfigSchemaSpec extends Phobject {
           $column_type = 'date';
           break;
         default:
-          $column_type = pht('<unknown>');
+          $column_type = self::DATATYPE_UNKNOWN;
-          $charset = pht('<unknown>');
+          $charset = self::DATATYPE_UNKNOWN;
-          $collation = pht('<unknown>');
+          $collation = self::DATATYPE_UNKNOWN;
           break;
       }
     }

src/applications/config/schema/PhabricatorConfigStorageSchema.php

@@ -16,6 +16,7 @@ abstract class PhabricatorConfigStorageSchema extends Phobject {
   const ISSUE_SUBWARN = 'subwarn';
   const ISSUE_SUBFAIL = 'subfail';
   const ISSUE_AUTOINCREMENT = 'autoincrement';
+  const ISSUE_UNKNOWN = 'unknown';
 
   const STATUS_OKAY = 'okay';
   const STATUS_WARN = 'warn';
@@ -127,6 +128,8 @@ abstract class PhabricatorConfigStorageSchema extends Phobject {
         return pht('Subschemata Have Failures');
       case self::ISSUE_AUTOINCREMENT:
         return pht('Column has Wrong Autoincrement');
+      case self::ISSUE_UNKNOWN:
+        return pht('Column Has No Specification');
       default:
         throw new Exception(pht('Unknown schema issue "%s"!', $issue));
     }
@@ -162,6 +165,8 @@ abstract class PhabricatorConfigStorageSchema extends Phobject {
         return pht('Subschemata have setup failures.');
       case self::ISSUE_AUTOINCREMENT:
         return pht('This column has the wrong autoincrement setting.');
+      case self::ISSUE_UNKNOWN:
+        return pht('This column is missing a type specification.');
       default:
         throw new Exception(pht('Unknown schema issue "%s"!', $issue));
     }
@@ -173,6 +178,7 @@ abstract class PhabricatorConfigStorageSchema extends Phobject {
       case self::ISSUE_SURPLUS:
       case self::ISSUE_NULLABLE:
       case self::ISSUE_SUBFAIL:
+      case self::ISSUE_UNKNOWN:
         return self::STATUS_FAIL;
       case self::ISSUE_SUBWARN:
       case self::ISSUE_COLUMNTYPE:

src/infrastructure/storage/lisk/LiskDAO.php

@@ -1804,7 +1804,7 @@ abstract class LiskDAO {
       }
 
       // We don't know the type of this column.
-      $map[$property] = '<unknown>';
+      $map[$property] = PhabricatorConfigSchemaSpec::DATATYPE_UNKNOWN;
     }
 
     return $map;

src/infrastructure/storage/management/workflow/PhabricatorStorageManagementAdjustWorkflow.php

@@ -78,14 +78,15 @@ final class PhabricatorStorageManagementAdjustWorkflow
       "%s\n",
       pht('Verifying database schemata...'));
 
-    $adjustments = $this->findAdjustments();
+    list($adjustments, $errors) = $this->findAdjustments();
     $api = $this->getAPI();
 
     if (!$adjustments) {
       $console->writeOut(
         "%s\n",
-        pht('Found no issues with schemata.'));
+        pht('Found no adjustments for schemata.'));
-      return;
+
+      return $this->printErrors($errors, 0);
     }
 
     if (!$force && !$api->isCharacterSetAvailable('utf8mb4')) {
@@ -343,39 +344,44 @@ final class PhabricatorStorageManagementAdjustWorkflow
       $console->writeOut(
         "%s\n",
         pht('Completed fixing all schema issues.'));
-      return 0;
+
+      $err = 0;
+    } else {
+      $table = id(new PhutilConsoleTable())
+        ->addColumn('target', array('title' => pht('Target')))
+        ->addColumn('error', array('title' => pht('Error')));
+
+      foreach ($failed as $failure) {
+        list($adjust, $ex) = $failure;
+
+        $pieces = array_select_keys(
+          $adjust,
+          array('database', 'table', 'name'));
+        $pieces = array_filter($pieces);
+        $target = implode('.', $pieces);
+
+        $table->addRow(
+          array(
+            'target' => $target,
+            'error' => $ex->getMessage(),
+          ));
+      }
+
+      $console->writeOut("\n");
+      $table->draw();
+      $console->writeOut(
+        "\n%s\n",
+        pht('Failed to make some schema adjustments, detailed above.'));
+      $console->writeOut(
+        "%s\n",
+        pht(
+          'For help troubleshooting adjustments, see "Managing Storage '.
+          'Adjustments" in the documentation.'));
+
+      $err = 1;
     }
 
-    $table = id(new PhutilConsoleTable())
+    return $this->printErrors($errors, $err);
-      ->addColumn('target', array('title' => pht('Target')))
-      ->addColumn('error', array('title' => pht('Error')));
-
-    foreach ($failed as $failure) {
-      list($adjust, $ex) = $failure;
-
-      $pieces = array_select_keys($adjust, array('database', 'table', 'name'));
-      $pieces = array_filter($pieces);
-      $target = implode('.', $pieces);
-
-      $table->addRow(
-        array(
-          'target' => $target,
-          'error' => $ex->getMessage(),
-        ));
-    }
-
-    $console->writeOut("\n");
-    $table->draw();
-    $console->writeOut(
-      "\n%s\n",
-      pht('Failed to make some schema adjustments, detailed above.'));
-    $console->writeOut(
-      "%s\n",
-      pht(
-        'For help troubleshooting adjustments, see "Managing Storage '.
-        'Adjustments" in the documentation.'));
-
-    return 1;
   }
 
   private function findAdjustments() {
@@ -392,7 +398,15 @@ final class PhabricatorStorageManagementAdjustWorkflow
     $issue_auto = PhabricatorConfigStorageSchema::ISSUE_AUTOINCREMENT;
 
     $adjustments = array();
+    $errors = array();
     foreach ($comp->getDatabases() as $database_name => $database) {
+      foreach ($this->findErrors($database) as $issue) {
+        $errors[] = array(
+          'database' => $database_name,
+          'issue' => $issue,
+        );
+      }
+
       $expect_database = $expect->getDatabase($database_name);
       $actual_database = $actual->getDatabase($database_name);
 
@@ -420,6 +434,14 @@ final class PhabricatorStorageManagementAdjustWorkflow
       }
 
       foreach ($database->getTables() as $table_name => $table) {
+        foreach ($this->findErrors($table) as $issue) {
+          $errors[] = array(
+            'database' => $database_name,
+            'table' => $table_name,
+            'issue' => $issue,
+          );
+        }
+
         $expect_table = $expect_database->getTable($table_name);
         $actual_table = $actual_database->getTable($table_name);
 
@@ -443,6 +465,15 @@ final class PhabricatorStorageManagementAdjustWorkflow
         }
 
         foreach ($table->getColumns() as $column_name => $column) {
+          foreach ($this->findErrors($column) as $issue) {
+            $errors[] = array(
+              'database' => $database_name,
+              'table' => $table_name,
+              'name' => $column_name,
+              'issue' => $issue,
+            );
+          }
+
           $expect_column = $expect_table->getColumn($column_name);
           $actual_column = $actual_table->getColumn($column_name);
 
@@ -503,6 +534,15 @@ final class PhabricatorStorageManagementAdjustWorkflow
         }
 
         foreach ($table->getKeys() as $key_name => $key) {
+          foreach ($this->findErrors($key) as $issue) {
+            $errors[] = array(
+              'database' => $database_name,
+              'table' => $table_name,
+              'name' => $key_name,
+              'issue' => $issue,
+            );
+          }
+
           $expect_key = $expect_table->getKey($key_name);
           $actual_key = $actual_table->getKey($key_name);
 
@@ -560,8 +600,66 @@ final class PhabricatorStorageManagementAdjustWorkflow
       }
     }
 
-    return $adjustments;
+    return array($adjustments, $errors);
   }
 
+  private function findErrors(PhabricatorConfigStorageSchema $schema) {
+    $result = array();
+    foreach ($schema->getLocalIssues() as $issue) {
+      $status = PhabricatorConfigStorageSchema::getIssueStatus($issue);
+      if ($status == PhabricatorConfigStorageSchema::STATUS_FAIL) {
+        $result[] = $issue;
+      }
+    }
+    return $result;
+  }
+
+  private function printErrors(array $errors, $default_return) {
+    if (!$errors) {
+      return $default_return;
+    }
+
+    $console = PhutilConsole::getConsole();
+
+    $table = id(new PhutilConsoleTable())
+      ->addColumn('target', array('title' => pht('Target')))
+      ->addColumn('error', array('title' => pht('Error')));
+
+    foreach ($errors as $error) {
+      $pieces = array_select_keys(
+        $error,
+        array('database', 'table', 'name'));
+      $pieces = array_filter($pieces);
+      $target = implode('.', $pieces);
+
+      $name = PhabricatorConfigStorageSchema::getIssueName($error['issue']);
+
+      $table->addRow(
+        array(
+          'target' => $target,
+          'error' => $name,
+        ));
+    }
+
+    $console->writeOut("\n");
+    $table->draw();
+    $console->writeOut("\n");
+
+    $message = pht(
+      "The schemata have serious errors (detailed above) which the adjustment ".
+      "workflow can not fix.\n\n".
+      "If you are not developing Phabricator itself, report this issue to ".
+      "the upstream.\n\n".
+      "If you are developing Phabricator, these errors usually indicate that ".
+      "your schema specifications do not agree with the schemata your code ".
+      "actually builds.");
+
+    $console->writeOut(
+      "**<bg:red> %s </bg>**\n\n%s\n",
+      pht('SCHEMATA ERRORS'),
+      phutil_console_wrap($message));
+
+    return 2;
+  }
 
 }