Aude has uploaded a new change for review.
https://gerrit.wikimedia.org/r/49500
Change subject: (bug 43990) Robust serialization of change objects
......................................................................
(bug 43990) Robust serialization of change objects
This change causes Diffs contained in Change objects to be serialized
as JSON structures instead of objects using PHP serialization. This
allows for more flexibility in parsing, and avoids problems when changing
the structure of the diff representation.
Change-Id: I9fd62298f6176f3937850cc3e009b22fa7126826
---
M lib/WikibaseLib.php
M lib/config/WikibaseLib.default.php
M lib/includes/ChangesTable.php
M lib/includes/changes/ChangeRow.php
M lib/includes/changes/DiffChange.php
M lib/includes/changes/EntityChange.php
M lib/includes/entity/EntityDiff.php
M lib/includes/item/ItemDiff.php
M lib/tests/phpunit/ChangesTableTest.php
M lib/tests/phpunit/changes/ChangeRowTest.php
M lib/tests/phpunit/changes/DiffChangeTest.php
M lib/tests/phpunit/changes/TestChanges.php
12 files changed, 372 insertions(+), 9 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/00/49500/1
diff --git a/lib/WikibaseLib.php b/lib/WikibaseLib.php
index 3051ab9..caa867e 100644
--- a/lib/WikibaseLib.php
+++ b/lib/WikibaseLib.php
@@ -89,6 +89,7 @@
$wgAutoloadClasses['Wikibase\LibHooks'] = $dir .
'WikibaseLib.hooks.php';
// includes
+$wgAutoloadClasses['Wikibase\Arrayalizer'] = $dir
. 'includes/Arrayalizer.php';
$wgAutoloadClasses['Wikibase\ByPropertyIdArray'] = $dir .
'includes/ByPropertyIdArray.php';
$wgAutoloadClasses['Wikibase\ChangeNotifier'] = $dir .
'includes/ChangeNotifier.php';
$wgAutoloadClasses['Wikibase\ChangeNotificationJob'] = $dir .
'includes/ChangeNotificationJob.php';
diff --git a/lib/config/WikibaseLib.default.php
b/lib/config/WikibaseLib.default.php
index ef8e453..aeb4d48 100644
--- a/lib/config/WikibaseLib.default.php
+++ b/lib/config/WikibaseLib.default.php
@@ -57,6 +57,11 @@
// local by default. Set to something LBFactory understands.
$wgWBSettings['changesDatabase'] = false;
+// JSON is more robust against version differences between repo and client,
+// but only once the client can cope with the JSON form of the change.
+// TODO: make this default to true!
+$wgWBSettings['changesAsJson'] = false;
+
// list of logical database names of local client wikis.
// may contain mappings from site-id to db-name.
$wgWBSettings['localClientDatabases'] = array();
diff --git a/lib/includes/ChangesTable.php b/lib/includes/ChangesTable.php
index acc050f..8c8aa58 100644
--- a/lib/includes/ChangesTable.php
+++ b/lib/includes/ChangesTable.php
@@ -85,7 +85,7 @@
'type' => 'str',
'time' => 'str', // TS_MW
- 'info' => 'blob',
+ 'info' => 'data', // handled specially by ChangeRow
'object_id' => 'str',
'user_id' => 'int',
'revision_id' => 'int',
diff --git a/lib/includes/changes/ChangeRow.php
b/lib/includes/changes/ChangeRow.php
index ca1785c..d53777e 100644
--- a/lib/includes/changes/ChangeRow.php
+++ b/lib/includes/changes/ChangeRow.php
@@ -144,4 +144,83 @@
return $this->getField( 'object_id' );
}
+
+ /**
+ * @see ORMRow::setField
+ *
+ * @since 0.4
+ *
+ * @param string $name
+ * @param mixed $value
+ *
+ * @throws \MWException
+ */
+ public function setField( $name, $value ) {
+ if ( $name === 'info' && is_string( $value ) ) {
+ $value = $this->unserializeInfo( $value );
+ }
+
+ parent::setField( $name, $value );
+ }
+
+ /**
+ * @see ORMRow::getWriteValues()
+ *
+ * @since 0.4
+ *
+ * @return array
+ */
+ protected function getWriteValues() {
+ $values = parent::getWriteValues();
+ $infoField = $this->table->getPrefixedField( 'info' );
+
+ if ( isset( $values[$infoField] ) ) {
+ $values[$infoField] = $this->serializeInfo(
$values[$infoField] );
+ }
+
+ return $values;
+ }
+
+ /**
+ * Serialized the info field using json_encode.
+ * This may be overridden by subclasses to implement special handling
+ * for information in the info field.
+ *
+ * @since 0.4
+ *
+ * @param array $info
+ *
+ * @return string
+ */
+ protected function serializeInfo( array $info ) {
+ if ( Settings::get( "changesAsJson" ) === true ) {
+ //XXX: we could JSON_UNESCAPED_UNICODE here, perhaps.
+ return json_encode( $info );
+ } else {
+ // for compatibility with old client code.
+ return serialize( $info );
+ }
+ }
+
+ /**
+ * Unserializes the info field using json_decode.
+ * This may be overridden by subclasses to implement special handling
+ * for information in the info field.
+ *
+ * @since 0.4
+ *
+ * @param string $str
+ *
+ * @return array the info array
+ */
+ protected function unserializeInfo( $str ) {
+ if ( $str[0] === '{' ) { // json
+ $info = json_decode( $str, true );
+ } else {
+ // we may still have legacy stuff in the database for a
while!
+ $info = unserialize( $str );
+ }
+
+ return $info;
+ }
}
diff --git a/lib/includes/changes/DiffChange.php
b/lib/includes/changes/DiffChange.php
index 33c2faa..1da1c8f 100644
--- a/lib/includes/changes/DiffChange.php
+++ b/lib/includes/changes/DiffChange.php
@@ -2,6 +2,7 @@
namespace Wikibase;
use Diff\IDiff;
+use Diff\Diff;
/**
* Class for changes that can be represented as a IDiff.
@@ -97,4 +98,117 @@
return true;
}
+ /**
+ * @see ChangeRow::serializeInfo()
+ *
+ * Overwritten to use the array representation of the diff.
+ *
+ * @since 0.4
+ * @param array $info
+ * @return string
+ */
+ protected function serializeInfo( array $info ) {
+ if ( isset( $info['diff'] ) && $info['diff'] instanceof
\Diff\DiffOp ) {
+ if ( Settings::get( "changesAsJson" ) === true ) {
+ /* @var \Diff\DiffOp $op */
+ $op = $info['diff'];
+ $info['diff'] = $op->toArray( array( $this,
'arrayalizeObjects' ) );
+ }
+ }
+
+ return parent::serializeInfo( $info );
+ }
+
+ /**
+ * @see ChangeRow::unserializeInfo()
+ *
+ * Overwritten to use the array representation of the diff.
+ *
+ * @since 0.4
+ * @param string $str
+ * @return array the info array
+ */
+ protected function unserializeInfo( $str ) {
+ static $factory = null;
+
+ if ( $factory == null ) {
+ $factory = new WikibaseDiffOpFactory( array( $this,
'objectifyArrays' ) );
+ }
+
+ $info = parent::unserializeInfo( $str );
+
+ if ( isset( $info['diff'] ) && is_array( $info['diff'] ) ) {
+ $info['diff'] = $factory->newFromArray( $info['diff'] );
+ }
+
+ return $info;
+ }
+
+ /**
+ * Converts an object to an array structure.
+ * Callback function for use by \Diff\DiffOp::toArray().
+ *
+ * Subclasses should override this to provide array representations of
specific value objects.
+ *
+ * @since 0.4
+ *
+ * @param mixed $data
+ * @return mixed
+ */
+ public function arrayalizeObjects( $data ) {
+ return $data; // noop
+ }
+
+ /**
+ * May be overwritten by subclasses to provide special handling.
+ * Callback function for use by \Diff\DiffOpFactory
+ *
+ * Subclasses should override this to reconstruct value objects from
arrays.
+ *
+ * @since 0.4
+ *
+ * @param array $data
+ * @return mixed
+ */
+ public function objectifyArrays( array $data ) {
+ return $data; // noop
+ }
+}
+
+class WikibaseDiffOpFactory extends \Diff\DiffOpFactory {
+ public function newFromArray( array $diffOp ) {
+ $this->assertHasKey( 'type', $diffOp );
+
+ // see EntityDiff::getType() and ItemDiff::getType()
+ if ( preg_match( '!^diff/(.*)$!', $diffOp['type'], $matches ) )
{
+ $itemType = $matches[1];
+ $this->assertHasKey( 'operations', $diffOp );
+
+ $operations = $this->createOperations(
$diffOp['operations'] );
+ $diff = EntityDiff::newForType( $itemType, $operations
);
+
+ return $diff;
+ }
+
+ return parent::newFromArray( $diffOp );
+ }
+
+ /**
+ * Converts a list of diff operations represented by arrays into a list
of
+ * DiffOp objects.
+ *
+ * @todo: pull this up into DiffOpFactory
+ *
+ * @param array $data the input data
+ * @return \Diff\DiffOp[] The diff ops
+ */
+ protected function createOperations( array $data ) {
+ $operations = array();
+
+ foreach ( $data as $key => $operation ) {
+ $operations[$key] = $this->newFromArray( $operation );
+ }
+
+ return $operations;
+ }
}
diff --git a/lib/includes/changes/EntityChange.php
b/lib/includes/changes/EntityChange.php
index 91676da..71e8a40 100644
--- a/lib/includes/changes/EntityChange.php
+++ b/lib/includes/changes/EntityChange.php
@@ -354,4 +354,54 @@
$s .= preg_replace( '/\s+/s', ' ', var_export( $fields, true )
);
return $s;
}
+
+ /**
+ * @see DiffChange::arrayalizeObjects
+ *
+ * Overwritten to handle Claim objects.
+ *
+ * @since 0.4
+ *
+ * @param mixed $data
+ * @return mixed
+ */
+ public function arrayalizeObjects( $data ) {
+ $data = parent::arrayalizeObjects( $data );
+
+ if ( $data instanceof Claim ) {
+ $a = $data->toArray();
+ $a['_claimclass_'] = get_class( $data );
+
+ return $a;
+ }
+
+ return $data;
+ }
+
+ /**
+ * @see DiffChange::objectifyArrays
+ *
+ * Overwritten to handle Claim objects.
+ *
+ * @since 0.4
+ *
+ * @param array $data
+ * @return mixed
+ */
+ public function objectifyArrays( array $data ) {
+ $data = parent::objectifyArrays( $data );
+
+ if ( is_array( $data ) && isset( $data['_claimclass_'] ) ) {
+ $class = $data['_claimclass_'];
+
+ if ( $class === 'Wikibase\Claim' || is_subclass_of(
$class, 'Wikibase\Claim' ) ) {
+ unset( $data['_claimclass_'] );
+
+ $claim = call_user_func( array( $class,
'newFromArray' ), $data );
+ return $claim;
+ }
+ }
+
+ return $data;
+ }
}
diff --git a/lib/includes/entity/EntityDiff.php
b/lib/includes/entity/EntityDiff.php
index 8f92ea5..72b0d1b 100644
--- a/lib/includes/entity/EntityDiff.php
+++ b/lib/includes/entity/EntityDiff.php
@@ -116,4 +116,12 @@
&& $this->getClaimsDiff()->isEmpty();
}
+ /**
+ * @see DiffOp::getType();
+ *
+ * @return string 'diff/entity'
+ */
+ public function getType() {
+ return 'diff/entity'; // generic
+ }
}
diff --git a/lib/includes/item/ItemDiff.php b/lib/includes/item/ItemDiff.php
index fadc4de..d765185 100644
--- a/lib/includes/item/ItemDiff.php
+++ b/lib/includes/item/ItemDiff.php
@@ -54,4 +54,12 @@
&& $this->getSiteLinkDiff()->isEmpty();
}
+ /**
+ * @see DiffOp::getType();
+ *
+ * @return string 'diff/' . Item::ENTITY_TYPE
+ */
+ public function getType() {
+ return 'diff/' . Item::ENTITY_TYPE;
+ }
}
\ No newline at end of file
diff --git a/lib/tests/phpunit/ChangesTableTest.php
b/lib/tests/phpunit/ChangesTableTest.php
index fed26a9..138e196 100644
--- a/lib/tests/phpunit/ChangesTableTest.php
+++ b/lib/tests/phpunit/ChangesTableTest.php
@@ -56,6 +56,29 @@
$this->setMwGlobals( 'wgUser', self::$user );
+ // Check that we can save and retrieve diffs.
+ $diff1 = new\Wikibase\ItemDiff(
+ array(
+ 'label' => new \Diff\Diff(
+ array(
+ "en" => new \Diff\DiffOpChange(
"OLD", "NEW" ),
+ )
+ )
+ )
+ );
+
+ // Make sure we can save and retrieve complex diff structures,
+ // even if they contain objects as values.
+ $diff2 = new\Wikibase\ItemDiff(
+ array(
+ 'claim' => new \Diff\Diff(
+ array(
+ new \Diff\DiffOpAdd( new
\Wikibase\Claim( new \Wikibase\PropertyNoValueSnak( 77 ) ) ),
+ )
+ )
+ )
+ );
+
return array(
array(
array(
@@ -65,8 +88,7 @@
'revision_id' => 9001,
'object_id' => $id->getPrefixedId(),
'info' => array(
- 'entity' =>
\Wikibase\Item::newEmpty(),
- 'diff' => new
\Wikibase\ItemDiff(),
+ 'diff' => $diff1,
)
),
true
@@ -74,13 +96,12 @@
array(
array(
'type' => 'wikibase-item~update',
- 'time' => '20120101000000',
+ 'time' => '20120101000005',
'user_id' => $wgUser->getId(),
- 'revision_id' => 9001,
+ 'revision_id' => 9002,
'object_id' => $id->getPrefixedId(),
'info' => array(
- 'entity' =>
\Wikibase\Item::newEmpty(),
- 'diff' => new
\Wikibase\ItemDiff,
+ 'diff' => $diff2,
)
),
true
@@ -130,6 +151,7 @@
$obtainedChange = $changesTable->selectRow( null, array( 'id'
=> $id ) );
$this->assertTrue( $obtainedChange !== false, 'Change could not
be loaded via ORMTable!' );
+ $this->assertEquals( $change, $obtainedChange, 'round trip' );
$this->assertEquals( 1, $changesTable->count( array( 'id' =>
$id ) ) );
diff --git a/lib/tests/phpunit/changes/ChangeRowTest.php
b/lib/tests/phpunit/changes/ChangeRowTest.php
index 4f70985..b3a8774 100644
--- a/lib/tests/phpunit/changes/ChangeRowTest.php
+++ b/lib/tests/phpunit/changes/ChangeRowTest.php
@@ -2,6 +2,9 @@
namespace Wikibase\Test;
use \Wikibase\ChangesTable;
+use \Wikibase\ChangeRow;
+use \Wikibase\Settings;
+
/**
* Tests for the Wikibase\ChangeRow class.
@@ -35,8 +38,21 @@
*
* @licence GNU GPL v2+
* @author Katie Filbert < [email protected] >
+ * @author Daniel Kinzler
*/
class ChangeRowTest extends \ORMRowTest {
+
+ /**
+ * @since 1.20
+ * @return array
+ */
+ protected function getMockValues() {
+ $values = parent::getMockValues();
+
+ // register special "data" type
+ $values['data'] = array( "foo", 'bar' );
+ return $values;
+ }
/**
* @see ORMRowTest::getRowClass
@@ -110,4 +126,36 @@
}
}
+ public function provideSaveAndLoad() {
+ $instanceCases = $this->instanceProvider();
+ $cases = array();
+
+ foreach ( $instanceCases as $case ) {
+ $cases[] = array( $case[0], true );
+ $cases[] = array( $case[0], false );
+ }
+
+ return $cases;
+ }
+
+ /**
+ * @dataProvider provideSaveAndLoad
+ */
+ public function testSaveAndLoad( ChangeRow $changeRow, $json = false ) {
+ Settings::singleton()->offsetSet( "changesAsJson", $json );
+ $this->assertEquals( $json, Settings::get( "changesAsJson" ) );
// sanity
+
+ $changeRow->save();
+ $id = $changeRow->getId();
+
+ /* @var ChangesTable $table */
+ $table = $this->getTableInstance();
+ $rows = $table->selectObjects( null, array( 'id' => $id ) );
+
+ $this->assertEquals( 1, count( $rows ), "Expected exactly one
object with the given ID" );
+ $loadedRow = reset( $rows );
+
+ $this->verifyFields( $loadedRow, $changeRow->getFields() );
+ }
+
}
diff --git a/lib/tests/phpunit/changes/DiffChangeTest.php
b/lib/tests/phpunit/changes/DiffChangeTest.php
index 3539253..9f6b9fc 100644
--- a/lib/tests/phpunit/changes/DiffChangeTest.php
+++ b/lib/tests/phpunit/changes/DiffChangeTest.php
@@ -28,14 +28,25 @@
* @ingroup WikibaseLib
* @ingroup Test
*
+ * @group Database
* @group Wikibase
* @group WikibaseLib
* @group WikibaseChange
*
* @licence GNU GPL v2+
* @author Jeroen De Dauw < [email protected] >
+ * @author Daniel Kinzler
*/
-class DiffChangeTest extends \MediaWikiTestCase {
+class DiffChangeTest extends ChangeRowTest {
+
+ /**
+ * @see ORMRowTest::getRowClass
+ * @since 0.4
+ * @return string
+ */
+ protected function getRowClass() {
+ return '\Wikibase\DiffChange';
+ }
public function diffProvider() {
$differ = new MapDiffer();
@@ -48,6 +59,23 @@
);
}
+ public function constructorTestProvider() {
+ $data = TestChanges::getChange();
+
+ $diffCases = $this->diffProvider();
+ $cases = array();
+
+ foreach ( $diffCases as $diffCase ) {
+ $case = $data;
+ $diff = $diffCase[0];
+ $case['info']['diff'] = $diff;
+
+ $cases[] = array( $case, true );
+ }
+
+ return $cases;
+ }
+
/**
* @param Diff $diff
* @dataProvider diffProvider
diff --git a/lib/tests/phpunit/changes/TestChanges.php
b/lib/tests/phpunit/changes/TestChanges.php
index c86b73f..ba8fc25 100644
--- a/lib/tests/phpunit/changes/TestChanges.php
+++ b/lib/tests/phpunit/changes/TestChanges.php
@@ -67,7 +67,7 @@
'revision_id' => 452,
'user_id' => 0,
'info' => array(
- 'entity' => self::getItem(),
+ //NOTE: we no longer put Entity objects into
the change, they are just too big.
'metadata' => array(
'rc_user' => 0,
'rc_user_text' => '208.80.152.201'
--
To view, visit https://gerrit.wikimedia.org/r/49500
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9fd62298f6176f3937850cc3e009b22fa7126826
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: mw1.21-wmf10
Gerrit-Owner: Aude <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
