jenkins-bot has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/387328 )
Change subject: Update: report server URL redirect statuses ...................................................................... Update: report server URL redirect statuses When a redirect occurs on a PageModule.getInitialProps() request issued server-side, such as https://en.wikipedia.org/api/rest_v1/page/summary/Albert%20Einstein, report the 3xx HTTP status instead of following the URL. This gives browsers the expected behavior when visiting a non-canonical URL. No changes were intended to client-side behavior yet. PageModule.getInitialProps() requests work the same as before when no redirect occurs. However, when a redirect does occur on the server, this is considered exceptional and an error with the status code and resolved URL is thrown. For example, a getInitialProps() request for https://en.wikipedia.org/api/rest_v1/page/summary/Albert%20Einstein would throw an error with an HTTP status of 301 and a URL of https://en.wikipedia.org/api/rest_v1/page/summary/Albert_Einstein. The PageModule itself catches the error and constructs the correct destination URL for that page, since only it knows how to get the route parameters needed to pass to Route.toPath(), for example, http://localhost:3000/page/summary/Albert_Einstein, then finally rethrowing the error with the new URL for the server router to handle. An alternative implementation for redirected responses was considered: instead of throwing an error, return a response with an empty body. This might work well later when TypeScript discriminated union types become more fully featured. However, today this would percolate response body existence checking at every layer. The unmarshaller has nothing to do with a redirected response so this seems unwanted. Additionally, throwing an error is not entirely beyond compare in that the Fetch API itself supports a similar redirect behavior when RequestInit.redirect is set to "error". It would be desirable for the server to handle all redirect logic in a generic way for all requests but it's not clear how to do that. The getInitialProps() RESTBase URL needs to be destructured and a new Marvin URL must be created from the result. Bug: T177681 Change-Id: I89e03f7a9e45f318c01e26379114a195ebbff2cb --- A src/common/http/fetch-with-redirect.ts M src/common/http/page-http-client.ts M src/common/http/page-summary-http-client.ts M src/common/pages/home.tsx M src/common/pages/summary.tsx M src/common/pages/wiki.tsx M src/server/index.tsx 7 files changed, 103 insertions(+), 14 deletions(-) Approvals: Niedzielski: Looks good to me, approved jenkins-bot: Verified diff --git a/src/common/http/fetch-with-redirect.ts b/src/common/http/fetch-with-redirect.ts new file mode 100644 index 0000000..afa4730 --- /dev/null +++ b/src/common/http/fetch-with-redirect.ts @@ -0,0 +1,42 @@ +import * as unfetch from "isomorphic-unfetch"; + +/** true if executing on server, false otherwise. */ +const server = typeof window !== "object"; + +/** Server-only redirect status code and destination URL. */ +export class RedirectError extends Error { + /** Status code in the domain of [300, 399]. */ + status: number; + + /** Destination URL. */ + url: string; + + constructor(status: number, url: string) { + super(); + this.status = status; + this.url = url; + } +} + +/** + * Isomorphic fetch with transparent throw-on-redirect behavior for requests + * issued on the server. The redirect behavior may be overridden by setting + * RequestInit.redirect to "follow". On the client, the implementation performs + * identically to fetch. Capturing redirects allows the server to respond with + * the appropriate status code and resolved URL. + */ +export function fetch( + input: RequestInfo, + init?: RequestInit +): Promise<Response> { + // Setting the redirect mode to "error" doesn't appear to yield the status + // code so "manual" is used instead. + const redirect = server ? "manual" : undefined; + return unfetch(input, { redirect, ...init }).then(response => { + if (server && response.status >= 300 && response.status <= 399) { + const url = response.headers.get("location"); + throw new RedirectError(response.status, url as string); + } + return response; + }); +} diff --git a/src/common/http/page-http-client.ts b/src/common/http/page-http-client.ts index 48235e7..ec73e02 100644 --- a/src/common/http/page-http-client.ts +++ b/src/common/http/page-http-client.ts @@ -1,4 +1,3 @@ -import * as fetch from "isomorphic-unfetch"; import { IsomorphicHeaders } from "../types/isomorphic-unfetch-extras"; import { JSONObject } from "../types/json"; import { Page, PageLead } from "../models/page/page"; @@ -10,6 +9,7 @@ import { RESTBase } from "../marshallers/restbase"; import HttpResponse from "./http-response"; import { PageRedirect } from "./page-redirect"; +import { fetch } from "./fetch-with-redirect"; import reencodeRESTBaseTitlePath from "./restbase-title-encoder"; // https://en.wikipedia.org/api/rest_v1/#!/Mobile/get_page_mobile_sections_title_revision diff --git a/src/common/http/page-summary-http-client.ts b/src/common/http/page-summary-http-client.ts index f64f798..78e740a 100644 --- a/src/common/http/page-summary-http-client.ts +++ b/src/common/http/page-summary-http-client.ts @@ -1,10 +1,10 @@ -import * as fetch from "isomorphic-unfetch"; import { PageSummary } from "../models/page/summary"; import { PageTitlePath } from "../models/page/title"; import { RESTBase } from "../marshallers/restbase"; import { unmarshalPageSummary } from "../marshallers/page-summary/page-summary-unmarshaller"; // eslint-disable-line max-len import HttpResponse from "./http-response"; import { PageRedirect } from "./page-redirect"; +import { fetch } from "./fetch-with-redirect"; import reencodeRESTBaseTitlePath from "./restbase-title-encoder"; // https://en.wikipedia.org/api/rest_v1/#!/Page_content/get_page_summary_title diff --git a/src/common/pages/home.tsx b/src/common/pages/home.tsx index 78ac04c..20b1a64 100644 --- a/src/common/pages/home.tsx +++ b/src/common/pages/home.tsx @@ -29,6 +29,14 @@ { title: "Bill_&_Ted's_Excellent_Adventure", text: "With two paragraphs, unencoded path, and styled title" + }, + { + title: "Carrot cake", + text: "Encoding redirect (301)" + }, + { + title: "Cheese_cake", + text: "Redirect page (302)" } ]; @@ -42,12 +50,12 @@ text: "Disambiguation" }, { - title: "Cheese_cake", - text: "Redirect" + title: "Carrot cake", + text: "Encoding redirect (301)" }, { - title: "Carrot cake", - text: "Encoding redirect" + title: "Cheese_cake", + text: "Redirect page (302)" }, { title: "Ice_cream_cake", diff --git a/src/common/pages/summary.tsx b/src/common/pages/summary.tsx index a22a766..b6037de 100644 --- a/src/common/pages/summary.tsx +++ b/src/common/pages/summary.tsx @@ -5,10 +5,13 @@ import { PageTitleID, PageTitlePath } from "../models/page/title"; import Page from "../components/page/page"; import { RouteParams } from "../routers/route"; +import { summary } from "../routers/api"; import { request } from "../http/page-summary-http-client"; import ContentHeader from "../components/content-header/content-header"; import ContentFooter from "../components/content-footer/content-footer"; import HttpResponse from "../http/http-response"; +import { RedirectError } from "../http/fetch-with-redirect"; +import { unmarshalPageTitleID } from "../marshallers/page-base/page-base-unmarshaller"; // eslint-disable-line max-len interface PageParams extends RouteParams { /** @@ -32,10 +35,22 @@ params.title === undefined ? { random: true } : { titlePath: params.title } - ).then(({ status, data }) => ({ - status, - data: { summary: data } - })); + ) + .then(({ status, data }) => ({ + status, + data: { summary: data } + })) + .catch(error => { + if (error instanceof RedirectError) { + error = new RedirectError( + error.status, + summary.toPath({ + title: decodeURIComponent(unmarshalPageTitleID(error.url)) + }) + ); + } + throw error; + }); }, Component({ summary }: Props): JSX.Element { diff --git a/src/common/pages/wiki.tsx b/src/common/pages/wiki.tsx index 934f976..a410686 100644 --- a/src/common/pages/wiki.tsx +++ b/src/common/pages/wiki.tsx @@ -5,10 +5,13 @@ import { PageTitleID, PageTitlePath } from "../models/page/title"; import Page from "../components/page/page"; import { RouteParams } from "../routers/route"; +import { wiki } from "../routers/api"; import { requestPage } from "../http/page-http-client"; import ContentFooter from "../components/content-footer/content-footer"; import ContentPage from "../components/content-page/content-page"; import HttpResponse from "../http/http-response"; +import { RedirectError } from "../http/fetch-with-redirect"; +import { unmarshalPageTitleID } from "../marshallers/page-base/page-base-unmarshaller"; // eslint-disable-line max-len interface PageParams extends RouteParams { /** @@ -42,7 +45,20 @@ ? undefined : parseInt(params.revision, 10) } - ).then(({ status, data }) => ({ status, data: { page: data } })); + ) + .then(({ status, data }) => ({ status, data: { page: data } })) + .catch(error => { + if (error instanceof RedirectError) { + error = new RedirectError( + error.status, + wiki.toPath({ + title: decodeURIComponent(unmarshalPageTitleID(error.url)), + revision: params.revision + }) + ); + } + throw error; + }); }, Component({ page }: Props): JSX.Element { diff --git a/src/server/index.tsx b/src/server/index.tsx index cc1551b..d73d3ed 100644 --- a/src/server/index.tsx +++ b/src/server/index.tsx @@ -17,6 +17,7 @@ import { render as renderToString } from "preact-render-to-string"; import { RouteResponse, newRouter } from "../common/routers/router"; +import { RedirectError } from "../common/http/fetch-with-redirect"; import { routes } from "../common/routers/api"; import { PRODUCTION, @@ -57,9 +58,16 @@ response.status(routeResponse.status).send(render(routeResponse)) ) .catch(error => { - const message = `${error.message}\n${error.stack}`; - console.error(message); // eslint-disable-line no-console - response.status(500).send(message); + if (error instanceof RedirectError) { + return response + .status(error.status) + .header("location", error.url) + .send(); + } else { + const message = `${error.message}\n${error.stack}`; + console.error(message); // eslint-disable-line no-console + return response.status(500).send(message); + } }); }); -- To view, visit https://gerrit.wikimedia.org/r/387328 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I89e03f7a9e45f318c01e26379114a195ebbff2cb Gerrit-PatchSet: 5 Gerrit-Project: marvin Gerrit-Branch: master Gerrit-Owner: Niedzielski <[email protected]> Gerrit-Reviewer: Jhernandez <[email protected]> Gerrit-Reviewer: Niedzielski <[email protected]> Gerrit-Reviewer: Sniedzielski <[email protected]> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
