Allow bin/storage adjust to correct column types and collations

Summary:
Ref T1191. Allow `bin/storage adjust` to modify columns.

  - Although `CREATE TABLE ... colname VARCHAR(64) CHARACTER SET BINARY` works fine, it's actually a trick. Adjust the binary columns for this.

Test Plan: See comment.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6130, T6128, T6135, T6137, T6138, T6149, T6151, T1191

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

src/applications/config/schema/PhabricatorConfigSchemaSpec.php

@@ -230,6 +230,10 @@ abstract class PhabricatorConfigSchemaSpec extends Phobject {
       $data_type = substr($data_type, 0, -1);
     }
 
+    // NOTE: MySQL allows fragments like "VARCHAR(32) CHARACTER SET binary",
+    // but just interprets that to mean "VARBINARY(32)". The fragment is
+    // totally disallowed in a MODIFY statement vs a CREATE TABLE statement.
+
     switch ($data_type) {
       case 'id':
       case 'epoch':
@@ -248,39 +252,25 @@ abstract class PhabricatorConfigSchemaSpec extends Phobject {
         break;
       case 'phid':
       case 'policy';
-        $column_type = 'varchar(64)';
+        $column_type = 'varbinary(64)';
-        $charset = 'binary';
-        $collation = 'binary';
         break;
       case 'bytes64':
-        $column_type = 'char(64)';
+        $column_type = 'binary(64)';
-        $charset = 'binary';
-        $collation = 'binary';
         break;
       case 'bytes40':
-        $column_type = 'char(40)';
+        $column_type = 'binary(40)';
-        $charset = 'binary';
-        $collation = 'binary';
         break;
       case 'bytes32':
-        $column_type = 'char(32)';
+        $column_type = 'binary(32)';
-        $charset = 'binary';
-        $collation = 'binary';
         break;
       case 'bytes20':
-        $column_type = 'char(20)';
+        $column_type = 'binary(20)';
-        $charset = 'binary';
-        $collation = 'binary';
         break;
       case 'bytes12':
-        $column_type = 'char(12)';
+        $column_type = 'binary(12)';
-        $charset = 'binary';
-        $collation = 'binary';
         break;
       case 'bytes4':
-        $column_type = 'char(4)';
+        $column_type = 'binary(4)';
-        $charset = 'binary';
-        $collation = 'binary';
         break;
       case 'bytes':
         $column_type = 'longblob';

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

@@ -127,38 +127,93 @@ final class PhabricatorStorageManagementAdjustWorkflow
     $api = $this->getAPI();
     $conn = $api->getConn(null);
 
+    $failed = array();
+
     $bar = id(new PhutilConsoleProgressBar())
       ->setTotal(count($adjustments));
     foreach ($adjustments as $adjust) {
-      switch ($adjust['kind']) {
+      try {
-        case 'database':
+        switch ($adjust['kind']) {
-          queryfx(
+          case 'database':
-            $conn,
+            queryfx(
-            'ALTER DATABASE %T CHARACTER SET = %s COLLATE = %s',
+              $conn,
-            $adjust['database'],
+              'ALTER DATABASE %T CHARACTER SET = %s COLLATE = %s',
-            $adjust['charset'],
+              $adjust['database'],
-            $adjust['collation']);
+              $adjust['charset'],
-          break;
+              $adjust['collation']);
-        case 'table':
+            break;
-          queryfx(
+          case 'table':
-            $conn,
+            queryfx(
-            'ALTER TABLE %T.%T COLLATE = %s',
+              $conn,
-            $adjust['database'],
+              'ALTER TABLE %T.%T COLLATE = %s',
-            $adjust['table'],
+              $adjust['database'],
-            $adjust['collation']);
+              $adjust['table'],
-          break;
+              $adjust['collation']);
-        default:
+            break;
-          throw new Exception(
+          case 'column':
-            pht('Unknown schema adjustment kind "%s"!', $adjust['kind']));
+            $parts = array();
-      }
+            if ($adjust['charset']) {
+              $parts[] = qsprintf(
+                $conn,
+                'CHARACTER SET %Q COLLATE %Q',
+                $adjust['charset'],
+                $adjust['collation']);
+            }
 
+            queryfx(
+              $conn,
+              'ALTER TABLE %T.%T MODIFY %T %Q %Q %Q',
+              $adjust['database'],
+              $adjust['table'],
+              $adjust['name'],
+              $adjust['type'],
+              implode(' ', $parts),
+              $adjust['nullable'] ? 'NULL' : 'NOT NULL');
+
+            break;
+          default:
+            throw new Exception(
+              pht('Unknown schema adjustment kind "%s"!', $adjust['kind']));
+        }
+      } catch (AphrontQueryException $ex) {
+        $failed[] = array($adjust, $ex);
+      }
       $bar->update(1);
     }
     $bar->done();
 
+    if (!$failed) {
+      $console->writeOut(
+        "%s\n",
+        pht('Completed fixing all schema issues.'));
+      return;
+    }
+
+    $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', 'naeme'));
+      $pieces = array_filter($pieces);
+      $target = implode('.', $pieces);
+
+      $table->addRow(
+        array(
+          'target' => $target,
+          'error' => $ex->getMessage(),
+        ));
+    }
+
+    $console->writeOut("\n");
+    $table->draw();
     $console->writeOut(
-      "%s\n",
+      "\n%s\n",
-      pht('Completed fixing all schema issues.'));
+      pht('Failed to make some schema adjustments, detailed above.'));
+
+    return 1;
   }
 
   private function findAdjustments() {
@@ -166,6 +221,7 @@ final class PhabricatorStorageManagementAdjustWorkflow
 
     $issue_charset = PhabricatorConfigStorageSchema::ISSUE_CHARSET;
     $issue_collation = PhabricatorConfigStorageSchema::ISSUE_COLLATION;
+    $issue_columntype = PhabricatorConfigStorageSchema::ISSUE_COLUMNTYPE;
 
     $adjustments = array();
     foreach ($comp->getDatabases() as $database_name => $database) {
@@ -217,6 +273,54 @@ final class PhabricatorStorageManagementAdjustWorkflow
             'collation' => $expect_table->getCollation(),
           );
         }
+
+        foreach ($table->getColumns() as $column_name => $column) {
+          $expect_column = $expect_table->getColumn($column_name);
+          $actual_column = $actual_table->getColumn($column_name);
+
+          if (!$expect_column || !$actual_column) {
+            continue;
+          }
+
+          $issues = array();
+          if ($column->hasIssue($issue_collation)) {
+            $issues[] = $issue_collation;
+          }
+          if ($column->hasIssue($issue_charset)) {
+            $issues[] = $issue_charset;
+          }
+          if ($column->hasIssue($issue_columntype)) {
+            $issues[] = $issue_columntype;
+          }
+
+          if ($issues) {
+            if ($expect_column->getCharacterSet() === null) {
+              // For non-text columns, we won't be specifying a collation or
+              // character set.
+              $charset = null;
+              $collation = null;
+            } else {
+              $charset = $expect_column->getCharacterSet();
+              $collation = $expect_column->getCollation();
+            }
+
+
+            $adjustments[] = array(
+              'kind' => 'column',
+              'database' => $database_name,
+              'table' => $table_name,
+              'name' => $column_name,
+              'issues' => $issues,
+              'collation' => $collation,
+              'charset' => $charset,
+              'type' => $expect_column->getColumnType(),
+
+              // NOTE: We don't adjust column nullability because it is
+              // dangerous, so always use the current nullability.
+              'nullable' => $actual_column->getNullable(),
+            );
+          }
+        }
       }
     }