Soeren.oldag has uploaded a new change for review.
https://gerrit.wikimedia.org/r/216071
Change subject: Improved sql escaping.
......................................................................
Improved sql escaping.
Change-Id: Iee8a4b4ae705b151550d169e9041c293615e6e07
---
M includes/DumpMetaInformation/DumpMetaInformationRepo.php
M includes/ExternalDataRepo.php
M tests/phpunit/DumpMetaInformation/DumpMetaInformationRepoTest.php
M tests/phpunit/ExternalDataRepoTest.php
4 files changed, 90 insertions(+), 76 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/WikidataQualityExternalValidation
refs/changes/71/216071/1
diff --git a/includes/DumpMetaInformation/DumpMetaInformationRepo.php
b/includes/DumpMetaInformation/DumpMetaInformationRepo.php
index 8d2fc29..762f5ad 100755
--- a/includes/DumpMetaInformation/DumpMetaInformationRepo.php
+++ b/includes/DumpMetaInformation/DumpMetaInformationRepo.php
@@ -106,14 +106,17 @@
}
}
- $db = wfGetDB( DB_SLAVE );
- $condition = $this->buildSqlInCondition(
- sprintf( '%s.dump_id', $db->tableName( DUMP_META_TABLE, null ) ),
- $dumpIds
- );
- $dumpMetaInformation = $this->getFromDb( $db, $condition );
+ if( count( $dumpIds ) > 0 ) {
+ $db = wfGetDB( DB_SLAVE );
+ $dumpIdColumn = sprintf( '%s.dump_id', $db->tableName(
$this->dumpMetaTableName ) );
+ $conditions = array(
+ $dumpIdColumn => $dumpIds
+ );
- return $dumpMetaInformation;
+ return $this->getFromDb( $db, $conditions );
+ }
+
+ return array();
}
/**
@@ -130,26 +133,32 @@
}
}
- $db = wfGetDB( DB_SLAVE );
- $identifierPropertyIds = array_map(
- function ( PropertyId $propertyId ) {
- return $propertyId->getSerialization();
- },
- $identifierPropertyIds
- );
- $result = $db->select(
- DUMP_IDENTIFIER_PROPERTIES_TABLE,
- 'dump_id',
- $this->buildSqlInCondition( 'identifier_pid',
$identifierPropertyIds )
- );
- $dumpIds = array();
- foreach ( $result as $row ) {
- if ( !in_array( $row->dump_id, $dumpIds ) ) {
- $dumpIds[] = $row->dump_id;
+ if( count( $identifierPropertyIds ) > 0 ) {
+ $db = wfGetDB( DB_SLAVE );
+ $identifierPropertyIds = array_map(
+ function ( PropertyId $propertyId ) {
+ return $propertyId->getSerialization();
+ },
+ $identifierPropertyIds
+ );
+ $result = $db->select(
+ $this->identifierPropertiesTableName,
+ 'dump_id',
+ array(
+ 'identifier_pid' => $identifierPropertyIds
+ )
+ );
+ $dumpIds = array();
+ foreach ( $result as $row ) {
+ if ( !in_array( $row->dump_id, $dumpIds ) ) {
+ $dumpIds[] = $row->dump_id;
+ }
}
+
+ return $this->getWithIds( $dumpIds );
}
- return $this->getWithIds( $dumpIds );
+ return array();
}
/**
@@ -167,17 +176,17 @@
/**
* @param DatabaseBase $db
- * @param string $condition
+ * @param array|string $conditions
* @return DumpMetaInformation[]
*/
- private function getFromDb( DatabaseBase $db, $condition = '' ) {
+ private function getFromDb( DatabaseBase $db, $conditions = '' ) {
$result = $db->select(
array(
$this->dumpMetaTableName,
$this->identifierPropertiesTableName
),
'*',
- $condition,
+ $conditions,
__METHOD__,
array(),
array(
@@ -233,28 +242,6 @@
return $dumpMetaInformation;
}
- /**
- * Creates SQL condition for WHERE clauses using the IN operator.
- *
- * @param string $columnName
- * @param array $values
- * @return string
- */
- private static function buildSqlInCondition( $columnName, array $values ) {
- if ( count( $values ) > 0 ) {
- $values = array_map(
- function ( $value ) {
- return sprintf( '"%s"', $value );
- },
- $values
- );
-
- return sprintf( '%s in (%s)', $columnName, implode( ',', $values )
);
- } else {
- return '0=1';
- }
- }
-
/**
* Inserts or updates given dump meta information to database
@@ -262,21 +249,21 @@
* @param DumpMetaInformation $dumpMetaInformation
*/
public function save( DumpMetaInformation $dumpMetaInformation ) {
- $db = wfGetDB( DB_SLAVE );
- $accumulator = $this->getDumpInformationArray( $db,
$dumpMetaInformation );
-
$dumpId = $dumpMetaInformation->getDumpId();
+ $accumulator = $this->getDumpInformationArray( $dumpMetaInformation );
+
+ $db = wfGetDB( DB_SLAVE );
$existing = $db->selectRow(
$this->dumpMetaTableName,
array( 'dump_id' ),
- array( "dump_id='$dumpId'" )
+ array( 'dump_id' => $dumpId )
);
if ( $existing ) {
$db->update(
$this->dumpMetaTableName,
$accumulator,
- array( "dump_id='$dumpId'" )
+ array( 'dump_id' => $dumpId )
);
} else {
$db->insert(
@@ -287,7 +274,7 @@
$db->delete(
$this->identifierPropertiesTableName,
- "dump_id='$dumpId'"
+ array( 'dump_id' => $dumpId )
);
$db->insert(
$this->identifierPropertiesTableName,
diff --git a/includes/ExternalDataRepo.php b/includes/ExternalDataRepo.php
index 686d5f4..dc09988 100755
--- a/includes/ExternalDataRepo.php
+++ b/includes/ExternalDataRepo.php
@@ -5,6 +5,7 @@
use Doctrine\Instantiator\Exception\InvalidArgumentException;
use Wikibase\DataModel\Entity\PropertyId;
use DBError;
+use DatabaseBase;
/**
@@ -59,6 +60,17 @@
$this->assertIsArrayOfStrings( $dumpIds, '$dumpIds' );
$this->assertIsArrayOfStrings( $externalIds, '$externalIds' );
+ $conditions = array();
+ if( $dumpIds ) {
+ $conditions['dump_id'] = $dumpIds;
+ }
+ if( $externalIds ) {
+ $conditions['external_id'] = $externalIds;
+ }
+ if( $propertyIds ) {
+ $conditions['pid'] = $propertyIds;
+ }
+
$externalData = array();
$db = wfGetDB( DB_SLAVE );
$result = $db->select(
@@ -69,11 +81,7 @@
'pid',
'external_value'
),
- array(
- $this->buildSqlInCondition( 'dump_id', $dumpIds ),
- $this->buildSqlInCondition( 'external_id', $externalIds ),
- $this->buildSqlInCondition( 'pid', $propertyIds )
- )
+ $conditions
);
foreach ( $result as $row ) {
@@ -108,8 +116,9 @@
* @return bool
*/
public function insertBatch( array $externalDataBatch ) {
+ $db = wfGetDB( DB_MASTER );
$accumulator = array_map(
- function ( $externalData ) {
+ function ( $externalData ) use ( $db) {
return array(
'dump_id' => $externalData[0],
'external_id' => $externalData[1],
@@ -120,7 +129,6 @@
$externalDataBatch
);
- $db = wfGetDB( DB_MASTER );
$db->commit( __METHOD__, "flush" );
wfWaitForSlaves();
@@ -132,13 +140,13 @@
*
* @param string $dumpId
* @param int $batchSize
- * @return bool|\ResultWrapper
* @throws \DBUnexpectedError
*/
public function deleteOfDump( $dumpId, $batchSize = 1000 ) {
$this->assertIsString( $dumpId, '$dumpId' );
+ $this->assertIsInt( $batchSize, 'batchSize' );
- return $this->deleteOfDumps( array( $dumpId ), $batchSize );
+ $this->deleteOfDumps( array( $dumpId ), $batchSize );
}
/**
@@ -150,9 +158,10 @@
*/
public function deleteOfDumps( array $dumpIds, $batchSize = 1000 ) {
$this->assertIsArrayOfStrings( $dumpIds, '$dumpIds' );
+ $this->assertIsInt( $batchSize, 'batchSize' );
$db = wfGetDB( DB_MASTER );
- $condition = $this->buildSqlInCondition( 'dump_id', $dumpIds );
+ $condition = $this->buildSqlInCondition( $db, 'dump_id', $dumpIds );
if ( $db->getType() === 'sqlite' ) {
$db->delete( DUMP_DATA_TABLE, $condition );
} else {
@@ -172,18 +181,15 @@
* @param array $values
* @return string
*/
- private static function buildSqlInCondition( $columnName, array $values ) {
+ private static function buildSqlInCondition( DatabaseBase $db,
$columnName, array $values ) {
if ( count( $values ) > 0 ) {
$values = array_map(
- function ( $value ) {
- return sprintf( '"%s"', $value );
+ function ( $value ) use ( $db ) {
+ return $db->addQuotes( $value );
},
$values
);
-
- return sprintf( '%s in (%s)', $columnName, implode( ',', $values )
);
- } else {
- return '0=1';
+ return sprintf( '%s IN (%s)', $columnName, implode( ',', $values )
);
}
}
@@ -193,7 +199,17 @@
*/
private function assertIsString( $value, $argumentName ) {
if ( !is_string( $value ) ) {
- throw new InvalidArgumentException( "$argumentName must be
strings." );
+ throw new InvalidArgumentException( "$argumentName must be
string." );
+ }
+ }
+
+ /**
+ * @param int $value
+ * @param string $argumentName
+ */
+ private function assertIsInt( $value, $argumentName ) {
+ if ( !is_int( $value ) ) {
+ throw new InvalidArgumentException( "$argumentName must be int." );
}
}
diff --git a/tests/phpunit/DumpMetaInformation/DumpMetaInformationRepoTest.php
b/tests/phpunit/DumpMetaInformation/DumpMetaInformationRepoTest.php
index 3976e31..420c671 100755
--- a/tests/phpunit/DumpMetaInformation/DumpMetaInformationRepoTest.php
+++ b/tests/phpunit/DumpMetaInformation/DumpMetaInformationRepoTest.php
@@ -333,6 +333,11 @@
'baz' => $this->dumpMetaInformation['baz']
)
),
+ // Empty input array
+ array(
+ array(),
+ array()
+ ),
// Invalid identifier property ids
array(
array( 'P42' ),
diff --git a/tests/phpunit/ExternalDataRepoTest.php
b/tests/phpunit/ExternalDataRepoTest.php
index 58317f6..fef62fd 100755
--- a/tests/phpunit/ExternalDataRepoTest.php
+++ b/tests/phpunit/ExternalDataRepoTest.php
@@ -151,10 +151,16 @@
)
),
array(
+ array( 'fubar' ),
array(),
- array( 'foo', 'bar' ),
- array( 'P1', 'P3' ),
- array()
+ array(),
+ array(
+ 'fubar' => array(
+ 'bar' => array(
+ 'P3' => array( 'baz' )
+ )
+ )
+ )
),
array(
array( 42 ),
--
To view, visit https://gerrit.wikimedia.org/r/216071
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iee8a4b4ae705b151550d169e9041c293615e6e07
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/WikidataQualityExternalValidation
Gerrit-Branch: master
Gerrit-Owner: Soeren.oldag <[email protected]>
Gerrit-Reviewer: Jonaskeutel <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
