From ca64e7e4077f2724ae583e163ad62d52eea6318b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Mon, 14 Aug 2023 17:14:12 +0200 Subject: [PATCH] ApiParsoidTrait/DirectParsoidClient: Simplify error handling After catching the REST exception, just throw an API exception. Remove the step inbetween where we built a response array. Bonus: * Remove tests that only tested this array building. * Remove the unused 'code' from successful responses, and update documentation of return value shapes. * Remove impossible check for `$ex instanceof LocalizedException`. After moving the code into one place, it's obvious that it can't happen, since LocalizedException doesn't inherit from HttpException. Bug: T341613 Change-Id: I31370efeed9d5283b53eafca102b2f1efc811d27 --- includes/ApiParsoidTrait.php | 78 ++++++++++----- includes/DirectParsoidClient.php | 96 ++++++------------- includes/ParsoidClient.php | 12 +-- .../integration/DirectParsoidClientTest.php | 77 --------------- 4 files changed, 85 insertions(+), 178 deletions(-) diff --git a/includes/ApiParsoidTrait.php b/includes/ApiParsoidTrait.php index 0aa2e11360..d792aa04c7 100644 --- a/includes/ApiParsoidTrait.php +++ b/includes/ApiParsoidTrait.php @@ -10,15 +10,19 @@ namespace MediaWiki\Extension\VisualEditor; +use ApiUsageException; use Language; use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; use MediaWiki\MediaWikiServices; +use MediaWiki\Rest\HttpException; +use MediaWiki\Rest\LocalizedHttpException; use MediaWiki\Revision\RevisionRecord; use Message; use NullStatsdDataFactory; use PrefixingStatsdDataFactoryProxy; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; +use RawMessage; use Title; use WebRequest; @@ -72,41 +76,56 @@ trait ApiParsoidTrait { } /** - * @param array $response + * @param HttpException $ex + * @return never + * @throws ApiUsageException */ - private function forwardErrorsAndCacheHeaders( array $response ) { - if ( !empty( $response['error'] ) ) { - $this->dieWithError( $response['error'] ); + private function dieWithRestHttpException( HttpException $ex ) { + if ( $ex instanceof LocalizedHttpException ) { + $msg = $ex->getMessageValue(); + } else { + $msg = new RawMessage( $ex->getMessage() ); } + + $this->dieWithError( [ + 'message' => $msg->getKey() ?? '', + 'params' => $msg->getParams() ?? [] + ] ); } /** - * Request page HTML from RESTBase + * Request page HTML from Parsoid. * * @param RevisionRecord $revision Page revision - * @return array The RESTBase server's response + * @return array An array mimicking a RESTbase server's response, with keys: 'headers' and 'body' + * @phan-return array{body:string,headers:array} + * @throws ApiUsageException */ protected function requestRestbasePageHtml( RevisionRecord $revision ): array { $title = Title::newFromLinkTarget( $revision->getPageAsLinkTarget() ); $lang = self::getPageLanguage( $title ); $startTime = $this->statsGetStartTime(); - $response = $this->getParsoidClient()->getPageHtml( $revision, $lang ); + try { + $response = $this->getParsoidClient()->getPageHtml( $revision, $lang ); + } catch ( HttpException $ex ) { + $this->dieWithRestHttpException( $ex ); + } $this->statsRecordTiming( 'ApiVisualEditor.ParsoidClient.getPageHtml', $startTime ); - $this->forwardErrorsAndCacheHeaders( $response ); - return $response; } /** - * Transform HTML to wikitext via Parsoid through RESTbase. Wrapper for ::postData(). + * Transform HTML to wikitext with Parsoid. * * @param Title $title The title of the page * @param string $html The HTML of the page to be transformed * @param int|null $oldid What oldid revision, if any, to base the request from (default: `null`) * @param string|null $etag The ETag to set in the HTTP request header - * @return array The RESTbase server's response, 'code', 'reason', 'headers' and 'body' + * @return array An array mimicking a RESTbase server's response, with keys: 'headers' and 'body' + * @phan-return array{body:string,headers:array} + * @throws ApiUsageException */ protected function transformHTML( Title $title, string $html, int $oldid = null, string $etag = null @@ -114,23 +133,27 @@ trait ApiParsoidTrait { $lang = self::getPageLanguage( $title ); $startTime = $this->statsGetStartTime(); - $response = $this->getParsoidClient()->transformHTML( $title, $lang, $html, $oldid, $etag ); + try { + $response = $this->getParsoidClient()->transformHTML( $title, $lang, $html, $oldid, $etag ); + } catch ( HttpException $ex ) { + $this->dieWithRestHttpException( $ex ); + } $this->statsRecordTiming( 'ApiVisualEditor.ParsoidClient.transformHTML', $startTime ); - $this->forwardErrorsAndCacheHeaders( $response ); - return $response; } /** - * Transform wikitext to HTML via Parsoid through RESTbase. Wrapper for ::postData(). + * Transform wikitext to HTML with Parsoid. * * @param Title $title The title of the page to use as the parsing context * @param string $wikitext The wikitext fragment to parse * @param bool $bodyOnly Whether to provide only the contents of the `` tag * @param int|null $oldid What oldid revision, if any, to base the request from (default: `null`) * @param bool $stash Whether to stash the result in the server-side cache (default: `false`) - * @return array The RESTbase server's response, 'code', 'reason', 'headers' and 'body' + * @return array An array mimicking a RESTbase server's response, with keys: 'headers' and 'body' + * @phan-return array{body:string,headers:array} + * @throws ApiUsageException */ protected function transformWikitext( Title $title, string $wikitext, bool $bodyOnly, int $oldid = null, bool $stash = false @@ -138,18 +161,20 @@ trait ApiParsoidTrait { $lang = self::getPageLanguage( $title ); $startTime = $this->statsGetStartTime(); - $response = $this->getParsoidClient()->transformWikitext( - $title, - $lang, - $wikitext, - $bodyOnly, - $oldid, - $stash - ); + try { + $response = $this->getParsoidClient()->transformWikitext( + $title, + $lang, + $wikitext, + $bodyOnly, + $oldid, + $stash + ); + } catch ( HttpException $ex ) { + $this->dieWithRestHttpException( $ex ); + } $this->statsRecordTiming( 'ApiVisualEditor.ParsoidClient.transformWikitext', $startTime ); - $this->forwardErrorsAndCacheHeaders( $response ); - return $response; } @@ -183,6 +208,7 @@ trait ApiParsoidTrait { * @param array|null $data See ApiErrorFormatter::addError() * @param int|null $httpCode HTTP error code to use * @return never + * @throws ApiUsageException */ abstract public function dieWithError( $msg, $code = null, $data = null, $httpCode = null ); diff --git a/includes/DirectParsoidClient.php b/includes/DirectParsoidClient.php index 7731842df7..ff0478022d 100644 --- a/includes/DirectParsoidClient.php +++ b/includes/DirectParsoidClient.php @@ -10,20 +10,15 @@ namespace MediaWiki\Extension\VisualEditor; -use Exception; -use LocalizedException; use MediaWiki\Page\PageIdentity; use MediaWiki\Parser\Parsoid\ParsoidRenderID; use MediaWiki\Permissions\Authority; use MediaWiki\Rest\Handler\Helper\HtmlInputTransformHelper; use MediaWiki\Rest\Handler\Helper\HtmlOutputRendererHelper; use MediaWiki\Rest\Handler\Helper\PageRestHelperFactory; -use MediaWiki\Rest\HttpException; -use MediaWiki\Rest\LocalizedHttpException; use MediaWiki\Revision\MutableRevisionRecord; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Revision\SlotRecord; -use RawMessage; use User; use Wikimedia\Bcp47Code\Bcp47Code; use WikitextContent; @@ -145,21 +140,17 @@ class DirectParsoidClient implements ParsoidClient { * @param RevisionRecord $revision Page revision * @param ?Bcp47Code $targetLanguage Page language (default: `null`) * - * @return array An array mimicking a RESTbase server's response, - * with keys: 'error', 'headers' and 'body' + * @return array An array mimicking a RESTbase server's response, with keys: 'headers' and 'body' + * @phan-return array{body:string,headers:array} */ public function getPageHtml( RevisionRecord $revision, ?Bcp47Code $targetLanguage = null ): array { // In the VE client, we always want to stash. $page = $revision->getPage(); - try { - $helper = $this->getHtmlOutputRendererHelper( $page, $revision, $targetLanguage, true ); - $parserOutput = $helper->getHtml(); + $helper = $this->getHtmlOutputRendererHelper( $page, $revision, $targetLanguage, true ); + $parserOutput = $helper->getHtml(); - return $this->fakeRESTbaseHTMLResponse( $parserOutput->getRawText(), $helper ); - } catch ( HttpException $ex ) { - return $this->fakeRESTbaseError( $ex ); - } + return $this->fakeRESTbaseHTMLResponse( $parserOutput->getRawText(), $helper ); } /** @@ -182,7 +173,7 @@ class DirectParsoidClient implements ParsoidClient { } /** - * Transform wikitext to HTML with Parsoid. Wrapper for ::postData(). + * Transform wikitext to HTML with Parsoid. * * @param PageIdentity $page The page the content belongs to use as the parsing context * @param Bcp47Code $targetLanguage Page language @@ -191,8 +182,8 @@ class DirectParsoidClient implements ParsoidClient { * @param int|null $oldid What oldid revision, if any, to base the request from (default: `null`) * @param bool $stash Whether to stash the result in the server-side cache (default: `false`) * - * @return array An array mimicking a RESTbase server's response, - * with keys 'code', 'reason', 'headers' and 'body' + * @return array An array mimicking a RESTbase server's response, with keys: 'headers' and 'body' + * @phan-return array{body:string,headers:array} */ public function transformWikitext( PageIdentity $page, @@ -204,20 +195,16 @@ class DirectParsoidClient implements ParsoidClient { ): array { $revision = $this->makeFakeRevision( $page, $wikitext ); - try { - $helper = $this->getHtmlOutputRendererHelper( $page, $revision, $targetLanguage, $stash ); + $helper = $this->getHtmlOutputRendererHelper( $page, $revision, $targetLanguage, $stash ); - if ( $bodyOnly ) { - $helper->setFlavor( 'fragment' ); - } - - $parserOutput = $helper->getHtml(); - $html = $parserOutput->getRawText(); - - return $this->fakeRESTbaseHTMLResponse( $html, $helper ); - } catch ( HttpException $ex ) { - return $this->fakeRESTbaseError( $ex ); + if ( $bodyOnly ) { + $helper->setFlavor( 'fragment' ); } + + $parserOutput = $helper->getHtml(); + $html = $parserOutput->getRawText(); + + return $this->fakeRESTbaseHTMLResponse( $html, $helper ); } /** @@ -229,27 +216,23 @@ class DirectParsoidClient implements ParsoidClient { * @param ?int $oldid What oldid revision, if any, to base the request from (default: `null`) * @param ?string $etag The ETag to set in the HTTP request header * - * @return array The response, 'code', 'reason', 'headers' and 'body' + * @return array An array mimicking a RESTbase server's response, with keys: 'headers' and 'body' + * @phan-return array{body:string,headers:array} */ public function transformHTML( PageIdentity $page, Bcp47Code $targetLanguage, string $html, ?int $oldid, ?string $etag ): array { - try { - $helper = $this->getHtmlInputTransformHelper( $page, $html, $oldid, $etag, $targetLanguage ); + $helper = $this->getHtmlInputTransformHelper( $page, $html, $oldid, $etag, $targetLanguage ); - $content = $helper->getContent(); - $format = $content->getDefaultFormat(); + $content = $helper->getContent(); + $format = $content->getDefaultFormat(); - return [ - 'code' => 200, - 'headers' => [ - 'Content-Type' => $format, - ], - 'body' => $content->serialize( $format ), - ]; - } catch ( HttpException $ex ) { - return $this->fakeRESTbaseError( $ex ); - } + return [ + 'headers' => [ + 'Content-Type' => $format, + ], + 'body' => $content->serialize( $format ), + ]; } /** @@ -261,7 +244,6 @@ class DirectParsoidClient implements ParsoidClient { private function fakeRESTbaseHTMLResponse( $data, HtmlOutputRendererHelper $helper ): array { $contentLanguage = $helper->getHtmlOutputContentLanguage(); return [ - 'code' => 200, 'headers' => [ 'content-language' => $contentLanguage->toBcp47Code(), 'etag' => $helper->getETag() @@ -270,28 +252,4 @@ class DirectParsoidClient implements ParsoidClient { ]; } - /** - * @param Exception $ex - * - * @return array - */ - private function fakeRESTbaseError( Exception $ex ): array { - if ( $ex instanceof LocalizedHttpException ) { - $msg = $ex->getMessageValue(); - } elseif ( $ex instanceof LocalizedException ) { - $msg = $ex->getMessageObject(); - } else { - $msg = new RawMessage( $ex->getMessage() ); - } - - return [ - 'error' => [ - 'message' => $msg->getKey() ?? '', - 'params' => $msg->getParams() ?? [] - ], - 'headers' => [], - 'body' => $ex->getMessage(), - ]; - } - } diff --git a/includes/ParsoidClient.php b/includes/ParsoidClient.php index b1613a18f0..0095d4678d 100644 --- a/includes/ParsoidClient.php +++ b/includes/ParsoidClient.php @@ -13,8 +13,8 @@ interface ParsoidClient { * @param RevisionRecord $revision Page revision * @param Bcp47Code|null $targetLanguage Desired output language * - * @return array An array containing the keys 'body', 'headers', and optionally 'error' - * @phan-return array{body:string,headers:array,error?:non-empty-array} + * @return array An array mimicking a RESTbase server's response, with keys: 'headers' and 'body' + * @phan-return array{body:string,headers:array} */ public function getPageHtml( RevisionRecord $revision, ?Bcp47Code $targetLanguage ): array; @@ -27,8 +27,8 @@ interface ParsoidClient { * @param ?int $oldid What oldid revision, if any, to base the request from (default: `null`) * @param ?string $etag The ETag to set in the HTTP request header * - * @return array An array containing the keys 'body', 'headers', and optionally 'error' - * @phan-return array{body:string,headers:array,error?:non-empty-array} + * @return array An array mimicking a RESTbase server's response, with keys: 'headers' and 'body' + * @phan-return array{body:string,headers:array} */ public function transformHTML( PageIdentity $page, @@ -48,8 +48,8 @@ interface ParsoidClient { * @param ?int $oldid What oldid revision, if any, to base the request from (default: `null`) * @param bool $stash Whether to stash the result in the server-side cache (default: `false`) * - * @return array An array containing the keys 'body', 'headers', and optionally 'error' - * @phan-return array{body:string,headers:array,error?:non-empty-array} + * @return array An array mimicking a RESTbase server's response, with keys: 'headers' and 'body' + * @phan-return array{body:string,headers:array} */ public function transformWikitext( PageIdentity $page, diff --git a/tests/phpunit/integration/DirectParsoidClientTest.php b/tests/phpunit/integration/DirectParsoidClientTest.php index c47383cd3d..e3300bce98 100644 --- a/tests/phpunit/integration/DirectParsoidClientTest.php +++ b/tests/phpunit/integration/DirectParsoidClientTest.php @@ -6,8 +6,6 @@ use Generator; use MediaWiki\Extension\VisualEditor\DirectParsoidClient; use MediaWiki\Page\PageIdentityValue; use MediaWiki\Parser\Parsoid\ParsoidOutputAccess; -use MediaWiki\Rest\Handler\Helper\PageRestHelperFactory; -use MediaWiki\Rest\HttpException; use MediaWiki\Revision\RevisionRecord; use MediaWikiIntegrationTestCase; use Wikimedia\Bcp47Code\Bcp47CodeValue; @@ -31,28 +29,6 @@ class DirectParsoidClientTest extends MediaWikiIntegrationTestCase { return $directClient; } - /** - * @return DirectParsoidClient - */ - private function createDirectClientWithHttpExceptionFromFactory(): DirectParsoidClient { - $factory = $this->createNoOpMock( PageRestHelperFactory::class, [ - 'newHtmlOutputRendererHelper', - 'newHtmlInputTransformHelper', - ] ); - - $e = new HttpException( 'testing', 400 ); - $factory->method( 'newHtmlOutputRendererHelper' )->willThrowException( $e ); - $factory->method( 'newHtmlInputTransformHelper' )->willThrowException( $e ); - - $services = $this->getServiceContainer(); - $directClient = new DirectParsoidClient( - $factory, - $services->getUserFactory()->newAnonymous() - ); - - return $directClient; - } - /** @return Generator */ public static function provideLanguageCodes() { yield 'German language code' => [ 'de' ]; @@ -229,57 +205,4 @@ class DirectParsoidClientTest extends MediaWikiIntegrationTestCase { $this->assertStringContainsString( 'More Text', $updatedWikitext ); } - /** - * @covers ::getPageHtml - */ - public function testGetPageHtml_HttpException() { - $directClient = $this->createDirectClientWithHttpExceptionFromFactory(); - - $revision = $this->getExistingTestPage( 'DirectParsoidClient' ) - ->getRevisionRecord(); - - $response = $directClient->getPageHtml( $revision ); - $this->assertArrayHasKey( 'error', $response ); - $this->assertSame( 'testing', $response['error']['message'] ); - } - - /** - * @covers ::getPageHtml - */ - public function testTransformHtml_HttpException() { - $directClient = $this->createDirectClientWithHttpExceptionFromFactory(); - - $page = $this->getExistingTestPage( 'DirectParsoidClient' ); - - $response = $directClient->transformHTML( - $page, - $this->getServiceContainer()->getLanguageFactory()->getLanguage( 'en' ), - 'some html', - null, - null - ); - $this->assertArrayHasKey( 'error', $response ); - $this->assertSame( 'testing', $response['error']['message'] ); - } - - /** - * @covers ::getPageHtml - */ - public function testTransformWikitext_HttpException() { - $directClient = $this->createDirectClientWithHttpExceptionFromFactory(); - - $page = $this->getExistingTestPage( 'DirectParsoidClient' ); - - $response = $directClient->transformWikitext( - $page, - $this->getServiceContainer()->getLanguageFactory()->getLanguage( 'en' ), - 'some text', - false, - null, - false - ); - $this->assertArrayHasKey( 'error', $response ); - $this->assertSame( 'testing', $response['error']['message'] ); - } - }