From db67de7fad899de33d4c8e3b6ee8189f8570d826 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Thu, 20 Dec 2018 09:59:02 -0500 Subject: [PATCH] ApiDelete: Handle batched deletions properly When batched deletions via the job queue were added in Ie800fb5a, the way this was reported caused ApiDelete to report an error. Instead it should report success with appropriate signaling to the client. Bug: T212356 Change-Id: I1ef66277e988572c6720cf3e3cb36b18530746b4 --- RELEASE-NOTES-1.33 | 4 ++- includes/api/ApiBase.php | 9 +++-- includes/api/ApiDelete.php | 11 ++++-- includes/api/ApiErrorFormatter.php | 7 ++-- tests/phpunit/includes/api/ApiDeleteTest.php | 29 ++++++++++++++++ .../includes/api/ApiErrorFormatterTest.php | 34 +++++++++++++++++++ 6 files changed, 87 insertions(+), 7 deletions(-) diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33 index 5b6f6b2120..ce95bd61d2 100644 --- a/RELEASE-NOTES-1.33 +++ b/RELEASE-NOTES-1.33 @@ -75,7 +75,9 @@ production. exceptions will now include the "Wikimedia\Rdbms\" prefix in the class name. * The code including an exception class name is deprecated. In the future, all internal errors will use code "internal_api_error". -* … +* (T212356) When using action=delete on pages with many revisions, the module + may return a boolean-true 'scheduled' and no 'logid'. This signifies that the + deletion will be processed via the job queue. === Action API internal changes in 1.33 === * A number of deprecated methods for API documentation, intended for overriding diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index d29131b6cc..1efd747f80 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -1949,9 +1949,14 @@ abstract class ApiBase extends ContextSource { * @since 1.29 * @param StatusValue $status * @param string[] $types 'warning' and/or 'error' + * @param string[] $filter Message keys to filter out (since 1.33) */ - public function addMessagesFromStatus( StatusValue $status, $types = [ 'warning', 'error' ] ) { - $this->getErrorFormatter()->addMessagesFromStatus( $this->getModulePath(), $status, $types ); + public function addMessagesFromStatus( + StatusValue $status, $types = [ 'warning', 'error' ], array $filter = [] + ) { + $this->getErrorFormatter()->addMessagesFromStatus( + $this->getModulePath(), $status, $types, $filter + ); } /** diff --git a/includes/api/ApiDelete.php b/includes/api/ApiDelete.php index ec857b7da9..7e8041d667 100644 --- a/includes/api/ApiDelete.php +++ b/includes/api/ApiDelete.php @@ -75,9 +75,10 @@ class ApiDelete extends ApiBase { $status = self::delete( $pageObj, $user, $reason, $params['tags'] ); } - if ( !$status->isGood() ) { + if ( !$status->isOk() ) { $this->dieStatus( $status ); } + $this->addMessagesFromStatus( $status, [ 'warning' ], [ 'delete-scheduled' ] ); // Deprecated parameters if ( $params['watch'] ) { @@ -92,8 +93,14 @@ class ApiDelete extends ApiBase { $r = [ 'title' => $titleObj->getPrefixedText(), 'reason' => $reason, - 'logid' => $status->value ]; + if ( $status->hasMessage( 'delete-scheduled' ) ) { + $r['scheduled'] = true; + } + if ( $status->value !== null ) { + // Scheduled deletions don't currently have a log entry available at this point + $r['logid'] = $status->value; + } $this->getResult()->addValue( null, $this->getModuleName(), $r ); } diff --git a/includes/api/ApiErrorFormatter.php b/includes/api/ApiErrorFormatter.php index a37ecc2df9..9669464733 100644 --- a/includes/api/ApiErrorFormatter.php +++ b/includes/api/ApiErrorFormatter.php @@ -153,9 +153,10 @@ class ApiErrorFormatter { * @param string|null $modulePath * @param StatusValue $status * @param string[]|string $types 'warning' and/or 'error' + * @param string[] $filter Messages to filter out (since 1.33) */ public function addMessagesFromStatus( - $modulePath, StatusValue $status, $types = [ 'warning', 'error' ] + $modulePath, StatusValue $status, $types = [ 'warning', 'error' ], array $filter = [] ) { if ( $status->isGood() || !$status->getErrors() ) { return; @@ -178,7 +179,9 @@ class ApiErrorFormatter { ->inLanguage( $this->lang ) ->title( $this->getDummyTitle() ) ->useDatabase( $this->useDB ); - $this->addWarningOrError( $tag, $modulePath, $msg ); + if ( !in_array( $msg->getKey(), $filter, true ) ) { + $this->addWarningOrError( $tag, $modulePath, $msg ); + } } } diff --git a/tests/phpunit/includes/api/ApiDeleteTest.php b/tests/phpunit/includes/api/ApiDeleteTest.php index fc546ffbe9..803eefb498 100644 --- a/tests/phpunit/includes/api/ApiDeleteTest.php +++ b/tests/phpunit/includes/api/ApiDeleteTest.php @@ -41,6 +41,35 @@ class ApiDeleteTest extends ApiTestCase { $this->assertFalse( Title::newFromText( $name )->exists() ); } + public function testBatchedDelete() { + $this->setMwGlobals( 'wgDeleteRevisionsBatchSize', 1 ); + + $name = 'Help:' . ucfirst( __FUNCTION__ ); + for ( $i = 1; $i <= 3; $i++ ) { + $this->editPage( $name, "Revision $i" ); + } + + $apiResult = $this->doApiRequestWithToken( [ + 'action' => 'delete', + 'title' => $name, + ] )[0]; + + $this->assertArrayHasKey( 'delete', $apiResult ); + $this->assertArrayHasKey( 'title', $apiResult['delete'] ); + $this->assertSame( $name, $apiResult['delete']['title'] ); + $this->assertArrayHasKey( 'scheduled', $apiResult['delete'] ); + $this->assertTrue( $apiResult['delete']['scheduled'] ); + $this->assertArrayNotHasKey( 'logid', $apiResult['delete'] ); + + // Run the jobs + JobQueueGroup::destroySingletons(); + $jobs = new RunJobs; + $jobs->loadParamsAndArgs( null, [ 'quiet' => true ], null ); + $jobs->execute(); + + $this->assertFalse( Title::newFromText( $name )->exists( Title::GAID_FOR_UPDATE ) ); + } + public function testDeleteNonexistent() { $this->setExpectedException( ApiUsageException::class, "The page you specified doesn't exist." ); diff --git a/tests/phpunit/includes/api/ApiErrorFormatterTest.php b/tests/phpunit/includes/api/ApiErrorFormatterTest.php index 312ef55e8f..d7628e0ccf 100644 --- a/tests/phpunit/includes/api/ApiErrorFormatterTest.php +++ b/tests/phpunit/includes/api/ApiErrorFormatterTest.php @@ -634,6 +634,40 @@ class ApiErrorFormatterTest extends MediaWikiLangTestCase { ]; } + public function testAddMessagesFromStatus_filter() { + $result = new ApiResult( 8388608 ); + $formatter = new ApiErrorFormatter( $result, Language::factory( 'qqx' ), 'plaintext', false ); + + $status = Status::newGood(); + $status->warning( 'mainpage' ); + $status->warning( 'parentheses', 'foobar' ); + $status->warning( wfMessage( 'mainpage' ) ); + $status->error( 'mainpage' ); + $status->error( 'parentheses', 'foobaz' ); + $formatter->addMessagesFromStatus( 'status', $status, [ 'warning', 'error' ], [ 'mainpage' ] ); + $this->assertSame( [ + 'errors' => [ + [ + 'code' => 'parentheses', + 'text' => '(parentheses: foobaz)', + 'module' => 'status', + ApiResult::META_CONTENT => 'text', + ], + ApiResult::META_INDEXED_TAG_NAME => 'error', + ], + 'warnings' => [ + [ + 'code' => 'parentheses', + 'text' => '(parentheses: foobar)', + 'module' => 'status', + ApiResult::META_CONTENT => 'text', + ], + ApiResult::META_INDEXED_TAG_NAME => 'warning', + ], + ApiResult::META_TYPE => 'assoc', + ], $result->getResultData() ); + } + /** * @dataProvider provideIsValidApiCode * @covers ApiErrorFormatter::isValidApiCode -- 2.20.1