jenkins-bot has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/376781 )
Change subject: Diffing and Patching Lexeme forms and nextFormId ...................................................................... Diffing and Patching Lexeme forms and nextFormId Change-Id: Icfa5d5bfcbe7e48cab9f1d4c99eeba1bd656d39f --- M src/DataModel/Services/Diff/LexemeDiff.php M src/DataModel/Services/Diff/LexemeDiffer.php M src/DataModel/Services/Diff/LexemePatcher.php M tests/phpunit/composer/DataModel/Services/Diff/LexemeDifferPatcherTest.php 4 files changed, 259 insertions(+), 3 deletions(-) Approvals: WMDE-leszek: Looks good to me, approved jenkins-bot: Verified diff --git a/src/DataModel/Services/Diff/LexemeDiff.php b/src/DataModel/Services/Diff/LexemeDiff.php index ee9f4e3..cf452bd 100644 --- a/src/DataModel/Services/Diff/LexemeDiff.php +++ b/src/DataModel/Services/Diff/LexemeDiff.php @@ -20,6 +20,7 @@ * @param DiffOp[] $operations */ public function __construct( array $operations = [] ) { + //TODO Probably can be removed. Does it do anything useful? $this->fixSubstructureDiff( $operations, 'lemmas' ); $this->fixSubstructureDiff( $operations, 'lexicalCategory' ); $this->fixSubstructureDiff( $operations, 'language' ); @@ -56,13 +57,24 @@ } /** + * @return Diff + */ + public function getFormsDiff() { + return isset( $this['forms'] ) ? $this['forms'] : new Diff( [], true ); + } + + /** * Returns if there are any changes (equivalent to: any differences between the entities). * * @return bool */ public function isEmpty() { return $this->getLemmasDiff()->isEmpty() - && $this->getClaimsDiff()->isEmpty(); + && $this->getClaimsDiff()->isEmpty(); + } + + public function getNextFormIdDiff() { + return isset( $this['nextFormId'] ) ? $this['nextFormId'] : new Diff( [], true ); } } diff --git a/src/DataModel/Services/Diff/LexemeDiffer.php b/src/DataModel/Services/Diff/LexemeDiffer.php index 374c147..f33531f 100644 --- a/src/DataModel/Services/Diff/LexemeDiffer.php +++ b/src/DataModel/Services/Diff/LexemeDiffer.php @@ -3,11 +3,14 @@ namespace Wikibase\Lexeme\DataModel\Services\Diff; use Diff\Differ\MapDiffer; +use Diff\DiffOp\Diff\Diff; +use Diff\DiffOp\DiffOpChange; use UnexpectedValueException; use Wikibase\DataModel\Entity\EntityDocument; use Wikibase\DataModel\Services\Diff\EntityDiff; use Wikibase\DataModel\Services\Diff\EntityDifferStrategy; use Wikibase\DataModel\Services\Diff\StatementListDiffer; +use Wikibase\Lexeme\DataModel\Form; use Wikibase\Lexeme\DataModel\Lexeme; use Wikimedia\Assert\Assert; use InvalidArgumentException; @@ -27,9 +30,15 @@ */ private $recursiveMapDiffer; + /** + * @var FormDiffer + */ + private $formDiffer; + public function __construct() { $this->recursiveMapDiffer = new MapDiffer( true ); $this->statementListDiffer = new StatementListDiffer(); + $this->formDiffer = new FormDiffer(); } /** @@ -73,6 +82,13 @@ $diffOps['claim'] = $this->statementListDiffer->getDiff( $from->getStatements(), $to->getStatements() + ); + + $diffOps['nextFormId'] = $this->getNextFormIdCounterDiff( $from, $to ); + + $diffOps['forms'] = $this->getFormsDiff( + $from->getForms(), + $to->getForms() ); return new LexemeDiff( $diffOps ); @@ -132,4 +148,55 @@ return $this->diffEntities( $entity, new Lexeme() ); } + /** + * @param Form[] $from + * @param Form[] $to + * + * @return Diff; + */ + private function getFormsDiff( array $from, array $to ) { + $differ = new MapDiffer(); + + $differ->setComparisonCallback( function ( Form $from, Form $to ) { + return $from == $to; + } ); + + $from = $this->toFormsDiffArray( $from ); + $to = $this->toFormsDiffArray( $to ); + $formDiffOps = $differ->doDiff( $from, $to ); + + foreach ( $formDiffOps as $index => $formDiffOp ) { + if ( $formDiffOp instanceof DiffOpChange ) { + /** @var DiffOpChange $formDiffOp */ + $formDiffOps[$index] = $this->formDiffer->diff( + $formDiffOp->getOldValue(), + $formDiffOp->getNewValue() + ); + + } + } + + return new Diff( $formDiffOps, true ); + } + + /** + * @param Form[] $forms + */ + private function toFormsDiffArray( array $forms ) { + $result = []; + foreach ( $forms as $form ) { + $result[$form->getId()->getSerialization()] = $form; + } + + return $result; + } + + private function getNextFormIdCounterDiff( Lexeme $from, Lexeme $to ) { + if ( $to->getNextFormId() <= $from->getNextFormId() ) { + return new Diff( [] ); + } + + return new Diff( [ new DiffOpChange( $from->getNextFormId(), $to->getNextFormId() ) ] ); + } + } diff --git a/src/DataModel/Services/Diff/LexemePatcher.php b/src/DataModel/Services/Diff/LexemePatcher.php index 0ec031c..1c20b2c 100644 --- a/src/DataModel/Services/Diff/LexemePatcher.php +++ b/src/DataModel/Services/Diff/LexemePatcher.php @@ -6,6 +6,7 @@ use Diff\DiffOp\DiffOpAdd; use Diff\DiffOp\DiffOpChange; use Diff\DiffOp\DiffOpRemove; +use Diff\Patcher\ListPatcher; use Diff\Patcher\PatcherException; use InvalidArgumentException; use Wikibase\DataModel\Entity\EntityDocument; @@ -15,7 +16,9 @@ use Wikibase\DataModel\Services\Diff\StatementListPatcher; use Wikibase\DataModel\Services\Diff\TermListPatcher; use Wikibase\DataModel\Term\TermList; +use Wikibase\Lexeme\DataModel\Form; use Wikibase\Lexeme\DataModel\Lexeme; +use Wikibase\Lexeme\DataModel\LexemePatchAccess; use Wikimedia\Assert\Assert; /** @@ -24,7 +27,7 @@ * @author Thiemo Mättig */ class LexemePatcher implements EntityPatcherStrategy { - + private $formPatcher; /** * @var TermListPatcher */ @@ -38,6 +41,7 @@ public function __construct() { $this->termListPatcher = new TermListPatcher(); $this->statementListPatcher = new StatementListPatcher(); + $this->formPatcher = new FormPatcher(); } /** @@ -57,9 +61,11 @@ */ public function patchEntity( EntityDocument $entity, EntityDiff $patch ) { Assert::parameterType( Lexeme::class, $entity, '$entity' ); - + Assert::parameterType( LexemeDiff::class, $patch, '$patch' ); /** @var Lexeme $entity */ /** @var LexemeDiff $patch */ + + //TODO Lemmas can't be null. Redundant check $lemmas = $entity->getLemmas() !== null ? $entity->getLemmas() : new TermList(); $this->termListPatcher->patchTermList( $lemmas, @@ -81,6 +87,10 @@ if ( $itemId !== false ) { $entity->setLanguage( $itemId ); } + + $this->patchNextFormId( $entity, $patch ); + + $this->patchForms( $entity, $patch ); } /** @@ -112,4 +122,57 @@ throw new PatcherException( 'Invalid ItemId diff' ); } + private function patchNextFormId( Lexeme $entity, LexemeDiff $patch ) { + $nextFormIdDiffList = $patch->getNextFormIdDiff(); + foreach ( $nextFormIdDiffList as $nextFormIdDiff ) { + switch ( get_class( $nextFormIdDiff ) ) { + case DiffOpChange::class: + /** @var DiffOpChange $nextFormIdDiff */ + $newNumber = $nextFormIdDiff->getNewValue(); + if ( $newNumber > $entity->getNextFormId() ) { + $entity->patch( function ( LexemePatchAccess $patchAccess ) use ( $newNumber ) { + $patchAccess->increaseNextFormIdTo( $newNumber ); + } ); + } + break; + default: + throw new PatcherException( 'Invalid forms list diff' ); + } + } + } + + private function patchForms( Lexeme $entity, LexemeDiff $patch ) { + $formsDiff = $patch->getFormsDiff(); + foreach ( $formsDiff as $formDiff ) { + switch ( get_class( $formDiff ) ) { + case DiffOpAdd::class: + /** @var DiffOpAdd $formDiff */ + /** @var Form $form */ + $form = $formDiff->getNewValue(); + $entity->patch( + function ( LexemePatchAccess $patchAccess ) use ( $form ) { + $patchAccess->addForm( $form ); + } + ); + break; + case DiffOpRemove::class: + /** @var DiffOpRemove $formDiff */ + /** @var Form $form */ + $form = $formDiff->getOldValue(); + $entity->removeForm( $form->getId() ); + break; + case ChangeFormDiffOp::class: + /** @var ChangeFormDiffOp $formDiff */ + /** @var Form $form */ + //TODO: This implementation is incomplete/incorrect. + //TODO: Proper implementation of Forms patching is needed + $form = $entity->getForms()[0]; + $this->formPatcher->patch( $form, $formDiff ); + break; + default: + throw new PatcherException( 'Invalid forms list diff: ' . get_class( $formDiff ) ); + } + } + } + } diff --git a/tests/phpunit/composer/DataModel/Services/Diff/LexemeDifferPatcherTest.php b/tests/phpunit/composer/DataModel/Services/Diff/LexemeDifferPatcherTest.php index 92325fb..9632933 100644 --- a/tests/phpunit/composer/DataModel/Services/Diff/LexemeDifferPatcherTest.php +++ b/tests/phpunit/composer/DataModel/Services/Diff/LexemeDifferPatcherTest.php @@ -2,12 +2,17 @@ namespace Wikibase\Lexeme\Tests\DataModel\Services\Diff; +use Wikibase\DataModel\Entity\ItemId; +use Wikibase\Lexeme\DataModel\Form; +use Wikibase\Lexeme\DataModel\FormId; use Eris\Facade; use Wikibase\Lexeme\DataModel\Lexeme; use Wikibase\Lexeme\DataModel\LexemeId; use Wikibase\Lexeme\DataModel\Services\Diff\LexemeDiffer; use Wikibase\Lexeme\DataModel\Services\Diff\LexemePatcher; use Wikibase\Lexeme\Tests\ErisGenerators\WikibaseLexemeGenerators; +use Wikibase\Lexeme\Tests\DataModel\NewForm; +use Wikibase\Lexeme\Tests\DataModel\NewLexeme; /** * @covers \Wikibase\Lexeme\DataModel\Services\Diff\LexemeDiffer @@ -45,4 +50,113 @@ } ); } + public function testAddedFormIsDiffedAndPatched() { + $differ = new LexemeDiffer(); + $patcher = new LexemePatcher(); + $initialLexeme = NewLexeme::create(); + $lexemeWithoutForm = $initialLexeme->build(); + $lexemeWithForm = $initialLexeme->withForm( NewForm::havingId( 'F1' ) )->build(); + + $diff = $differ->diffLexemes( $lexemeWithoutForm, $lexemeWithForm ); + $patcher->patchEntity( $lexemeWithoutForm, $diff ); + + $this->assertTrue( + $lexemeWithoutForm->equals( $lexemeWithForm ), + "Lexemes are not equal" + ); + } + + public function testRemovedFormIsDiffedAndPatched() { + $differ = new LexemeDiffer(); + $patcher = new LexemePatcher(); + $initialLexeme = NewLexeme::create(); + $lexemeWithForm = $initialLexeme->withForm( NewForm::havingId( 'F1' ) )->build(); + $lexemeWithoutForm = $initialLexeme->build(); + + $diff = $differ->diffLexemes( $lexemeWithForm, $lexemeWithoutForm ); + $patcher->patchEntity( $lexemeWithForm, $diff ); + + $this->assertEquals( [], $lexemeWithForm->getForms() ); + } + + public function testDiffAndPatchCanIncreaseNextFormIdCounter() { + $differ = new LexemeDiffer(); + $patcher = new LexemePatcher(); + $initialLexeme = NewLexeme::create(); + $lexemeWithoutForm = $initialLexeme->build(); + $lexemeThatHadForm = $initialLexeme->withForm( NewForm::havingId( 'F1' ) )->build(); + $lexemeThatHadForm->removeForm( new FormId( 'F1' ) ); + + $diff = $differ->diffLexemes( $lexemeWithoutForm, $lexemeThatHadForm ); + $patcher->patchEntity( $lexemeWithoutForm, $diff ); + + $this->assertEquals( + $lexemeThatHadForm->getNextFormId(), + $lexemeWithoutForm->getNextFormId() + ); + $this->assertTrue( + $lexemeWithoutForm->equals( $lexemeThatHadForm ), + "Lexemes are not equal" + ); + } + + public function testDiffAndPatchCanChangeExistingFormRepresentations() { + $differ = new LexemeDiffer(); + $patcher = new LexemePatcher(); + $initialLexeme = NewLexeme::create(); + $lexeme1 = $initialLexeme + ->withForm( + NewForm::havingId( 'F1' ) + ->andRepresentation( 'en', 'cat' ) + )->build(); + $lexeme2 = $initialLexeme + ->withForm( + NewForm::havingId( 'F1' ) + ->andRepresentation( 'en', 'goat' ) + )->build(); + + $diff = $differ->diffLexemes( $lexeme1, $lexeme2 ); + $patcher->patchEntity( $lexeme1, $diff ); + + $this->assertTrue( + $lexeme1->equals( $lexeme2 ), + "Lexemes are not equal" + ); + } + + public function testDiffAndPatchCanAtomicallyChangeExistingFormRepresentations() { + $differ = new LexemeDiffer(); + $patcher = new LexemePatcher(); + $initialLexeme = NewLexeme::create(); + $lexeme1 = $initialLexeme + ->withForm( + NewForm::havingId( 'F1' ) + ->andRepresentation( 'en', 'en-value' ) + )->build(); + $lexeme2 = $initialLexeme + ->withForm( + NewForm::havingId( 'F1' ) + ->andRepresentation( 'en', 'en-value' ) + ->andRepresentation( 'fr', 'fr-value' ) + )->build(); + $latestLexeme = $initialLexeme + ->withForm( + NewForm::havingId( 'F1' ) + ->andRepresentation( 'de', 'de-value' ) + )->build(); + + $diff = $differ->diffLexemes( $lexeme1, $lexeme2 ); + $patcher->patchEntity( $latestLexeme, $diff ); + + $form = $latestLexeme->getForms()[0]; + $this->assertEquals( + 'fr-value', + $form->getRepresentations()->getByLanguage( 'fr' )->getText() + ); + $this->assertEquals( + 'de-value', + $form->getRepresentations()->getByLanguage( 'de' )->getText() + ); + } + } -- To view, visit https://gerrit.wikimedia.org/r/376781 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Icfa5d5bfcbe7e48cab9f1d4c99eeba1bd656d39f Gerrit-PatchSet: 17 Gerrit-Project: mediawiki/extensions/WikibaseLexeme Gerrit-Branch: master Gerrit-Owner: Aleksey Bekh-Ivanov (WMDE) <aleksey.bekh-iva...@wikimedia.de> Gerrit-Reviewer: Aleksey Bekh-Ivanov (WMDE) <aleksey.bekh-iva...@wikimedia.de> Gerrit-Reviewer: Jonas Kress (WMDE) <jonas.kr...@wikimedia.de> Gerrit-Reviewer: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de> Gerrit-Reviewer: WMDE-leszek <leszek.mani...@wikimedia.de> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits