Niedzielski has uploaded a new change for review. ( 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 to client-side behavior yet. Bug: T177681 Change-Id: I89e03f7a9e45f318c01e26379114a195ebbff2cb --- A src/common/data-clients/fetch-with-redirect.ts M src/common/data-clients/page-data-client.ts M src/common/data-clients/page-summary-data-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, 104 insertions(+), 14 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/marvin refs/changes/28/387328/1 diff --git a/src/common/data-clients/fetch-with-redirect.ts b/src/common/data-clients/fetch-with-redirect.ts new file mode 100644 index 0000000..afa4730 --- /dev/null +++ b/src/common/data-clients/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/data-clients/page-data-client.ts b/src/common/data-clients/page-data-client.ts index 0fa40a4..b762c35 100644 --- a/src/common/data-clients/page-data-client.ts +++ b/src/common/data-clients/page-data-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/data-clients/page-summary-data-client.ts b/src/common/data-clients/page-summary-data-client.ts index a223344..ad48181 100644 --- a/src/common/data-clients/page-summary-data-client.ts +++ b/src/common/data-clients/page-summary-data-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-unmarshaller"; 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 c4657fe..2f03787 100644 --- a/src/common/pages/home.tsx +++ b/src/common/pages/home.tsx @@ -30,6 +30,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)" } ]; const testPages = [ @@ -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 f2ef0f1..8252bc4 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 "../data-clients/page-summary-data-client"; import ContentHeader from "../components/content-header/content-header"; import ContentFooter from "../components/content-footer/content-footer"; import HttpResponse from "../data-clients/http-response"; +import { RedirectError } from "../data-clients/fetch-with-redirect"; +import { unmarshalPageTitleID } from "../marshallers/page-unmarshaller"; interface PageParams extends RouteParams { /** @@ -31,10 +34,20 @@ ): Promise<HttpResponse<Props>> => request( 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: unmarshalPageTitleID(error.url) }) + ); + } + throw error; + }); export const Component = ({ summary }: Props): JSX.Element => ( <App> diff --git a/src/common/pages/wiki.tsx b/src/common/pages/wiki.tsx index 71141f7..bc60d12 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 "../data-clients/page-data-client"; import ContentFooter from "../components/content-footer/content-footer"; import ContentPage from "../components/content-page/content-page"; import HttpResponse from "../data-clients/http-response"; +import { RedirectError } from "../data-clients/fetch-with-redirect"; +import { unmarshalPageTitleID } from "../marshallers/page-unmarshaller"; interface PageParams extends RouteParams { /** @@ -43,7 +46,23 @@ ? 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: unmarshalPageTitleID(error.url), + revision: params.revision + }) + ); + } + throw error; + }); export const Component = ({ page }: Props): JSX.Element => ( <App> diff --git a/src/server/index.tsx b/src/server/index.tsx index cc1551b..0b24acc 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/data-clients/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: newchange Gerrit-Change-Id: I89e03f7a9e45f318c01e26379114a195ebbff2cb Gerrit-PatchSet: 1 Gerrit-Project: marvin Gerrit-Branch: master Gerrit-Owner: Niedzielski <[email protected]> Gerrit-Reviewer: Sniedzielski <[email protected]> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
