FIXED: Repaired failing test cases (DataObjectSchemaGenerationTest)

FIXED: Incorrect implementation of alterIndex
UPDATED: Improved parsing of index specification by including code borrowed from the postgres database connector module. Possibly a candidate for refactor into SS_Database.

code/MSSQLDatabase.php

@@ -587,8 +587,7 @@ class MSSQLDatabase extends SS_Database {
 			$bits = preg_split('/ *= */', $segment);
 			for($i = 1; $i < sizeof($bits); $i += 2) {
 				array_unshift($constraints, substr(rtrim($bits[$i], ')'), 1, -1));
-
+			}
-		}
 		}
 		return $constraints;
 	}
@@ -688,8 +687,8 @@ class MSSQLDatabase extends SS_Database {
 	 * @param string $newName The new name of the field
 	 */
 	public function renameField($tableName, $oldName, $newName) {
-			$this->query("EXEC sp_rename @objname = '$tableName.$oldName', @newname = '$newName', @objtype = 'COLUMN'");
+		$this->query("EXEC sp_rename @objname = '$tableName.$oldName', @newname = '$newName', @objtype = 'COLUMN'");
-		}
+	}
 
 	public function fieldList($table) {
 		//This gets us more information than we need, but I've included it all for the moment....
@@ -815,56 +814,146 @@ class MSSQLDatabase extends SS_Database {
 	 * arrays to be created.
 	 */
 	public function convertIndexSpec($indexSpec){
-		if(is_array($indexSpec)){
+		$indexSpec = $this->parseIndexSpec(null, $indexSpec);
-			//Here we create a db-specific version of whatever index we need to create.
+		return "{$indexSpec['type']} ({$indexSpec['value']})";
-			switch($indexSpec['type']){
+	}
-				case 'fulltext':
+	
-					$indexSpec='fulltext (' . str_replace(' ', '', $indexSpec['value']) . ')';
+	/**
-					break;
+	 * Splits a spec string safely, considering quoted columns, whitespace, 
-				case 'unique':
+	 * and cleaning brackets
-					$indexSpec='unique (' . $indexSpec['value'] . ')';
+	 * @param string $spec The input index specification
-					break;
+	 * @return array List of columns in the spec
-			}
+	 */
+	function explodeColumnString($spec) {
+		// Remove any leading/trailing brackets and outlying modifiers
+		// E.g. 'unique (Title, "QuotedColumn");' => 'Title, "QuotedColumn"'
+		$containedSpec = preg_replace('/(.*\(\s*)|(\s*\).*)/', '', $spec);
+		
+		// Split potentially quoted modifiers
+		// E.g. 'Title, "QuotedColumn"' => array('Title', 'QuotedColumn')
+		return preg_split('/"?\s*,\s*"?/', trim($containedSpec, '(") '));
+	}
+	
+	/**
+	 * Builds a properly quoted column list from an array
+	 * @param array $columns List of columns to implode
+	 * @return string A properly quoted list of column names
+	 */
+	function implodeColumnList($columns) {
+		if(empty($columns)) return '';
+		return '"' . implode('","', $columns) . '"';
+	}
+	
+	/**
+	 * Given an index specification in the form of a string ensure that each
+	 * column name is property quoted, stripping brackets and modifiers.
+	 * This index may also be in the form of a "CREATE INDEX..." sql fragment
+	 * @param string $spec The input specification or query. E.g. 'unique (Column1, Column2)'
+	 * @return string The properly quoted column list. E.g. '"Column1", "Column2"'
+	 */
+	function quoteColumnSpecString($spec) {
+		$bits = $this->explodeColumnString($spec);
+		return $this->implodeColumnList($bits);
+	}
+	
+	/**
+	 * Given an index spec determines the index type
+	 * @param type $spec
+	 * @return string 
+	 */
+	function determineIndexType($spec) {
+		// check array spec
+		if(is_array($spec) && isset($spec['type'])) {
+			return $spec['type'];
+		} elseif (!is_array($spec) && preg_match('/(?<type>\w+)\s*\(/', $spec, $matchType)) {
+			return strtolower($matchType['type']);
+		} else {
+			return 'index';
 		}
+	}
+	
+	/**
+	 * Converts an array or string index spec into a universally useful array
+	 * @see convertIndexSpec() for approximate inverse
+	 * @param string|array $spec An index specification in either one of two formats:
+	 * <ul>
+	 * <li>A string in the format <code>"indextype (Column1, Column2, Column3)"</code> indextype
+	 *   may be omitted, in which case it will be treated as an 'index' type</li>
+	 * <li>An associative array with the keys 'name' (which should max the $name
+	 *   argument), 'value' (in the same format as the above column list) and
+	 *   'type'.</li>
+	 * </ul>
+	 * Index types may be one of unique, index, fulltext, or any adaptor specific
+	 * format. Column names may be double quoted.
+	 * @return array The resulting spec array with the required fields 'name', 
+	 * 'type', and 'value'. The index value will have cleanly quoted column names.
+	 * E.g. <code>array(
+	 *	'type' => 'unique', 
+	 *  'name' => 'MyIndex', 
+	 *	'value' => '"Title", "Content"'
+	 * )</code>
+	 */
+	function parseIndexSpec($name, $spec) {
+		
+		// Do minimal cleanup on any already parsed spec
+		if(is_array($spec)) {
+			$spec['value'] = $this->quoteColumnSpecString($spec['value']);
+			return $spec;
+		}
+		
+		// Nicely formatted spec!
+		return array(
+			'name' => $name,
+			'value' => $this->quoteColumnSpecString($spec),
+			'type' => $this->determineIndexType($spec)
+		);
+	}
 
-		return $indexSpec;
+	/**
+	 * Builds the internal MS SQL Server index name given the silverstripe table and index name
+	 * @param string $tableName
+	 * @param string $indexName 
+	 * @param string $prefix The optional prefix for the index. Defaults to "ix" for indexes.
+	 * @return string The postgres name of the index
+	 */
+	function buildMSSQLIndexName($tableName, $indexName, $prefix = 'ix') {
+		
+		// Cleanup names of namespaced tables
+		$tableName = str_replace('\\', '_', $tableName);
+		
+		return "{$prefix}_{$tableName}_{$indexName}";
 	}
 
 	/**
 	 * Return SQL for dropping and recreating an index
 	 */
 	protected function getIndexSqlDefinition($tableName, $indexName, $indexSpec) {
-		$index = 'ix_' . str_replace('\\', '_', $tableName) . '_' . $indexName;
+		
+		// Determine index name
+		$index = $this->buildMSSQLIndexName($tableName, $indexName);
+
+		// Consolidate/Cleanup spec into array format
+		$indexSpec = $this->parseIndexSpec($indexName, $indexSpec);
+		
 		$drop = "IF EXISTS (SELECT name FROM sys.indexes WHERE name = '$index') DROP INDEX $index ON \"" . $tableName . "\";";
 		
-		if(!is_array($indexSpec)) {
+		// create a type-specific index
-			$indexSpec=trim($indexSpec, '()');
+		if($indexSpec['type'] == 'fulltext' && $this->fullTextEnabled()) {
-			return "$drop CREATE INDEX $index ON \"" . $tableName . "\" (" . $indexSpec . ");";
+			// enable fulltext on this table
-		} else {
+			$this->createFullTextCatalog();
-			// create a type-specific index
+			$primary_key = $this->getPrimaryKey($tableName);
-			if($indexSpec['type'] == 'fulltext') {
-				if($this->fullTextEnabled()) {
-					// enable fulltext on this table
-					$this->createFullTextCatalog();
-					$primary_key = $this->getPrimaryKey($tableName);
 
-					$query = '';
+			if($primary_key) {
-					if($primary_key) {
+				return "$drop CREATE FULLTEXT INDEX ON \"$tableName\" ({$indexSpec['value']})"
-						$query .= "CREATE FULLTEXT INDEX ON \"$tableName\" ({$indexSpec['value']}) KEY INDEX $primary_key WITH CHANGE_TRACKING AUTO;";
+					 . "KEY INDEX $primary_key WITH CHANGE_TRACKING AUTO;";
-					}
-
-					return $query;
-				}
-			}
-
-			if($indexSpec['type'] == 'unique') {
-				if(!is_array($indexSpec['value'])) $columns = preg_split('/ *, */', trim($indexSpec['value']));
-				else $columns = $indexSpec['value'];
-				$SQL_columnList = implode(', ', $columns);
-
-				return "$drop CREATE UNIQUE INDEX $index ON \"" . $tableName . "\" ($SQL_columnList);";
 			}
 		}
+
+		if($indexSpec['type'] == 'unique') {
+			return "$drop CREATE UNIQUE INDEX $index ON \"$tableName\" ({$indexSpec['value']});";
+		}
+
+		return "$drop CREATE INDEX $index ON \"$tableName\" ({$indexSpec['value']});";
 	}
 
 	function getDbSqlDefinition($tableName, $indexName, $indexSpec){
@@ -878,19 +967,7 @@ class MSSQLDatabase extends SS_Database {
 	 * @param string $indexSpec The specification of the index, see SS_Database::requireIndex() for more details.
 	 */
 	public function alterIndex($tableName, $indexName, $indexSpec) {
-	    $indexSpec = trim($indexSpec);
+		$this->createIndex($tableName, $indexName, $indexSpec);
-	    if($indexSpec[0] != '(') {
-	    	list($indexType, $indexFields) = explode(' ',$indexSpec,2);
-	    } else {
-	    	$indexFields = $indexSpec;
-	    }
-
-	    if(!$indexType) {
-	    	$indexType = "index";
-	    }
-
-    	$this->query("DROP INDEX $indexName ON $tableName;");
-		$this->query("ALTER TABLE \"$tableName\" ADD $indexType \"$indexName\" $indexFields");
 	}
 
 	/**
@@ -900,33 +977,43 @@ class MSSQLDatabase extends SS_Database {
 	 */
 	public function indexList($table) {
 		$indexes = DB::query("EXEC sp_helpindex '$table';");
-		$prefix = '';
 		$indexList = array();
-
+		
+		// Enumerate all basic indexes
 		foreach($indexes as $index) {
 			if(strpos($index['index_description'], 'unique') !== false) {
-				$prefix='unique ';
+				$prefix = 'unique ';
+			} else {
+				$prefix= 'index ';
 			}
+			
+			// Extract name from index
+			$baseIndexName = $this->buildMSSQLIndexName($table, '');
+			$indexName = substr($index['index_name'], strlen($baseIndexName));
+			
+			// Extract columns
+			$columns = $this->quoteColumnSpecString($index['index_keys']);
 
-			$key = str_replace(', ', ',', $index['index_keys']);
+			$indexList[$indexName]['indexname'] = $indexName;
-			$indexList[$key]['indexname'] = $index['index_name'];
+			$indexList[$indexName]['spec'] = "$prefix($columns)";
-			$indexList[$key]['spec'] = $prefix . '(' . $key . ')';
 		}
 
 		// Now we need to check to see if we have any fulltext indexes attached to this table:
 		if($this->fullTextEnabled()) {
 			$result = DB::query('EXEC sp_help_fulltext_columns;');
-			$columns = '';
+			
+			// Extract columns from this fulltext definition
+			$columns = array();
 			foreach($result as $row) {
 				if($row['TABLE_NAME'] == $table) {
-					$columns .= $row['FULLTEXT_COLUMN_NAME'] . ',';
+					$columns[] = $row['FULLTEXT_COLUMN_NAME'];
 				}
 			}
 
-			if($columns!=''){
+			if(!empty($columns)) {
-				$columns=trim($columns, ',');
+				$columns = $this->implodeColumnList($columns);
 				$indexList['SearchFields']['indexname'] = 'SearchFields';
-				$indexList['SearchFields']['spec'] = 'fulltext (' . $columns . ')';
+				$indexList['SearchFields']['spec'] = "fulltext ($columns)";
 			}
 		}
 
@@ -1282,10 +1369,9 @@ class MSSQLDatabase extends SS_Database {
 
 	/**
 	 * This changes the index name depending on database requirements.
-	 * MSSQL requires underscores to be replaced with commas.
 	 */
 	function modifyIndex($index) {
-		return str_replace('_', ',', $index);
+		return $index;
 	}
 
 	/**