mirror of
https://gerrit.wikimedia.org/r/mediawiki/extensions/VisualEditor
synced 2024-11-23 14:06:52 +00:00
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
This commit is contained in:
Bartosz Dziewoński
parent
01b3137afa
commit
ca64e7e407
|
|
@ -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<string,string>}
|
||||
* @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<string,string>}
|
||||
* @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 `<body>` 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<string,string>}
|
||||
* @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 );
|
||||
|
||||
|
|
|
|||
|
|
@ -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<string,string>}
|
||||
*/
|
||||
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<string,string>}
|
||||
*/
|
||||
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<string,string>}
|
||||
*/
|
||||
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(),
|
||||
];
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<string,string>,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<string,string>}
|
||||
*/
|
||||
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<string,string>,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<string,string>}
|
||||
*/
|
||||
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<string,string>,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<string,string>}
|
||||
*/
|
||||
public function transformWikitext(
|
||||
PageIdentity $page,
|
||||
|
|
|
|||
|
|
@ -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'] );
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue