Jeroen De Dauw has submitted this change and it was merged. Change subject: Add escaping of identifiers in MYSQL ......................................................................
Add escaping of identifiers in MYSQL Change-Id: I225a78ed3789b69b3b50582c3c04691c909de26a --- M src/MySQL/MySQLFieldSqlBuilder.php M src/MySQL/MySQLIndexSqlBuilder.php M src/MySQL/MySQLSchemaSqlBuilder.php M src/MySQL/MySQLTableSqlBuilder.php M tests/phpunit/MySQL/MySQLFieldSqlBuilderTest.php M tests/phpunit/MySQL/MySQLIndexSqlBuilderTest.php M tests/phpunit/MySQL/MySQLSchemaSqlBuilderTest.php M tests/phpunit/MySQL/MySQLTableSqlBuilderTest.php 8 files changed, 81 insertions(+), 32 deletions(-) Approvals: Jeroen De Dauw: Looks good to me, approved jenkins-bot: Verified diff --git a/src/MySQL/MySQLFieldSqlBuilder.php b/src/MySQL/MySQLFieldSqlBuilder.php index 4034902..3adbc40 100644 --- a/src/MySQL/MySQLFieldSqlBuilder.php +++ b/src/MySQL/MySQLFieldSqlBuilder.php @@ -24,8 +24,7 @@ } public function getFieldSQL( FieldDefinition $field ){ - //todo escape name once identifier escaping is implemented - $sql = $field->getName() . ' '; + $sql = $this->escaper->getEscapedIdentifier( $field->getName() ). ' '; $sql .= $this->getFieldType( $field->getType() ); diff --git a/src/MySQL/MySQLIndexSqlBuilder.php b/src/MySQL/MySQLIndexSqlBuilder.php index 5140173..526c904 100644 --- a/src/MySQL/MySQLIndexSqlBuilder.php +++ b/src/MySQL/MySQLIndexSqlBuilder.php @@ -3,6 +3,7 @@ namespace Wikibase\Database\MySQL; use RuntimeException; +use Wikibase\Database\Escaper; use Wikibase\Database\Schema\Definitions\IndexDefinition; use Wikibase\Database\Schema\Definitions\TableDefinition; use Wikibase\Database\Schema\IndexSqlBuilder; @@ -15,10 +16,15 @@ */ class MySQLIndexSqlBuilder extends IndexSqlBuilder { + protected $escaper; + protected $tableNameFormatter; + /** + * @param Escaper $escaper * @param TableNameFormatter $tableNameFormatter */ - public function __construct( TableNameFormatter $tableNameFormatter ) { + public function __construct( Escaper $escaper, TableNameFormatter $tableNameFormatter ) { + $this->escaper = $escaper; $this->tableNameFormatter = $tableNameFormatter; } @@ -27,17 +33,17 @@ $sql .= $this->getIndexType( $index->getType() ); if( $index->getType() !== IndexDefinition::TYPE_PRIMARY ){ - $sql .= ' `'.$index->getName().'`';//todo escape index name? + $sql .= ' ' . $this->escaper->getEscapedIdentifier( $index->getName() ); } $sql .= ' ON ' . $this->tableNameFormatter->formatTableName( $tableName ); $columnNames = array(); foreach( $index->getColumns() as $columnName => $intSize ){ - $columnNames[] = $columnName; + $columnNames[] = $this->escaper->getEscapedIdentifier( $columnName ); } - $sql .= ' (`'.implode( '`,`', $columnNames ).'`)'; + $sql .= ' (' . implode( ',', $columnNames ). ')'; return $sql; } diff --git a/src/MySQL/MySQLSchemaSqlBuilder.php b/src/MySQL/MySQLSchemaSqlBuilder.php index 75baf07..06fc9c0 100644 --- a/src/MySQL/MySQLSchemaSqlBuilder.php +++ b/src/MySQL/MySQLSchemaSqlBuilder.php @@ -18,11 +18,13 @@ */ class MySQLSchemaSqlBuilder implements SchemaModificationSqlBuilder { + protected $escaper; protected $fieldSqlBuilder; protected $tableNameFormatter; - public function __construct( Escaper $fieldValueEscaper, TableNameFormatter $tableNameFormatter ) { - $this->fieldSqlBuilder = new MySQLFieldSqlBuilder( $fieldValueEscaper ); + public function __construct( Escaper $escaper, TableNameFormatter $tableNameFormatter ) { + $this->escaper = $escaper; + $this->fieldSqlBuilder = new MySQLFieldSqlBuilder( $escaper ); $this->tableNameFormatter = $tableNameFormatter; } @@ -34,6 +36,7 @@ */ public function getRemoveFieldSql( $tableName, $fieldName ) { $tableName = $this->tableNameFormatter->formatTableName( $tableName ); + $fieldName = $this->escaper->getEscapedIdentifier( $fieldName ); return "ALTER TABLE {$tableName} DROP {$fieldName}"; //todo escape $fieldName? } @@ -56,6 +59,7 @@ */ public function getRemoveIndexSql( $tableName, $indexName ){ $tableName = $this->tableNameFormatter->formatTableName( $tableName ); + $indexName = $this->escaper->getEscapedIdentifier( $indexName ); return "DROP INDEX {$indexName} ON {$tableName}"; } @@ -66,7 +70,7 @@ * @return string */ public function getAddIndexSql( $tableName, IndexDefinition $index ){ - $indexSqlBuilder = new MySQLIndexSqlBuilder( $this->tableNameFormatter ); + $indexSqlBuilder = new MySQLIndexSqlBuilder( $this->escaper, $this->tableNameFormatter ); return $indexSqlBuilder->getIndexSQL( $index, $tableName ); } diff --git a/src/MySQL/MySQLTableSqlBuilder.php b/src/MySQL/MySQLTableSqlBuilder.php index 41d3818..c32e488 100644 --- a/src/MySQL/MySQLTableSqlBuilder.php +++ b/src/MySQL/MySQLTableSqlBuilder.php @@ -80,18 +80,17 @@ $sql = $this->getIndexType( $index->getType() ); if( $index->getType() !== IndexDefinition::TYPE_PRIMARY ){ - //todo escape name once identifier escaping is implemented - $sql .= ' `'.$index->getName().'`'; + $sql .= ' ' . $this->escaper->getEscapedIdentifier( $index->getName() ); } $columnNames = array(); //TODO FIXME Error: 1170 BLOB/TEXT column 'textfield' used in key specification without a key length (localhost) //todo $intSize here needs to specify the length of the key for text fields. foreach( $index->getColumns() as $columnName => $intSize ){ - $columnNames[] = $columnName; + $columnNames[] = $this->escaper->getEscapedIdentifier( $columnName ); } - $sql .= ' (`'.implode( '`,`', $columnNames ).'`)'; + $sql .= ' (' . implode( ',', $columnNames ) . ')'; return $sql; } diff --git a/tests/phpunit/MySQL/MySQLFieldSqlBuilderTest.php b/tests/phpunit/MySQL/MySQLFieldSqlBuilderTest.php index 1bca0a3..8d99654 100644 --- a/tests/phpunit/MySQL/MySQLFieldSqlBuilderTest.php +++ b/tests/phpunit/MySQL/MySQLFieldSqlBuilderTest.php @@ -30,6 +30,12 @@ return '|' . $value . '|'; } ) ); + $mockEscaper->expects( $this->any() ) + ->method( 'getEscapedIdentifier' ) + ->will( $this->returnCallback( function( $value ) { + return '-' . $value . '-'; + } ) ); + $sqlBuilder = new MySQLFieldSqlBuilder( $mockEscaper ); $actualSQL = $sqlBuilder->getFieldSQL( $field ); @@ -45,7 +51,7 @@ 'fieldName', FieldDefinition::TYPE_INTEGER ), - 'fieldName INT NULL' + '-fieldName- INT NULL' ); $argLists[] = array( @@ -57,7 +63,7 @@ FieldDefinition::NO_ATTRIB, FieldDefinition::AUTOINCREMENT ), - 'fieldName INT NULL AUTO_INCREMENT' + '-fieldName- INT NULL AUTO_INCREMENT' ); $argLists[] = array( @@ -66,7 +72,7 @@ FieldDefinition::TYPE_FLOAT, FieldDefinition::NOT_NULL ), - 'fieldName FLOAT NOT NULL' + '-fieldName- FLOAT NOT NULL' ); $argLists[] = array( @@ -76,7 +82,7 @@ FieldDefinition::NOT_NULL, '1' ), - 'fieldName TINYINT DEFAULT |1| NOT NULL' + '-fieldName- TINYINT DEFAULT |1| NOT NULL' ); $argLists[] = array( @@ -86,7 +92,7 @@ FieldDefinition::NOT_NULL, 'foo' ), - 'fieldName BLOB DEFAULT |foo| NOT NULL' + '-fieldName- BLOB DEFAULT |foo| NOT NULL' ); return $argLists; diff --git a/tests/phpunit/MySQL/MySQLIndexSqlBuilderTest.php b/tests/phpunit/MySQL/MySQLIndexSqlBuilderTest.php index b6c9f32..912fd6e 100644 --- a/tests/phpunit/MySQL/MySQLIndexSqlBuilderTest.php +++ b/tests/phpunit/MySQL/MySQLIndexSqlBuilderTest.php @@ -22,12 +22,19 @@ * @dataProvider fieldAndSqlProvider */ public function testGetCreateTableSql( IndexDefinition $index, $expectedSQL ) { + $mockEscaper = $this->getMock( 'Wikibase\Database\Escaper' ); + $mockEscaper->expects( $this->any() ) + ->method( 'getEscapedIdentifier' ) + ->will( $this->returnCallback( function( $value ) { + return '-' . $value . '-'; + } ) ); + $mockTableNameFormatter = $this->getMock( 'Wikibase\Database\TableNameFormatter' ); $mockTableNameFormatter->expects( $this->any() ) ->method( 'formatTableName' ) ->will( $this->returnArgument(0) ); - $sqlBuilder = new MySQLIndexSqlBuilder( $mockTableNameFormatter ); + $sqlBuilder = new MySQLIndexSqlBuilder( $mockEscaper, $mockTableNameFormatter ); $sql = $sqlBuilder->getIndexSQL( $index, 'tableName' ); $this->assertEquals( $expectedSQL, $sql ); } @@ -44,7 +51,7 @@ array( 'intField' => 0, 'textField' => 0 ), IndexDefinition::TYPE_INDEX ), - 'CREATE INDEX `indexName` ON tableName (`intField`,`textField`)' + 'CREATE INDEX -indexName- ON tableName (-intField-,-textField-)' ); $argLists[] = array( @@ -53,7 +60,7 @@ array( 'intField' => 0, 'textField' => 0 ), IndexDefinition::TYPE_UNIQUE ), - 'CREATE UNIQUE INDEX `indexName` ON tableName (`intField`,`textField`)' + 'CREATE UNIQUE INDEX -indexName- ON tableName (-intField-,-textField-)' ); $argLists[] = array( @@ -62,7 +69,7 @@ array( 'intField' => 0, 'textField' => 0 ), IndexDefinition::TYPE_PRIMARY ), - 'CREATE PRIMARY KEY ON tableName (`intField`,`textField`)' + 'CREATE PRIMARY KEY ON tableName (-intField-,-textField-)' ); return $argLists; @@ -70,6 +77,13 @@ public function testUnsupportedType() { $this->setExpectedException( 'RuntimeException', 'does not support db indexes of type' ); + + $mockEscaper = $this->getMock( 'Wikibase\Database\Escaper' ); + $mockEscaper->expects( $this->any() ) + ->method( 'getEscapedIdentifier' ) + ->will( $this->returnCallback( function( $value ) { + return '-' . $value . '-'; + } ) ); $tableNameFormatter = $this->getMockBuilder( 'Wikibase\Database\MediaWiki\MediaWikiTableNameFormatter' ) ->disableOriginalConstructor() @@ -82,7 +96,7 @@ ->method( 'getType' ) ->will( $this->returnValue( 'foobar' ) ); - $sqlBuilder = new MySQLIndexSqlBuilder( $tableNameFormatter ); + $sqlBuilder = new MySQLIndexSqlBuilder( $mockEscaper, $tableNameFormatter ); $sqlBuilder->getIndexSQL( $indexDefinition, 'tableName' ); } diff --git a/tests/phpunit/MySQL/MySQLSchemaSqlBuilderTest.php b/tests/phpunit/MySQL/MySQLSchemaSqlBuilderTest.php index 5b7d886..305186e 100644 --- a/tests/phpunit/MySQL/MySQLSchemaSqlBuilderTest.php +++ b/tests/phpunit/MySQL/MySQLSchemaSqlBuilderTest.php @@ -21,10 +21,17 @@ private function newInstance() { $mockEscaper = $this->getMock( 'Wikibase\Database\Escaper' ); + $mockEscaper->expects( $this->any() ) ->method( 'getEscapedValue' ) ->will( $this->returnCallback( function( $input ) { return "|$input|"; + } ) ); + + $mockEscaper->expects( $this->any() ) + ->method( 'getEscapedIdentifier' ) + ->will( $this->returnCallback( function( $value ) { + return '-' . $value . '-'; } ) ); $mockTableNameFormatter = $this->getMock( 'Wikibase\Database\TableNameFormatter' ); @@ -40,26 +47,26 @@ public function testGetRemoveFieldSql(){ $instance = $this->newInstance(); $sql = $instance->getRemoveFieldSql( 'tableName', 'fieldName' ); - $this->assertEquals( "ALTER TABLE ||tableName|| DROP fieldName", $sql ); + $this->assertEquals( "ALTER TABLE ||tableName|| DROP -fieldName-", $sql ); } public function testGetAddFieldSql(){ $instance = $this->newInstance(); $field = new FieldDefinition( 'intField', FieldDefinition::TYPE_INTEGER, FieldDefinition::NOT_NULL, 42 ); $sql = $instance->getAddFieldSql( 'tableName', $field ); - $this->assertEquals( 'ALTER TABLE ||tableName|| ADD intField INT DEFAULT |42| NOT NULL', $sql ); + $this->assertEquals( 'ALTER TABLE ||tableName|| ADD -intField- INT DEFAULT |42| NOT NULL', $sql ); } public function testGetRemoveIndexSql(){ $instance = $this->newInstance(); $sql = $instance->getRemoveIndexSql( 'tableName', 'indexName' ); - $this->assertEquals( "DROP INDEX indexName ON ||tableName||", $sql ); + $this->assertEquals( "DROP INDEX -indexName- ON ||tableName||", $sql ); } public function testGetAddIndexSql(){ $instance = $this->newInstance(); - $sql = $instance->getAddIndexSql( 'tableName', new IndexDefinition( 'name', array( 'a' => 0, 'b' => 0 ), IndexDefinition::TYPE_INDEX ) ); - $this->assertEquals( "CREATE INDEX `name` ON ||tableName|| (`a`,`b`)", $sql ); + $sql = $instance->getAddIndexSql( 'tableName', new IndexDefinition( 'indexName', array( 'a' => 0, 'b' => 0 ), IndexDefinition::TYPE_INDEX ) ); + $this->assertEquals( "CREATE INDEX -indexName- ON ||tableName|| (-a-,-b-)", $sql ); } } \ No newline at end of file diff --git a/tests/phpunit/MySQL/MySQLTableSqlBuilderTest.php b/tests/phpunit/MySQL/MySQLTableSqlBuilderTest.php index 6dc7c4e..fbbc73c 100644 --- a/tests/phpunit/MySQL/MySQLTableSqlBuilderTest.php +++ b/tests/phpunit/MySQL/MySQLTableSqlBuilderTest.php @@ -35,10 +35,24 @@ ->method( 'getEscapedValue' ) ->will( $this->returnArgument(0) ); + $mockEscaper->expects( $this->any() ) + ->method( 'getEscapedValue' ) + ->will( $this->returnCallback( function( $value ) { + return '|' . $value . '|'; + } ) ); + + $mockEscaper->expects( $this->any() ) + ->method( 'getEscapedIdentifier' ) + ->will( $this->returnCallback( function( $value ) { + return '-' . $value . '-'; + } ) ); + $mockTableNameFormatter = $this->getMock( 'Wikibase\Database\TableNameFormatter' ); $mockTableNameFormatter->expects( $this->any() ) ->method( 'formatTableName' ) ->will( $this->returnArgument(0) ); + + return new MySQLTableSqlBuilder( self::DB_NAME, @@ -68,7 +82,7 @@ new FieldDefinition( 'fieldName', FieldDefinition::TYPE_INTEGER ) ) ), - 'CREATE TABLE `dbName`.tableName (fieldName INT NULL) ENGINE=InnoDB, DEFAULT CHARSET=binary;' + 'CREATE TABLE `dbName`.tableName (-fieldName- INT NULL) ENGINE=InnoDB, DEFAULT CHARSET=binary;' ); $argLists[] = array( @@ -86,7 +100,7 @@ ), ) ), - 'CREATE TABLE `dbName`.tableName (primaryField INT NOT NULL, textField BLOB NULL, intField INT DEFAULT 42 NOT NULL) ENGINE=InnoDB, DEFAULT CHARSET=binary;' + 'CREATE TABLE `dbName`.tableName (-primaryField- INT NOT NULL, -textField- BLOB NULL, -intField- INT DEFAULT 42 NOT NULL) ENGINE=InnoDB, DEFAULT CHARSET=binary;' ); @@ -107,7 +121,7 @@ ), ) ), - 'CREATE TABLE `dbName`.tableName (textField BLOB NULL, intField INT DEFAULT 42 NOT NULL, INDEX `indexName` (`textField`,`intField`)) ENGINE=InnoDB, DEFAULT CHARSET=binary;' + 'CREATE TABLE `dbName`.tableName (-textField- BLOB NULL, -intField- INT DEFAULT 42 NOT NULL, INDEX -indexName- (-textField-,-intField-)) ENGINE=InnoDB, DEFAULT CHARSET=binary;' ); $argLists[] = array( @@ -133,7 +147,7 @@ ), ) ), - 'CREATE TABLE `dbName`.tableName (textField BLOB NULL, intField INT DEFAULT 42 NOT NULL, textField2 BLOB NULL, INDEX `indexName` (`intField`), UNIQUE INDEX `uniqueIndexName` (`textField2`)) ENGINE=InnoDB, DEFAULT CHARSET=binary;' + 'CREATE TABLE `dbName`.tableName (-textField- BLOB NULL, -intField- INT DEFAULT 42 NOT NULL, -textField2- BLOB NULL, INDEX -indexName- (-intField-), UNIQUE INDEX -uniqueIndexName- (-textField2-)) ENGINE=InnoDB, DEFAULT CHARSET=binary;' ); return $argLists; -- To view, visit https://gerrit.wikimedia.org/r/89631 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I225a78ed3789b69b3b50582c3c04691c909de26a Gerrit-PatchSet: 3 Gerrit-Project: mediawiki/extensions/WikibaseDatabase Gerrit-Branch: master Gerrit-Owner: Addshore <addshorew...@gmail.com> Gerrit-Reviewer: Jeroen De Dauw <jeroended...@gmail.com> Gerrit-Reviewer: jenkins-bot _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits