jenkins-bot has submitted this change and it was merged.
Change subject: Fix GetClaimsStatementFilter not failing correctly with invalid
rank
......................................................................
Fix GetClaimsStatementFilter not failing correctly with invalid rank
Luckily this is not critical in production, because the API takes care
that no invalid rank can make it that far. See GetClaims::getAllowedParams.
But when looking at the two classes I'm touching in this patch, there is
no safety. Both filters (by rank and by property) had bugs and failed with
type errors and "undefined variable" errors.
The StatementRankSerializer did not had any safety.
This also increases code coverage (which was the reason why I started
working on this).
Change-Id: I750c5c569cd336ff67f64744d23ab1666ce790b6
---
M repo/includes/StatementRankSerializer.php
M repo/includes/api/GetClaimsStatementFilter.php
M repo/tests/phpunit/includes/StatementRankSerializerTest.php
A repo/tests/phpunit/includes/api/GetClaimsStatementFilterTest.php
4 files changed, 188 insertions(+), 38 deletions(-)
Approvals:
Daniel Kinzler: Looks good to me, approved
jenkins-bot: Verified
diff --git a/repo/includes/StatementRankSerializer.php
b/repo/includes/StatementRankSerializer.php
index f8ed719..d8da6c4 100644
--- a/repo/includes/StatementRankSerializer.php
+++ b/repo/includes/StatementRankSerializer.php
@@ -3,6 +3,8 @@
namespace Wikibase;
use Deserializers\Deserializer;
+use Deserializers\Exceptions\DeserializationException;
+use Serializers\Exceptions\SerializationException;
use Serializers\Serializer;
use Wikibase\DataModel\Statement\Statement;
@@ -15,6 +17,7 @@
* @author Jeroen De Dauw < jeroended...@gmail.com >
* @author Daniel Kinzler
* @author Addshore
+ * @author Thiemo Mättig
*/
class StatementRankSerializer implements Serializer, Deserializer {
@@ -41,21 +44,32 @@
*
* @param string $serializedRank
*
- * @return integer
+ * @throws DeserializationException
+ * @return int
*/
public function deserialize( $serializedRank ) {
$ranks = array_flip( self::$rankMap );
+
+ if ( !array_key_exists( $serializedRank, $ranks ) ) {
+ throw new DeserializationException( 'Invalid rank
serialization' );
+ }
+
return $ranks[$serializedRank];
}
/**
* Serializes the rank.
*
- * @param integer $rank
+ * @param int $rank
*
+ * @throws SerializationException
* @return string
*/
public function serialize( $rank ) {
+ if ( !array_key_exists( $rank, self::$rankMap ) ) {
+ throw new SerializationException( 'Invalid rank' );
+ }
+
return self::$rankMap[$rank];
}
diff --git a/repo/includes/api/GetClaimsStatementFilter.php
b/repo/includes/api/GetClaimsStatementFilter.php
index 3f16596..37ac88e 100644
--- a/repo/includes/api/GetClaimsStatementFilter.php
+++ b/repo/includes/api/GetClaimsStatementFilter.php
@@ -2,6 +2,7 @@
namespace Wikibase\Repo\Api;
+use Deserializers\Exceptions\DeserializationException;
use Wikibase\DataModel\Entity\EntityId;
use Wikibase\DataModel\Entity\EntityIdParser;
use Wikibase\DataModel\Entity\EntityIdParsingException;
@@ -13,6 +14,7 @@
* @licence GNU GPL v2+
* @author Jeroen De Dauw < jeroended...@gmail.com >
* @author Addshore
+ * @author Thiemo Mättig
*/
class GetClaimsStatementFilter implements StatementFilter {
@@ -29,11 +31,20 @@
private $idParser;
/**
- * @var array
+ * @var string[]
*/
private $requestParams;
- public function __construct( EntityIdParser $idParser, ApiErrorReporter
$errorReporter, array $requestParams ) {
+ /**
+ * @param EntityIdParser $idParser
+ * @param ApiErrorReporter $errorReporter
+ * @param string[] $requestParams
+ */
+ public function __construct(
+ EntityIdParser $idParser,
+ ApiErrorReporter $errorReporter,
+ array $requestParams
+ ) {
$this->idParser = $idParser;
$this->errorReporter = $errorReporter;
$this->requestParams = $requestParams;
@@ -42,22 +53,27 @@
/**
* @param Statement $statement
*
- * @return boolean
+ * @return bool
*/
public function statementMatches( Statement $statement ) {
return $this->rankMatchesFilter( $statement->getRank() )
&& $this->propertyMatchesFilter(
$statement->getPropertyId() );
}
+ /**
+ * @param int $rank
+ *
+ * @return bool
+ */
private function rankMatchesFilter( $rank ) {
- if ( $rank === null ) {
- return true;
- }
-
if ( isset( $this->requestParams['rank'] ) ) {
- $statementRankSerializer = new
StatementRankSerializer();
- $unserializedRank =
$statementRankSerializer->deserialize( $this->requestParams['rank'] );
- return $rank === $unserializedRank;
+ try {
+ $serializer = new StatementRankSerializer();
+ $deserializedRank = $serializer->deserialize(
$this->requestParams['rank'] );
+ return $rank === $deserializedRank;
+ } catch ( DeserializationException $ex ) {
+ $this->errorReporter->dieException( $ex,
'param-invalid' );
+ }
}
return true;
@@ -67,12 +83,10 @@
if ( isset( $this->requestParams['property'] ) ) {
try {
$parsedProperty = $this->idParser->parse(
$this->requestParams['property'] );
- } catch ( EntityIdParsingException $e ) {
- $this->errorReporter->dieException( $e,
'param-invalid' );
+ return $propertyId->equals( $parsedProperty );
+ } catch ( EntityIdParsingException $ex ) {
+ $this->errorReporter->dieException( $ex,
'param-invalid' );
}
-
- /** @var EntityId $parsedProperty */
- return $propertyId->equals( $parsedProperty );
}
return true;
diff --git a/repo/tests/phpunit/includes/StatementRankSerializerTest.php
b/repo/tests/phpunit/includes/StatementRankSerializerTest.php
index 2eb2838..761184e 100644
--- a/repo/tests/phpunit/includes/StatementRankSerializerTest.php
+++ b/repo/tests/phpunit/includes/StatementRankSerializerTest.php
@@ -3,7 +3,7 @@
namespace Wikibase\Test;
use DataValues\Serializers\DataValueSerializer;
-use Wikibase\DataModel\Entity\PropertyId;
+use PHPUnit_Framework_TestCase;
use Wikibase\DataModel\SerializerFactory;
use Wikibase\DataModel\Snak\PropertyNoValueSnak;
use Wikibase\DataModel\Statement\Statement;
@@ -17,24 +17,41 @@
*
* @licence GNU GPL v2+
* @author Addshore
+ * @author Thiemo Mättig
*/
-class StatementRankSerializerTest extends \PHPUnit_Framework_TestCase {
+class StatementRankSerializerTest extends PHPUnit_Framework_TestCase {
public function rankProvider() {
- $ranks = array(
- Statement::RANK_NORMAL,
- Statement::RANK_PREFERRED,
- Statement::RANK_DEPRECATED,
+ return array(
+ array( Statement::RANK_DEPRECATED, 'deprecated' ),
+ array( Statement::RANK_NORMAL, 'normal' ),
+ array( Statement::RANK_PREFERRED, 'preferred' ),
);
-
- return $this->arrayWrap( $ranks );
}
/**
* @dataProvider rankProvider
*/
- public function testRankSerialization( $rank ) {
- $statement = new Statement( new PropertyNoValueSnak( new
PropertyId( 'P42' ) ) );
+ public function testSerialize( $rank, $expected ) {
+ $serializer = new StatementRankSerializer();
+ $serialization = $serializer->serialize( $rank );
+ $this->assertSame( $expected, $serialization );
+ }
+
+ /**
+ * @dataProvider rankProvider
+ */
+ public function testDeserialize( $expected, $serialization ) {
+ $serializer = new StatementRankSerializer();
+ $deserialization = $serializer->deserialize( $serialization );
+ $this->assertSame( $expected, $deserialization );
+ }
+
+ /**
+ * @dataProvider rankProvider
+ */
+ public function testSerializerFactoryRoundtrip( $rank ) {
+ $statement = new Statement( new PropertyNoValueSnak( 1 ) );
$statement->setRank( $rank );
$factory = new SerializerFactory( new DataValueSerializer() );
@@ -44,26 +61,29 @@
$rankSerializer = new StatementRankSerializer();
- $this->assertEquals(
+ $this->assertSame(
$rank,
$rankSerializer->deserialize( $serialization['rank'] ),
- 'Roundtrip between rank serialization and
unserialization 1'
+ 'reference serialization can be deserialized'
);
- $this->assertEquals(
+ $this->assertSame(
$serialization['rank'],
$rankSerializer->serialize( $rank ),
- 'Roundtrip between rank serialization and
unserialization 2'
+ 'serialization is identical to reference'
);
}
- protected function arrayWrap( array $elements ) {
- return array_map(
- function ( $element ) {
- return array( $element );
- },
- $elements
- );
+ public function testGivenInvalidRank_serializationFails() {
+ $serializer = new StatementRankSerializer();
+ $this->setExpectedException(
'Serializers\Exceptions\SerializationException' );
+ $serializer->serialize( -1 );
+ }
+
+ public function testGivenInvalidSerialization_deserializeFails() {
+ $serializer = new StatementRankSerializer();
+ $this->setExpectedException(
'Deserializers\Exceptions\DeserializationException' );
+ $serializer->deserialize( 'invalid' );
}
}
diff --git a/repo/tests/phpunit/includes/api/GetClaimsStatementFilterTest.php
b/repo/tests/phpunit/includes/api/GetClaimsStatementFilterTest.php
new file mode 100644
index 0000000..f5eb29b
--- /dev/null
+++ b/repo/tests/phpunit/includes/api/GetClaimsStatementFilterTest.php
@@ -0,0 +1,102 @@
+<?php
+
+namespace Wikibase\DataModel\Services\Tests\Statement\Filter;
+
+use PHPUnit_Framework_TestCase;
+use Wikibase\DataModel\Entity\BasicEntityIdParser;
+use Wikibase\DataModel\Snak\PropertyNoValueSnak;
+use Wikibase\DataModel\Statement\Statement;
+use Wikibase\Repo\Api\ApiErrorReporter;
+use Wikibase\Repo\Api\GetClaimsStatementFilter;
+
+/**
+ * @covers Wikibase\Repo\Api\GetClaimsStatementFilter
+ *
+ * @licence GNU GPL v2+
+ * @author Thiemo Mättig
+ */
+class GetClaimsStatementFilterTest extends PHPUnit_Framework_TestCase {
+
+ /**
+ * @param bool $expectsError
+ *
+ * @return ApiErrorReporter
+ */
+ private function getApiErrorReporter( $expectsError = false ) {
+ $errorReporter = $this->getMockBuilder(
'Wikibase\Repo\Api\ApiErrorReporter' )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ if ( !$expectsError ) {
+ $errorReporter->expects( $this->never() )
+ ->method( 'dieException' );
+ } else {
+ $errorReporter->expects( $this->once() )
+ ->method( 'dieException' )
+ ->with(
+ $this->isInstanceOf( 'RuntimeException'
),
+ 'param-invalid'
+ );
+ }
+
+ return $errorReporter;
+ }
+
+ /**
+ * @dataProvider statementProvider
+ */
+ public function testIsMatch( array $requestParams, Statement
$statement, $expected ) {
+ $filter = new GetClaimsStatementFilter(
+ new BasicEntityIdParser(),
+ $this->getApiErrorReporter(),
+ $requestParams
+ );
+
+ $this->assertSame( $expected, $filter->statementMatches(
$statement ) );
+ }
+
+ public function statementProvider() {
+ $statement = new Statement( new PropertyNoValueSnak( 1 ) );
+
+ return array(
+ // No filter
+ array( array(), $statement, true ),
+
+ // Filter by rank
+ array( array( 'rank' => 'normal' ), $statement, true ),
+ array( array( 'rank' => 'deprecated' ), $statement,
false ),
+
+ // Filter by property
+ array( array( 'property' => 'p1' ), $statement, true ),
+ array( array( 'property' => 'p2' ), $statement, false ),
+
+ // Filter by both rank and property
+ array( array( 'rank' => 'normal', 'property' => 'p1' ),
$statement, true ),
+ array( array( 'rank' => 'deprecated', 'property' =>
'p1' ), $statement, false ),
+ array( array( 'rank' => 'normal', 'property' => 'p2' ),
$statement, false ),
+ );
+ }
+
+ public function testInvalidRankSerialization() {
+ $filter = new GetClaimsStatementFilter(
+ new BasicEntityIdParser(),
+ $this->getApiErrorReporter( true ),
+ array( 'rank' => 'invalid' )
+ );
+
+ $statement = new Statement( new PropertyNoValueSnak( 1 ) );
+ $this->assertTrue( $filter->statementMatches( $statement ) );
+ }
+
+ public function testInvalidPropertySerialization() {
+ $filter = new GetClaimsStatementFilter(
+ new BasicEntityIdParser(),
+ $this->getApiErrorReporter( true ),
+ array( 'property' => 'invalid' )
+ );
+
+ $statement = new Statement( new PropertyNoValueSnak( 1 ) );
+ $this->assertTrue( $filter->statementMatches( $statement ) );
+ }
+
+}
--
To view, visit https://gerrit.wikimedia.org/r/270004
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I750c5c569cd336ff67f64744d23ab1666ce790b6
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de>
Gerrit-Reviewer: Addshore <addshorew...@gmail.com>
Gerrit-Reviewer: Daniel Kinzler <daniel.kinz...@wikimedia.de>
Gerrit-Reviewer: Hoo man <h...@online.de>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
