Skip to content

Commit

Permalink
Merge branch 'm36_MDL-63319_MySQL_MSSQL_Rename_Field_Reserved_Word' of …
Browse files Browse the repository at this point in the history 
…https://github.com/scara/moodle
  • Loading branch information
@junpataleta
junpataleta committed Sep 18, 2018
2 parents 902ec3d + 211d04f commit 58e529e
Show file tree
Hide file tree
Showing 3 changed files with 137 additions and 4 deletions.
12 changes: 10 additions & 2 deletions lib/ddl/mssql_sql_generator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,16 @@ public function getRenameFieldSQL($xmldb_table, $xmldb_field, $newname) {
return array();
}

// Call to standard (parent) getRenameFieldSQL() function
$results = array_merge($results, parent::getRenameFieldSQL($xmldb_table, $xmldb_field, $newname));
// We can't call to standard (parent) getRenameFieldSQL() function since it would enclose the field name
// with improper quotes in MSSQL: here, we use a stored procedure to rename the field i.e. a column object;
// we need to take care about how this stored procedure expects parameters to be "qualified".
$rename = str_replace('TABLENAME', $this->getTableName($xmldb_table), $this->rename_column_sql);
// Qualifying the column object could require brackets: use them, regardless the column name not being a reserved word.
$rename = str_replace('OLDFIELDNAME', '[' . $xmldb_field->getName() . ']', $rename);
// The new field name should be passed as the actual name, w/o any quote.
$rename = str_replace('NEWFIELDNAME', $newname, $rename);

$results[] = $rename;

return $results;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/ddl/mysql_sql_generator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ public function getRenameFieldSQL($xmldb_table, $xmldb_field, $newname) {
$fieldsql = $this->getFieldSQL($xmldb_table, $xmldb_field_clone);

$sql = 'ALTER TABLE ' . $this->getTableName($xmldb_table) . ' CHANGE ' .
$xmldb_field->getName() . ' ' . $fieldsql;
$this->getEncQuoted($xmldb_field->getName()) . ' ' . $fieldsql;

return array($sql);
}
Expand Down
127 changes: 126 additions & 1 deletion lib/ddl/tests/ddl_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1545,12 +1545,45 @@ public function testRenameField() {
$field = new xmldb_field('type');
$field->set_attributes(XMLDB_TYPE_CHAR, '20', null, XMLDB_NOTNULL, null, 'general', 'course');

// 1. Rename the 'type' field into a generic new valid name.
// This represents the standard use case.
$dbman->rename_field($table, $field, 'newfieldname');

$columns = $DB->get_columns('test_table0');

$this->assertArrayNotHasKey('type', $columns);
$this->assertArrayHasKey('newfieldname', $columns);
$field->setName('newfieldname');

// 2. Rename the 'newfieldname' field into a reserved word, for testing purposes.
// This represents a questionable use case: we should support it but discourage the use of it on peer reviewing.
$dbman->rename_field($table, $field, 'where');

$columns = $DB->get_columns('test_table0');

$this->assertArrayNotHasKey('newfieldname', $columns);
$this->assertArrayHasKey('where', $columns);

// 3. Create a table with a column name named w/ a reserved word and get rid of it.
// This represents a "recovering" use case: a field name could be a reserved word in the future, at least for a DB type.
$table = new xmldb_table('test_table_res_word');
$table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
$table->add_field('where', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '0');
$table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
$table->setComment("This is a test'n drop table. You can drop it safely");
$dbman->create_table($table);
$dbman->table_exists('test_table_res_word');

$columns = $DB->get_columns('test_table_res_word');
$this->assertArrayHasKey('where', $columns);
$field = $table->getField('where');

$dbman->rename_field($table, $field, 'newfieldname');

$columns = $DB->get_columns('test_table_res_word');

$this->assertArrayNotHasKey('where', $columns);
$this->assertArrayHasKey('newfieldname', $columns);
}

public function testIndexExists() {
Expand Down Expand Up @@ -2168,7 +2201,7 @@ public function test_get_enc_quoted_provider() {
* This is a test for sql_generator::getEncQuoted().
*
* @dataProvider test_get_enc_quoted_provider
* @param string $reserved Whether the column name is reserved or not.
* @param bool $reserved Whether the column name is reserved or not.
* @param string $columnname The column name to be quoted, according to the value of $reserved.
**/
public function test_get_enc_quoted($reserved, $columnname) {
Expand Down Expand Up @@ -2197,6 +2230,98 @@ public function test_get_enc_quoted($reserved, $columnname) {
}
}

/**
* Data provider for test_sql_generator_get_rename_field_sql().
*
* @return array The type-old-new tuple fixture.
*/
public function test_sql_generator_get_rename_field_sql_provider() {
return array(
// Reserved: an example from SQL-92.
// Both names should be reserved.
[true, 'from', 'where'],
// Not reserved.
[false, 'my_old_column_name', 'my_awesome_column_name']
);
}

/**
* This is a unit test for sql_generator::getRenameFieldSQL().
*
* @dataProvider test_sql_generator_get_rename_field_sql_provider
* @param bool $reserved Whether the column name is reserved or not.
* @param string $oldcolumnname The column name to be renamed.
* @param string $newcolumnname The new column name.
**/
public function test_sql_generator_get_rename_field_sql($reserved, $oldcolumnname, $newcolumnname) {
$DB = $this->tdb;
$gen = $DB->get_manager()->generator;
$prefix = $DB->get_prefix();

$tablename = 'test_get_rename_field_sql';
$table = new xmldb_table($tablename);
$field = new xmldb_field($oldcolumnname, XMLDB_TYPE_INTEGER, '11', null, XMLDB_NOTNULL, null, null, null, '0', 'previous');

$dbfamily = $DB->get_dbfamily();
if (!$reserved) {
// No need to quote the column name.
switch ($dbfamily) {
case 'mysql':
$this->assertSame(
[ "ALTER TABLE {$prefix}$tablename CHANGE $oldcolumnname $newcolumnname BIGINT(11) NOT NULL" ],
$gen->getRenameFieldSQL($table, $field, $newcolumnname)
);
break;
case 'sqlite':
// Skip it, since the DB is not supported yet.
// BTW renaming a column name is already covered by the integration test 'testRenameField'.
break;
case 'mssql': // The Moodle connection runs under 'QUOTED_IDENTIFIER ON'.
$this->assertSame(
[ "sp_rename '{$prefix}$tablename.[$oldcolumnname]', '$newcolumnname', 'COLUMN'" ],
$gen->getRenameFieldSQL($table, $field, $newcolumnname)
);
break;
case 'oracle':
case 'postgres':
default:
$this->assertSame(
[ "ALTER TABLE {$prefix}$tablename RENAME COLUMN $oldcolumnname TO $newcolumnname" ],
$gen->getRenameFieldSQL($table, $field, $newcolumnname)
);
break;
}
} else {
// Column name should be quoted.
switch ($dbfamily) {
case 'mysql':
$this->assertSame(
[ "ALTER TABLE {$prefix}$tablename CHANGE `$oldcolumnname` `$newcolumnname` BIGINT(11) NOT NULL" ],
$gen->getRenameFieldSQL($table, $field, $newcolumnname)
);
break;
case 'sqlite':
// Skip it, since the DB is not supported yet.
// BTW renaming a column name is already covered by the integration test 'testRenameField'.
break;
case 'mssql': // The Moodle connection runs under 'QUOTED_IDENTIFIER ON'.
$this->assertSame(
[ "sp_rename '{$prefix}$tablename.[$oldcolumnname]', '$newcolumnname', 'COLUMN'" ],
$gen->getRenameFieldSQL($table, $field, $newcolumnname)
);
break;
case 'oracle':
case 'postgres':
default:
$this->assertSame(
[ "ALTER TABLE {$prefix}$tablename RENAME COLUMN \"$oldcolumnname\" TO \"$newcolumnname\"" ],
$gen->getRenameFieldSQL($table, $field, $newcolumnname)
);
break;
}
}
}

// Following methods are not supported == Do not test.
/*
public function testRenameIndex() {
Expand Down

0 comments on commit 58e529e

Please sign in to comment.