Follow redirects for page/{title} formats html/with_html

* Apply Legacy Temporary redirects (302) if page is a redirect in order
to have feature parity with RESTBase

* Check for normalization redirects and execute permanent redirects (301)

* Add/Update mocha tests for the redirects functionality

* Add query parameter 'redirect=no' check to bypass redirect logic

* Unit tests to check status code and location headers

Bug: T301372
Change-Id: I841c21d54a58e118617aaf5e2c604ea22914adaa

includes/Rest/Handler/PageContentHelper.php

@@ -244,6 +244,11 @@ class PageContentHelper {
 				ParamValidator::PARAM_TYPE => 'string',
 				ParamValidator::PARAM_REQUIRED => true,
 			],
+			'redirect' => [
+				Handler::PARAM_SOURCE => 'query',
+				ParamValidator::PARAM_TYPE => [ 'no' ],
+				ParamValidator::PARAM_REQUIRED => false,
+			]
 		];
 	}
 

includes/Rest/Handler/PageHTMLHandler.php

@@ -7,7 +7,9 @@ use IBufferingStatsdDataFactory;
 use LogicException;
 use MediaWiki\Edit\ParsoidOutputStash;
 use MediaWiki\MediaWikiServices;
+use MediaWiki\Page\ExistingPageRecord;
 use MediaWiki\Page\PageLookup;
+use MediaWiki\Page\RedirectStore;
 use MediaWiki\Parser\Parsoid\HtmlTransformFactory;
 use MediaWiki\Parser\Parsoid\ParsoidOutputAccess;
 use MediaWiki\Rest\LocalizedHttpException;
@@ -33,6 +35,12 @@ class PageHTMLHandler extends SimpleHandler {
 	/** @var PageContentHelper */
 	private $contentHelper;
 
+	/** @var TitleFormatter */
+	private $titleFormatter;
+
+	/** @var RedirectStore */
+	private $redirectStore;
+
 	public function __construct(
 		Config $config,
 		RevisionLookup $revisionLookup,
@@ -41,8 +49,11 @@ class PageHTMLHandler extends SimpleHandler {
 		ParsoidOutputStash $parsoidOutputStash,
 		IBufferingStatsdDataFactory $statsDataFactory,
 		ParsoidOutputAccess $parsoidOutputAccess,
-		HtmlTransformFactory $htmlTransformFactory
+		HtmlTransformFactory $htmlTransformFactory,
+		RedirectStore $redirectStore
 	) {
+		$this->titleFormatter = $titleFormatter;
+		$this->redirectStore = $redirectStore;
 		$this->contentHelper = new PageContentHelper(
 			$config,
 			$revisionLookup,
@@ -82,13 +93,18 @@ class PageHTMLHandler extends SimpleHandler {
 	 */
 	public function run(): Response {
 		$this->contentHelper->checkAccess();
-
 		$page = $this->contentHelper->getPage();
 
 		// The call to $this->contentHelper->getPage() should not return null if
 		// $this->contentHelper->checkAccess() did not throw.
 		Assert::invariant( $page !== null, 'Page should be known' );
 
+		$pageRedirectResponse = $this->createPageRedirectResponse( $page );
+
+		if ( $pageRedirectResponse !== null ) {
+			return $pageRedirectResponse;
+		}
+
 		$parserOutput = $this->htmlHelper->getHtml();
 		// Do not de-duplicate styles, Parsoid already does it in a slightly different way (T300325)
 		$parserOutputHtml = $parserOutput->getText( [ 'deduplicateStyles' => false ] );
@@ -118,6 +134,44 @@ class PageHTMLHandler extends SimpleHandler {
 		return $response;
 	}
 
+	/**
+	 * Check for Page Redirects and create a Redirect Response
+	 * @param ExistingPageRecord $page
+	 * @return Response|null
+	 */
+	private function createPageRedirectResponse( ExistingPageRecord $page ): ?Response {
+		$titleAsRequested = $this->contentHelper->getTitleText();
+		$normalizedTitle = $this->titleFormatter->getPrefixedDBkey( $page );
+
+		// Check for normalization redirects
+		if ( $titleAsRequested !== $normalizedTitle ) {
+			$redirectTargetUrl = $this->getRouteUrl( [
+				"title" => $normalizedTitle
+			] );
+			return $this->getResponseFactory()->createPermanentRedirect( $redirectTargetUrl );
+		}
+
+		$params = $this->getRequest()->getQueryParams();
+		$redirectParam = $params['redirect'] ?? null;
+		$redirectTarget = $this->redirectStore->getRedirectTarget( $page );
+
+		if ( $redirectTarget === null ) {
+			return null;
+		}
+
+		// Check if page is a redirect
+		if ( $page->isRedirect() && $redirectParam !== 'no' ) {
+			$redirectTargetUrl = $this->getRouteUrl( [
+				"title" => $this->titleFormatter->getPrefixedDBkey(
+					$redirectTarget
+				)
+			] );
+			return $this->getResponseFactory()->createTemporaryRedirect( $redirectTargetUrl );
+		}
+
+		return null;
+	}
+
 	/**
 	 * Returns an ETag representing a page's source. The ETag assumes a page's source has changed
 	 * if the latest revision of a page has been made private, un-readable for another reason,

includes/Rest/coreRoutes.json

@@ -158,7 +158,8 @@
 			"ParsoidOutputStash",
 			"StatsdDataFactory",
 			"ParsoidOutputAccess",
-			"HtmlTransformFactory"
+			"HtmlTransformFactory",
+			"RedirectStore"
 		],
 		"format": "html"
 	},
@@ -173,7 +174,8 @@
 			"ParsoidOutputStash",
 			"StatsdDataFactory",
 			"ParsoidOutputAccess",
-			"HtmlTransformFactory"
+			"HtmlTransformFactory",
+			"RedirectStore"
 		],
 		"format": "with_html"
 	},

tests/api-testing/REST/Page.js

@@ -3,14 +3,28 @@
 const { action, assert, REST, utils } = require( 'api-testing' );
 
 describe( 'Page Source', () => {
-	const page = utils.title( 'PageSource ' );
+	const page = utils.title( 'PageSource_' );
+	const pageWithSpaces = page.replace( '_', ' ' );
 	const variantPage = utils.title( 'PageSourceVariant' );
+
+	const redirectPage = utils.title( 'Redirect ' );
+
 	const client = new REST();
 	const anon = action.getAnon();
 	const baseEditText = "''Edit 1'' and '''Edit 2'''";
 
 	before( async () => {
+		const mindy = await action.mindy();
 		await anon.edit( page, { text: baseEditText } );
+
+		// Setup page with redirects
+		await anon.edit( redirectPage, { text: `Original name is ${redirectPage}` } );
+		const token = await mindy.token();
+		await mindy.action( 'move', {
+			from: redirectPage,
+			to: redirectPage.replace( 'Redirect', 'Redirected' ),
+			token
+		}, true );
 	} );
 
 	describe( 'GET /page/{title}', () => {
@@ -19,7 +33,7 @@ describe( 'Page Source', () => {
 			assert.deepEqual( status, 200, text );
 			assert.containsAllKeys( body, [ 'latest', 'id', 'key', 'license', 'title', 'content_model', 'source' ] );
 			assert.nestedPropertyVal( body, 'content_model', 'wikitext' );
-			assert.nestedPropertyVal( body, 'title', page );
+			assert.nestedPropertyVal( body, 'title', pageWithSpaces );
 			assert.nestedPropertyVal( body, 'key', utils.dbkey( page ) );
 			assert.nestedPropertyVal( body, 'source', baseEditText );
 		} );
@@ -55,9 +69,9 @@ describe( 'Page Source', () => {
 			assert.deepEqual( status, 200, text );
 			assert.containsAllKeys( body, [ 'latest', 'id', 'key', 'license', 'title', 'content_model', 'html_url' ] );
 			assert.nestedPropertyVal( body, 'content_model', 'wikitext' );
-			assert.nestedPropertyVal( body, 'title', page );
+			assert.nestedPropertyVal( body, 'title', pageWithSpaces );
 			assert.nestedPropertyVal( body, 'key', utils.dbkey( page ) );
-			assert.match( body.html_url, new RegExp( `/page/${encodeURIComponent( page )}/html$` ) );
+			assert.match( body.html_url, new RegExp( `/page/${encodeURIComponent( pageWithSpaces )}/html$` ) );
 		} );
 		it( 'Should return 404 error for non-existent page', async () => {
 			const dummyPageTitle = utils.title( 'DummyPage_' );
@@ -86,6 +100,23 @@ describe( 'Page Source', () => {
 	} );
 
 	describe( 'GET /page/{title}/html', () => {
+		it( 'Title normalization should return permanent redirect (301)', async () => {
+			const { status, text } = await client.get( `/page/${redirectPage}/html` );
+			assert.deepEqual( status, 301, text );
+		} );
+
+		it( 'Wiki redirects should return legacy temporary redirect (307)', async () => {
+			const redirectPageDbkey = utils.dbkey( redirectPage );
+			const { status, text } = await client.get( `/page/${redirectPageDbkey}/html` );
+			assert.deepEqual( status, 307, text );
+		} );
+
+		it( 'Bypass redirects with query param redirect=no', async () => {
+			const redirectPageDbkey = utils.dbkey( redirectPage );
+			const { status, text } = await client.get( `/page/${redirectPageDbkey}/html`, { redirect: 'no' } );
+			assert.deepEqual( status, 200, text );
+		} );
+
 		it( 'Should successfully return page HTML', async () => {
 			const { status, headers, text } = await client.get( `/page/${page}/html` );
 			assert.deepEqual( status, 200, text );
@@ -131,12 +162,29 @@ describe( 'Page Source', () => {
 	} );
 
 	describe( 'GET /page/{title}/with_html', () => {
+		it( 'Title normalization should return permanent redirect (301)', async () => {
+			const { status, text } = await client.get( `/page/${redirectPage}/with_html` );
+			assert.deepEqual( status, 301, text );
+		} );
+
+		it( 'Wiki redirects should return legacy temporary redirect (307)', async () => {
+			const redirectPageDbkey = utils.dbkey( redirectPage );
+			const { status, text } = await client.get( `/page/${redirectPageDbkey}/with_html` );
+			assert.deepEqual( status, 307, text );
+		} );
+
+		it( 'Bypass redirects with query param redirect=no', async () => {
+			const redirectPageDbkey = utils.dbkey( redirectPage );
+			const { status, text } = await client.get( `/page/${redirectPageDbkey}/with_html`, { redirect: 'no' } );
+			assert.deepEqual( status, 200, text );
+		} );
+
 		it( 'Should successfully return page HTML and metadata for Wikitext page', async () => {
 			const { status, body, text } = await client.get( `/page/${page}/with_html` );
 			assert.deepEqual( status, 200, text );
 			assert.containsAllKeys( body, [ 'latest', 'id', 'key', 'license', 'title', 'content_model', 'html' ] );
 			assert.nestedPropertyVal( body, 'content_model', 'wikitext' );
-			assert.nestedPropertyVal( body, 'title', page );
+			assert.nestedPropertyVal( body, 'title', pageWithSpaces );
 			assert.nestedPropertyVal( body, 'key', utils.dbkey( page ) );
 			assert.match( body.html, /<html / );
 			assert.match( body.html, /Edit \w+<\/b>/ );

tests/api-testing/REST/Transform.js

@@ -86,8 +86,10 @@ describe( '/transform/ endpoint', function () {
 	const parsedUrl = new url.URL( client.req.app );
 	const PARSOID_URL = parsedUrl.href;
 	const endpointPrefix = client.pathPrefix = 'rest.php/coredev/v0';
-	const page = utils.title( 'TransformSource ' );
+	const page = utils.title( 'TransformSource_' );
+	const pageWithSpaces = page.replace( '_', ' ' );
 	const pageEncoded = encodeURIComponent( page );
+	const pageWithSpacesEncoded = encodeURIComponent( pageWithSpaces );
 	const pageContent = '{|\nhi\n|ho\n|}';
 	let revid;
 
@@ -645,7 +647,7 @@ describe( '/transform/ endpoint', function () {
 				.expect( function ( res ) {
 					res.headers.should.have.property( 'location' );
 					res.headers.location.should.equal(
-						PARSOID_URL + endpointPrefix + `/transform/wikitext/to/html/${pageEncoded}/${revid}`
+						PARSOID_URL + endpointPrefix + `/transform/wikitext/to/html/${pageWithSpacesEncoded}/${revid}`
 					);
 				} )
 				.end( done );
@@ -664,7 +666,7 @@ describe( '/transform/ endpoint', function () {
 				.expect( function ( res ) {
 					res.headers.should.have.property( 'location' );
 					res.headers.location.should.equal(
-						PARSOID_URL + endpointPrefix + `/transform/wikitext/to/pagebundle/${pageEncoded}/${revid}`
+						PARSOID_URL + endpointPrefix + `/transform/wikitext/to/pagebundle/${pageWithSpacesEncoded}/${revid}`
 					);
 				} )
 				.end( done );
@@ -682,7 +684,7 @@ describe( '/transform/ endpoint', function () {
 				.expect( function ( res ) {
 					res.headers.should.have.property( 'location' );
 					const expected = PARSOID_URL + endpointPrefix +
-						`/transform/wikitext/to/html/${pageEncoded}/`;
+						`/transform/wikitext/to/html/${pageWithSpacesEncoded}/`;
 
 					assert.strictEqual(
 						res.headers.location.startsWith( expected ), true, res.headers.location

tests/phpunit/integration/includes/Rest/Handler/PageHTMLHandlerTest.php

@@ -117,7 +117,8 @@ class PageHTMLHandlerTest extends MediaWikiIntegrationTestCase {
 			$this->getParsoidOutputStash(),
 			$services->getStatsdDataFactory(),
 			$parsoidOutputAccess,
-			$services->getHtmlTransformFactory()
+			$services->getHtmlTransformFactory(),
+			$services->getRedirectStore()
 		);
 
 		return $handler;
@@ -169,6 +170,51 @@ class PageHTMLHandlerTest extends MediaWikiIntegrationTestCase {
 		$this->assertStringContainsString( self::HTML, $htmlResponse );
 	}
 
+	public function testTemporaryRedirectHtmlOnly() {
+		$this->markTestSkippedIfExtensionNotLoaded( 'Parsoid' );
+
+		$targetPageTitle = 'HtmlEndpointTestPage';
+		$redirectPageTitle = 'RedirectPage';
+		$this->getExistingTestPage( $targetPageTitle );
+		$status = $this->editPage( $redirectPageTitle, "#REDIRECT [[$targetPageTitle]]" );
+		$this->assertStatusOK( $status );
+
+		$request = new RequestData(
+			[ 'pathParams' => [ 'title' => $redirectPageTitle ] ]
+		);
+
+		$handler = $this->newHandler();
+		$response = $this->executeHandler( $handler, $request, [
+			'format' => 'html',
+			'path' => '/page/{title}/html'
+		] );
+
+		$this->assertEquals( 307, $response->getStatusCode() );
+		$this->assertStringContainsString( $targetPageTitle, $response->getHeaderLine( 'location' ) );
+	}
+
+	public function testPermanentRedirectHtmlOnly() {
+		$this->markTestSkippedIfExtensionNotLoaded( 'Parsoid' );
+		$page = $this->getExistingTestPage( 'HtmlEndpointTestPage with spaces' );
+		$this->assertTrue(
+			$this->editPage( $page, self::WIKITEXT )->isGood(),
+			'Edited a page'
+		);
+
+		$request = new RequestData(
+			[ 'pathParams' => [ 'title' => $page->getTitle()->getPrefixedText() ] ]
+		);
+
+		$handler = $this->newHandler();
+		$response = $this->executeHandler( $handler, $request, [
+			'format' => 'html',
+			'path' => '/page/{title}/html'
+		] );
+
+		$this->assertEquals( 301, $response->getStatusCode() );
+		$this->assertStringContainsString( $page->getTitle()->getPrefixedDBkey(), $response->getHeaderLine( 'location' ) );
+	}
+
 	/**
 	 * @dataProvider provideExecuteWithVariant
 	 */