Page MenuHomePhabricator
Files F41826373

0001-Use-EditEntity-for-ItemMergeInteractor.patch
Lucas_Werkmeister_WMDE (Lucas Werkmeister (WMDE))
Actions

Authored By
Lucas_Werkmeister_WMDE
Feb 9 2024, 11:22 AM
Size
12 KB
Referenced Files
None
Subscribers
None

0001-Use-EditEntity-for-ItemMergeInteractor.patch
View Options

From 6bc39b670b802e1beb2c2ac9267c203aae431ec2 Mon Sep 17 00:00:00 2001
From: Lucas Werkmeister <lucas.werkmeister@wikimedia.de>
Date: Tue, 6 Feb 2024 17:04:02 +0100
Subject: [PATCH] Use EditEntity for ItemMergeInteractor
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This means less duplicated code, and also makes dealing with IP Masking
and Temporary Users easier – we should still get the savedTempUser from
the returned status, but let’s do that separately.
Note that we still have to check permissions, because EditEntity only
checks “edit” permissions, not “merge” permissions, and that’s currently
not configurable (addRequiredPermission() was removed in change
Ibfb8b4c224, commit 2af5acfcef). I’d also be okay with making the
permission configurable in EditEntity again.
In the tests, I decided to use a real MediaWikiEditEntity rather than
mocking it, mainly because it means fewer changes to the test are
necessary – we can still use the same mocked permission checker and just
plug it into MediaWikiEditEntity, for instance. But I think it also
makes some sense to use the real service for the integration test here.
Bug: T356149
Bug: T356764
Change-Id: I919db202f9c733f01c6900bd1ebe00826bd85aca
---
repo/WikibaseRepo.ServiceWiring.php | 5 +-
.../Interactors/ItemMergeInteractor.php | 57 +++++++------------
.../Interactors/ItemMergeInteractorTest.php | 50 +++++++++++-----
.../ServiceWiring/ItemMergeInteractorTest.php | 9 +--
4 files changed, 62 insertions(+), 59 deletions(-)
diff --git a/repo/WikibaseRepo.ServiceWiring.php b/repo/WikibaseRepo.ServiceWiring.php
index 4a5341623f..706c48e08d 100644
--- a/repo/WikibaseRepo.ServiceWiring.php
+++ b/repo/WikibaseRepo.ServiceWiring.php
@@ -1304,13 +1304,12 @@ function ( EntityNamespaceLookup $nsLookup, DatabaseEntitySource $source ): Enti
->getMergeFactory(),
WikibaseRepo::getStore( $services )
->getEntityRevisionLookup( Store::LOOKUP_CACHING_DISABLED ),
- WikibaseRepo::getEntityStore( $services ),
+ WikibaseRepo::getEditEntityFactory( $services ),
WikibaseRepo::getEntityPermissionChecker( $services ),
WikibaseRepo::getSummaryFormatter( $services ),
WikibaseRepo::getItemRedirectCreationInteractor( $services ),
WikibaseRepo::getEntityTitleStoreLookup( $services ),
- $services->getPermissionManager(),
- WikibaseRepo::getEditFilterHookRunner( $services )
+ $services->getPermissionManager()
);
},
diff --git a/repo/includes/Interactors/ItemMergeInteractor.php b/repo/includes/Interactors/ItemMergeInteractor.php
index 44af309f40..219f6f1348 100644
--- a/repo/includes/Interactors/ItemMergeInteractor.php
+++ b/repo/includes/Interactors/ItemMergeInteractor.php
@@ -12,7 +12,6 @@
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\Lib\FormatableSummary;
use Wikibase\Lib\Store\EntityRevisionLookup;
-use Wikibase\Lib\Store\EntityStore;
use Wikibase\Lib\Store\LookupConstants;
use Wikibase\Lib\Store\RevisionedUnresolvedRedirectException;
use Wikibase\Lib\Store\StorageException;
@@ -20,7 +19,7 @@
use Wikibase\Repo\ChangeOp\ChangeOpException;
use Wikibase\Repo\ChangeOp\ChangeOpsMerge;
use Wikibase\Repo\Content\EntityContent;
-use Wikibase\Repo\EditEntity\EditFilterHookRunner;
+use Wikibase\Repo\EditEntity\MediaWikiEditEntityFactory;
use Wikibase\Repo\Merge\MergeFactory;
use Wikibase\Repo\Store\EntityPermissionChecker;
use Wikibase\Repo\Store\EntityTitleStoreLookup;
@@ -45,9 +44,9 @@ class ItemMergeInteractor {
private $entityRevisionLookup;
/**
- * @var EntityStore
+ * @var MediaWikiEditEntityFactory
*/
- private $entityStore;
+ private $editEntityFactory;
/**
* @var EntityPermissionChecker
@@ -74,32 +73,30 @@ class ItemMergeInteractor {
*/
private $permissionManager;
- private EditFilterHookRunner $editFilterHookRunner;
-
public function __construct(
MergeFactory $mergeFactory,
EntityRevisionLookup $entityRevisionLookup,
- EntityStore $entityStore,
+ MediaWikiEditEntityFactory $editEntityFactory,
EntityPermissionChecker $permissionChecker,
SummaryFormatter $summaryFormatter,
ItemRedirectCreationInteractor $interactorRedirect,
EntityTitleStoreLookup $entityTitleLookup,
- PermissionManager $permissionManager,
- EditFilterHookRunner $editFilterHookRunner
+ PermissionManager $permissionManager
) {
$this->mergeFactory = $mergeFactory;
$this->entityRevisionLookup = $entityRevisionLookup;
- $this->entityStore = $entityStore;
+ $this->editEntityFactory = $editEntityFactory;
$this->permissionChecker = $permissionChecker;
$this->summaryFormatter = $summaryFormatter;
$this->interactorRedirect = $interactorRedirect;
$this->entityTitleLookup = $entityTitleLookup;
$this->permissionManager = $permissionManager;
- $this->editFilterHookRunner = $editFilterHookRunner;
}
/**
- * Check user's for the given entity ID.
+ * Check user's merge permissions for the given entity ID.
+ * (Note that this is not redundant with the check in EditEntity later,
+ * because that checks edit permissions, not merge.)
*
* @param EntityId $entityId
*
@@ -119,14 +116,6 @@ private function checkPermissions( EntityId $entityId, User $user ) {
}
}
- private function checkRateLimits( User $user ): void {
- if ( $user->pingLimiter( 'edit', 2 ) ) { // attemptSaveMerge() makes two edits
- // use generic 'failed-save' because ItemMergeException prepends 'wikibase-itemmerge-' for the message key,
- // so we can’t easily use the correct actionthrottledtext message (using Status would solve this)
- throw new ItemMergeException( 'rate limit hit', 'failed-save' );
- }
- }
-
/**
* Merges the content of the first item into the second and creates a redirect if the first item
* is empty after the merge.
@@ -162,7 +151,6 @@ public function mergeItems(
$user = $context->getUser();
$this->checkPermissions( $fromId, $user );
$this->checkPermissions( $toId, $user );
- $this->checkRateLimits( $user );
/**
* @var Item $fromItem
@@ -293,37 +281,32 @@ private function attemptSaveMerge( Item $fromItem, Item $toItem, ?string $summar
}
private function saveItem( Item $item, FormatableSummary $summary, IContextSource $context, bool $bot, array $tags ) {
- $user = $context->getUser();
-
// Given we already check all constraints in ChangeOpsMerge, it's
// fine to ignore them here. This is also needed to not run into
// the constraints we're supposed to ignore (see ChangeOpsMerge::removeConflictsWithEntity
// for reference)
$flags = EDIT_UPDATE | EntityContent::EDIT_IGNORE_CONSTRAINTS;
- if ( $bot && $this->permissionManager->userHasRight( $user, 'bot' ) ) {
+ if ( $bot && $this->permissionManager->userHasRight( $context->getUser(), 'bot' ) ) {
$flags |= EDIT_FORCE_BOT;
}
$formattedSummary = $this->summaryFormatter->formatSummary( $summary );
- $status = $this->editFilterHookRunner->run( $item, $context, $formattedSummary );
+ $editEntity = $this->editEntityFactory->newEditEntity( $context, $item->getId() );
+ $status = $editEntity->attemptSave(
+ $item,
+ $formattedSummary,
+ $flags,
+ false,
+ null,
+ $tags
+ );
if ( !$status->isOK() ) {
// as in checkPermissions() above, it would be better to just pass the Status to the API
throw new ItemMergeException( $status->getWikiText(), 'failed-save' );
}
- try {
- return $this->entityStore->saveEntity(
- $item,
- $formattedSummary,
- $user,
- $flags,
- false,
- $tags
- );
- } catch ( StorageException $ex ) {
- throw new ItemMergeException( $ex->getMessage(), 'failed-save', $ex );
- }
+ return $status->getValue()['revision'];
}
private function updateWatchlistEntries( ItemId $fromId, ItemId $toId ) {
diff --git a/repo/tests/phpunit/includes/Interactors/ItemMergeInteractorTest.php b/repo/tests/phpunit/includes/Interactors/ItemMergeInteractorTest.php
index 46ff498305..cf6ca9817c 100644
--- a/repo/tests/phpunit/includes/Interactors/ItemMergeInteractorTest.php
+++ b/repo/tests/phpunit/includes/Interactors/ItemMergeInteractorTest.php
@@ -10,15 +10,19 @@
use MediaWiki\Title\Title;
use MediaWiki\User\User;
use MediaWikiIntegrationTestCase;
+use NullStatsdDataFactory;
use RequestContext;
use TestSites;
use Wikibase\DataModel\Entity\EntityId;
use Wikibase\DataModel\Entity\ItemId;
+use Wikibase\DataModel\Services\Diff\EntityDiffer;
+use Wikibase\DataModel\Services\Diff\EntityPatcher;
use Wikibase\Lib\Store\LatestRevisionIdResult;
use Wikibase\Lib\Store\RevisionedUnresolvedRedirectException;
use Wikibase\Lib\Tests\MockRepository;
use Wikibase\Repo\Content\ItemContent;
use Wikibase\Repo\EditEntity\EditFilterHookRunner;
+use Wikibase\Repo\EditEntity\MediaWikiEditEntityFactory;
use Wikibase\Repo\Interactors\ItemMergeException;
use Wikibase\Repo\Interactors\ItemMergeInteractor;
use Wikibase\Repo\Interactors\ItemRedirectCreationInteractor;
@@ -104,14 +108,17 @@ public function getMockEditFilterHookRunner() {
private function getPermissionChecker() {
$permissionChecker = $this->createMock( EntityPermissionChecker::class );
+ $callback = function ( User $user ) {
+ if ( $user->getName() === self::USER_NAME_WITHOUT_PERMISSION ) {
+ return Status::newFatal( 'permissiondenied' );
+ } else {
+ return Status::newGood();
+ }
+ };
$permissionChecker->method( 'getPermissionStatusForEntityId' )
- ->willReturnCallback( function( User $user ) {
- if ( $user->getName() === self::USER_NAME_WITHOUT_PERMISSION ) {
- return Status::newFatal( 'permissiondenied' );
- } else {
- return Status::newGood();
- }
- } );
+ ->willReturnCallback( $callback );
+ $permissionChecker->method( 'getPermissionStatusForEntity' )
+ ->willReturnCallback( $callback );
return $permissionChecker;
}
@@ -158,24 +165,41 @@ private function newInteractor() {
);
$editFilterHookRunner = $this->getMockEditFilterHookRunner();
+
+ $entityTitleLookup = $this->getMockEntityTitleLookup();
+ $permissionChecker = $this->getPermissionChecker();
+ $editEntityFactory = new MediaWikiEditEntityFactory(
+ $entityTitleLookup,
+ $this->mockRepository,
+ $this->mockRepository,
+ $permissionChecker,
+ new EntityDiffer(),
+ new EntityPatcher(),
+ $editFilterHookRunner,
+ new NullStatsdDataFactory(),
+ MediaWikiServices::getInstance()->getUserOptionsLookup(),
+ MediaWikiServices::getInstance()->getTempUserCreator(),
+ 1024 * 1024,
+ [ 'item' ]
+ );
+
$interactor = new ItemMergeInteractor(
$mergeFactory,
$this->mockRepository,
- $this->mockRepository,
- $this->getPermissionChecker(),
+ $editEntityFactory,
+ $permissionChecker,
$summaryFormatter,
new ItemRedirectCreationInteractor(
$this->mockRepository,
$this->mockRepository,
- $this->getPermissionChecker(),
+ $permissionChecker,
$summaryFormatter,
$editFilterHookRunner,
$this->mockRepository,
- $this->getMockEntityTitleLookup()
+ $entityTitleLookup
),
$this->getEntityTitleLookup(),
- MediaWikiServices::getInstance()->getPermissionManager(),
- $editFilterHookRunner
+ MediaWikiServices::getInstance()->getPermissionManager()
);
return $interactor;
diff --git a/repo/tests/phpunit/unit/ServiceWiring/ItemMergeInteractorTest.php b/repo/tests/phpunit/unit/ServiceWiring/ItemMergeInteractorTest.php
index e8f6a27450..96c68ccdf2 100644
--- a/repo/tests/phpunit/unit/ServiceWiring/ItemMergeInteractorTest.php
+++ b/repo/tests/phpunit/unit/ServiceWiring/ItemMergeInteractorTest.php
@@ -4,9 +4,8 @@
namespace Wikibase\Repo\Tests\Unit\ServiceWiring;
use Wikibase\Lib\Store\EntityRevisionLookup;
-use Wikibase\Lib\Store\EntityStore;
use Wikibase\Repo\ChangeOp\ChangeOpFactoryProvider;
-use Wikibase\Repo\EditEntity\EditFilterHookRunner;
+use Wikibase\Repo\EditEntity\MediaWikiEditEntityFactory;
use Wikibase\Repo\Interactors\ItemMergeInteractor;
use Wikibase\Repo\Interactors\ItemRedirectCreationInteractor;
use Wikibase\Repo\Merge\MergeFactory;
@@ -31,8 +30,8 @@ public function testConstruction(): void {
=> $this->getMockChangeOpFactoryProvider(),
'WikibaseRepo.Store'
=> $this->getMockStore(),
- 'WikibaseRepo.EntityStore'
- => $this->createMock( EntityStore::class ),
+ 'WikibaseRepo.EditEntityFactory'
+ => $this->createMock( MediaWikiEditEntityFactory::class ),
'WikibaseRepo.EntityPermissionChecker'
=> $this->createMock( EntityPermissionChecker::class ),
'WikibaseRepo.SummaryFormatter'
@@ -41,8 +40,6 @@ public function testConstruction(): void {
=> $this->createMock( ItemRedirectCreationInteractor::class ),
'WikibaseRepo.EntityTitleStoreLookup'
=> $this->createMock( EntityTitleStoreLookup::class ),
- 'WikibaseRepo.EditFilterHookRunner'
- => $this->createMock( EditFilterHookRunner::class ),
] );
$this->serviceContainer
--
2.43.0

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
14562286
Default Alt Text
0001-Use-EditEntity-for-ItemMergeInteractor.patch (12 KB)

Event Timeline

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits