From 0b1919af0c3d1d45c6ca88109deaf5d2c0ebe3c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Tue, 5 Nov 2024 02:05:17 +0100 Subject: [PATCH] Improve handling of Parsoid resource limit exceeded exceptions Follow-up to 3d9585acbe118e5a23aacaa682efce6032fa1890. * Avoid deprecated method StatusValue::getErrors() * Don't use exceptions for control flow * Don't re-use Parsoid's ResourceLimitExceededException, it seems unexpected to me and might limit future refactoring * Use dieStatus() instead of dieWithException(), which will produce better localised error messages * Add missing error handling in ApiDiscussionToolsThank Change-Id: Ia032025f92f1c3cc0d62a0f45925dddba93fb42f --- includes/ApiDiscussionToolsCompare.php | 15 ++++++----- includes/ApiDiscussionToolsPageInfo.php | 9 +++---- includes/ApiDiscussionToolsThank.php | 8 +++--- includes/ContentThreadItemSetStatus.php | 29 ++++++++++++++++++++++ includes/Hooks/DataUpdatesHooks.php | 2 +- includes/Hooks/EchoHooks.php | 4 --- includes/Hooks/HookUtils.php | 23 ++++++++--------- includes/Notifications/EventDispatcher.php | 9 +------ maintenance/persistRevisionThreadItems.php | 2 +- 9 files changed, 60 insertions(+), 41 deletions(-) create mode 100644 includes/ContentThreadItemSetStatus.php diff --git a/includes/ApiDiscussionToolsCompare.php b/includes/ApiDiscussionToolsCompare.php index 846ec19a4..451b7a911 100644 --- a/includes/ApiDiscussionToolsCompare.php +++ b/includes/ApiDiscussionToolsCompare.php @@ -11,7 +11,6 @@ use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Title\Title; use Wikimedia\ParamValidator\ParamValidator; -use Wikimedia\Parsoid\Core\ResourceLimitExceededException; class ApiDiscussionToolsCompare extends ApiBase { @@ -89,12 +88,16 @@ class ApiDiscussionToolsCompare extends ApiBase { return; } - try { - $fromItemSet = HookUtils::parseRevisionParsoidHtml( $fromRev, __METHOD__ ); - $toItemSet = HookUtils::parseRevisionParsoidHtml( $toRev, __METHOD__ ); - } catch ( ResourceLimitExceededException $e ) { - $this->dieWithException( $e ); + $fromStatus = HookUtils::parseRevisionParsoidHtml( $fromRev, __METHOD__ ); + $toStatus = HookUtils::parseRevisionParsoidHtml( $toRev, __METHOD__ ); + if ( !$fromStatus->isOK() ) { + $this->dieStatus( $fromStatus ); } + if ( !$toStatus->isOK() ) { + $this->dieStatus( $toStatus ); + } + $fromItemSet = $fromStatus->getValueOrThrow(); + $toItemSet = $toStatus->getValueOrThrow(); $removedComments = []; foreach ( $fromItemSet->getCommentItems() as $fromComment ) { diff --git a/includes/ApiDiscussionToolsPageInfo.php b/includes/ApiDiscussionToolsPageInfo.php index e50015c1c..f7e32954d 100644 --- a/includes/ApiDiscussionToolsPageInfo.php +++ b/includes/ApiDiscussionToolsPageInfo.php @@ -15,7 +15,6 @@ use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Title\Title; use Wikimedia\ParamValidator\ParamValidator; -use Wikimedia\Parsoid\Core\ResourceLimitExceededException; use Wikimedia\Parsoid\DOM\Element; use Wikimedia\Parsoid\DOM\Text; use Wikimedia\Parsoid\Utils\DOMUtils; @@ -117,11 +116,11 @@ class ApiDiscussionToolsPageInfo extends ApiBase { $this->dieWithError( [ 'apierror-missingcontent-revid', $revision->getId() ], 'missingcontent' ); } - try { - return HookUtils::parseRevisionParsoidHtml( $revision, __METHOD__ ); - } catch ( ResourceLimitExceededException $e ) { - $this->dieWithException( $e ); + $status = HookUtils::parseRevisionParsoidHtml( $revision, __METHOD__ ); + if ( !$status->isOK() ) { + $this->dieStatus( $status ); } + return $status->getValueOrThrow(); } /** diff --git a/includes/ApiDiscussionToolsThank.php b/includes/ApiDiscussionToolsThank.php index ba2cb0062..787de57c5 100644 --- a/includes/ApiDiscussionToolsThank.php +++ b/includes/ApiDiscussionToolsThank.php @@ -16,7 +16,6 @@ use MediaWiki\Revision\RevisionLookup; use MediaWiki\Title\Title; use MediaWiki\User\UserFactory; use Wikimedia\ParamValidator\ParamValidator; -use Wikimedia\Parsoid\Core\ResourceLimitExceededException; /** * API module to send DiscussionTools comment thanks notifications @@ -49,7 +48,6 @@ class ApiDiscussionToolsThank extends ApiThank { /** * @inheritDoc * @throws ApiUsageException - * @throws ResourceLimitExceededException */ public function execute() { $user = $this->getUser(); @@ -77,7 +75,11 @@ class ApiDiscussionToolsThank extends ApiThank { 'nosuchrevid' ); } - $threadItemSet = HookUtils::parseRevisionParsoidHtml( $revision, __METHOD__ ); + $status = HookUtils::parseRevisionParsoidHtml( $revision, __METHOD__ ); + if ( !$status->isOK() ) { + $this->dieStatus( $status ); + } + $threadItemSet = $status->getValueOrThrow(); $comment = $threadItemSet->findCommentById( $commentId ); diff --git a/includes/ContentThreadItemSetStatus.php b/includes/ContentThreadItemSetStatus.php new file mode 100644 index 000000000..f366d05cd --- /dev/null +++ b/includes/ContentThreadItemSetStatus.php @@ -0,0 +1,29 @@ +merge( $other, true ); + } + + /** + * Like getValue(), but will throw if the value is null. Check isOK() first to avoid errors. + */ + public function getValueOrThrow(): ContentThreadItemSet { + $value = $this->getValue(); + if ( $value === null ) { + throw new NormalizedException( ...Status::wrap( $this )->getPsr3MessageAndContext() ); + } + return $value; + } + +} diff --git a/includes/Hooks/DataUpdatesHooks.php b/includes/Hooks/DataUpdatesHooks.php index 94a6ff5f0..3303e38bb 100644 --- a/includes/Hooks/DataUpdatesHooks.php +++ b/includes/Hooks/DataUpdatesHooks.php @@ -45,7 +45,7 @@ class DataUpdatesHooks implements RevisionDataUpdatesHook { $method = __METHOD__; $updates[] = new MWCallableUpdate( function () use ( $rev, $method ) { try { - $threadItemSet = HookUtils::parseRevisionParsoidHtml( $rev, $method ); + $threadItemSet = HookUtils::parseRevisionParsoidHtml( $rev, $method )->getValueOrThrow(); if ( !$this->threadItemStore->isDisabled() ) { $this->threadItemStore->insertThreadItems( $rev, $threadItemSet ); } diff --git a/includes/Hooks/EchoHooks.php b/includes/Hooks/EchoHooks.php index 8e4dc9fbf..46713c92b 100644 --- a/includes/Hooks/EchoHooks.php +++ b/includes/Hooks/EchoHooks.php @@ -23,7 +23,6 @@ use MediaWiki\Extension\Notifications\Model\Event; use MediaWiki\Extension\Notifications\UserLocator; use MediaWiki\Registration\ExtensionRegistry; use MediaWiki\Revision\RevisionRecord; -use Wikimedia\Parsoid\Core\ResourceLimitExceededException; class EchoHooks implements BeforeCreateEchoEventHook, @@ -143,9 +142,6 @@ class EchoHooks implements } } - /** - * @throws ResourceLimitExceededException - */ public function onEchoGetEventsForRevision( array &$events, RevisionRecord $revision, bool $isRevert ) { if ( $isRevert ) { return; diff --git a/includes/Hooks/HookUtils.php b/includes/Hooks/HookUtils.php index 4fb807b96..242ee5e8d 100644 --- a/includes/Hooks/HookUtils.php +++ b/includes/Hooks/HookUtils.php @@ -14,7 +14,7 @@ use MediaWiki\Context\IContextSource; use MediaWiki\Context\RequestContext; use MediaWiki\Extension\DiscussionTools\CommentParser; use MediaWiki\Extension\DiscussionTools\CommentUtils; -use MediaWiki\Extension\DiscussionTools\ContentThreadItemSet; +use MediaWiki\Extension\DiscussionTools\ContentThreadItemSetStatus; use MediaWiki\Linker\LinkTarget; use MediaWiki\MediaWikiServices; use MediaWiki\Output\OutputPage; @@ -22,12 +22,12 @@ use MediaWiki\Page\ParserOutputAccess; use MediaWiki\Parser\ParserOptions; use MediaWiki\Registration\ExtensionRegistry; use MediaWiki\Revision\RevisionRecord; +use MediaWiki\Status\Status; use MediaWiki\Title\Title; use MediaWiki\Title\TitleValue; use MediaWiki\User\UserIdentity; -use RuntimeException; use Wikimedia\Assert\Assert; -use Wikimedia\Parsoid\Core\ResourceLimitExceededException; +use Wikimedia\NormalizedException\NormalizedException; use Wikimedia\Parsoid\Utils\DOMCompat; use Wikimedia\Parsoid\Utils\DOMUtils; use Wikimedia\Rdbms\IDBAccessObject; @@ -118,13 +118,12 @@ class HookUtils { * Otherwise, it should be set to the name of the calling method (__METHOD__), * so we can track what is causing parser cache writes. * - * @return ContentThreadItemSet - * @throws ResourceLimitExceededException + * @return ContentThreadItemSetStatus */ public static function parseRevisionParsoidHtml( RevisionRecord $revRecord, $updateParserCacheFor - ): ContentThreadItemSet { + ): ContentThreadItemSetStatus { $services = MediaWikiServices::getInstance(); $mainConfig = $services->getMainConfig(); $parserOutputAccess = $services->getParserOutputAccess(); @@ -152,14 +151,12 @@ class HookUtils { ); if ( !$status->isOK() ) { - [ 'message' => $key, 'params' => $params ] = $status->getErrors()[0]; - // XXX: HtmlOutputRendererHelper checks for a resource limit exceeded message as well, - // maybe we shouldn't be catching that so low down. Re-throw for callers. + // This is currently the only expected failure, make the caller handle it if ( $status->hasMessage( 'parsoid-resource-limit-exceeded' ) ) { - throw new ResourceLimitExceededException( $params[0] ?? '' ); + return ContentThreadItemSetStatus::wrap( $status ); } - $message = wfMessage( $key, ...$params ); - throw new RuntimeException( $message->inLanguage( 'en' )->useDatabase( false )->text() ); + // Any other failures indicate a software bug, so throw an exception + throw new NormalizedException( ...Status::wrap( $status )->getPsr3MessageAndContext() ); } $parserOutput = $status->getValue(); @@ -176,7 +173,7 @@ class HookUtils { /** @var CommentParser $parser */ $parser = $services->getService( 'DiscussionTools.CommentParser' ); $title = TitleValue::newFromPage( $revRecord->getPage() ); - return $parser->parse( $container, $title ); + return ContentThreadItemSetStatus::newGood( $parser->parse( $container, $title ) ); } /** diff --git a/includes/Notifications/EventDispatcher.php b/includes/Notifications/EventDispatcher.php index 60d469769..1fc4a2f98 100644 --- a/includes/Notifications/EventDispatcher.php +++ b/includes/Notifications/EventDispatcher.php @@ -34,22 +34,15 @@ use MediaWiki\Revision\RevisionRecord; use MediaWiki\Title\Title; use MediaWiki\User\UserIdentity; use Wikimedia\Assert\Assert; -use Wikimedia\Parsoid\Core\ResourceLimitExceededException; use Wikimedia\Parsoid\Utils\DOMCompat; use Wikimedia\Parsoid\Utils\DOMUtils; use Wikimedia\Rdbms\IDBAccessObject; class EventDispatcher { - /** - * @throws ResourceLimitExceededException - */ private static function getParsedRevision( RevisionRecord $revRecord ): ContentThreadItemSet { - return HookUtils::parseRevisionParsoidHtml( $revRecord, __METHOD__ ); + return HookUtils::parseRevisionParsoidHtml( $revRecord, __METHOD__ )->getValueOrThrow(); } - /** - * @throws ResourceLimitExceededException - */ public static function generateEventsForRevision( array &$events, RevisionRecord $newRevRecord ): void { $services = MediaWikiServices::getInstance(); diff --git a/maintenance/persistRevisionThreadItems.php b/maintenance/persistRevisionThreadItems.php index 5c009888e..bcdceec8e 100644 --- a/maintenance/persistRevisionThreadItems.php +++ b/maintenance/persistRevisionThreadItems.php @@ -199,7 +199,7 @@ class PersistRevisionThreadItems extends Maintenance { $rev->getPageAsLinkTarget() ); if ( HookUtils::isAvailableForTitle( $title ) ) { - $threadItemSet = HookUtils::parseRevisionParsoidHtml( $rev, false ); + $threadItemSet = HookUtils::parseRevisionParsoidHtml( $rev, false )->getValueOrThrow(); // Store permalink data (even when store is disabled - T334258) $changed = $this->itemStore->insertThreadItems( $rev, $threadItemSet );