From 1765158ae75c6c91a560d3fd9151bebbea2f7c48 Mon Sep 17 00:00:00 2001 From: Daniel Phang Date: Sat, 26 Sep 2020 12:13:57 -0700 Subject: [PATCH 1/7] feat(lambda-at-edge): support custom redirects --- .../cypress/integration/redirects.test.ts | 119 ++++++++++++++++-- .../next-app/cypress/support/commands.ts | 19 ++- packages/e2e-tests/next-app/next.config.js | 41 ++++++ .../lambda-at-edge/src/default-handler.ts | 57 +++++---- .../lambda-at-edge/src/routing/matcher.ts | 28 +++++ .../lambda-at-edge/src/routing/redirector.ts | 109 ++++++++++++++++ ...h-routes-manifest-with-trailing-slash.json | 47 +++++++ .../default-basepath-routes-manifest.json | 42 ++++++- .../default-handler-with-basepath.test.ts | 108 ++++++++++------ .../default-handler/default-handler.test.ts | 107 ++++++++++------ ...t-routes-manifest-with-trailing-slash.json | 40 ++++++ .../default-routes-manifest.json | 35 +++++- .../default-handler/utils/runRedirectTest.ts | 41 ++++++ .../tests/routing/matcher.test.ts | 55 ++++++++ .../tests/routing/redirector.test.ts | 104 +++++++++++++++ packages/libs/lambda-at-edge/types.d.ts | 8 ++ 16 files changed, 836 insertions(+), 124 deletions(-) create mode 100644 packages/e2e-tests/next-app/next.config.js create mode 100644 packages/libs/lambda-at-edge/src/routing/matcher.ts create mode 100644 packages/libs/lambda-at-edge/src/routing/redirector.ts create mode 100644 packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json create mode 100644 packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json create mode 100644 packages/libs/lambda-at-edge/tests/default-handler/utils/runRedirectTest.ts create mode 100644 packages/libs/lambda-at-edge/tests/routing/matcher.test.ts create mode 100644 packages/libs/lambda-at-edge/tests/routing/redirector.test.ts diff --git a/packages/e2e-tests/next-app/cypress/integration/redirects.test.ts b/packages/e2e-tests/next-app/cypress/integration/redirects.test.ts index b972ed489e..f94a8c2c8e 100644 --- a/packages/e2e-tests/next-app/cypress/integration/redirects.test.ts +++ b/packages/e2e-tests/next-app/cypress/integration/redirects.test.ts @@ -7,19 +7,28 @@ describe("Redirects Tests", () => { describe("Pages redirect to non-trailing slash path", () => { [ - { path: "/ssr-page/" }, - { path: "/ssg-page/" }, - { path: "/errored-page/" }, - { path: "/errored-page-new-ssr/" }, - { path: "/unmatched/" } - ].forEach(({ path }) => { + { path: "/ssr-page/", expectedStatus: 200 }, + { path: "/ssg-page/", expectedStatus: 200 }, + { path: "/errored-page/", expectedStatus: 500 }, + { path: "/errored-page-new-ssr/", expectedStatus: 500 }, + { path: "/unmatched/", expectedStatus: 404 } + ].forEach(({ path, expectedStatus }) => { it(`redirects page ${path}`, () => { cy.ensureRouteHasStatusCode(path, 308); const redirectedPath = path.slice(0, -1); // Verify redirect response - cy.verifyPermanentRedirect(path, redirectedPath); + cy.verifyRedirect(path, redirectedPath, 308); + + // Verify status after following redirect + cy.request({ + url: path, + followRedirect: true, + failOnStatusCode: false + }).then((response) => { + expect(response.status).to.equal(expectedStatus); + }); // Visit to follow redirect cy.visit(path, { failOnStatusCode: false }); @@ -34,7 +43,7 @@ describe("Redirects Tests", () => { const redirectedPath = path.slice(0, -1); // Verify redirect response - cy.verifyPermanentRedirect(path, redirectedPath); + cy.verifyRedirect(path, redirectedPath, 308); // We can't use visit to follow redirect as it expects HTML content, not files. cy.request(path).then((response) => { @@ -56,7 +65,7 @@ describe("Redirects Tests", () => { const redirectedPath = fullPath.slice(0, -1); // Verify redirect response - cy.verifyPermanentRedirect(fullPath, redirectedPath); + cy.verifyRedirect(fullPath, redirectedPath, 308); // We can't use visit to follow redirect as it expects HTML content, not files. cy.request(fullPath).then((response) => { @@ -65,4 +74,96 @@ describe("Redirects Tests", () => { }); }); }); + + describe("Custom redirects defined in next.config.js", () => { + [ + { + path: "/permanent-redirect", + expectedRedirect: "/ssr-page", + expectedStatus: 200, + expectedRedirectStatus: 308 + }, + { + path: "/permanent-redirect?a=123", + expectedRedirect: "/ssr-page?a=123", + expectedStatus: 200, + expectedRedirectStatus: 308 + }, + { + path: "/temporary-redirect", + expectedRedirect: "/ssg-page", + expectedStatus: 200, + expectedRedirectStatus: 307 + }, + { + path: "/wildcard-redirect-1/a/b/c/d", + expectedRedirect: "/ssg-page", + expectedStatus: 200, + expectedRedirectStatus: 308 + }, + { + path: "/wildcard-redirect-1/a", + expectedRedirect: "/ssg-page", + expectedStatus: 200, + expectedRedirectStatus: 308 + }, + { + path: "/wildcard-redirect-2/a", // Redirects but the destination serves a 404 + expectedRedirect: "/wildcard-redirect-2-dest/a", + expectedStatus: 404, + expectedRedirectStatus: 308 + }, + { + path: "/regex-redirect-1/1234", + expectedRedirect: "/ssg-page", + expectedStatus: 200, + expectedRedirectStatus: 308 + }, + { + path: "/regex-redirect-1/abcd", // Not a redirect as the regex is for numbers only + expectedRedirect: null, + expectedStatus: null, + expectedRedirectStatus: null + }, + { + path: "/regex-redirect-2/12345", // Redirects but the destination serves a 404 + expectedRedirect: "/regex-redirect-2-dest/12345", + expectedStatus: 404, + expectedRedirectStatus: 308 + }, + { + path: "/custom-status-code-redirect", + expectedRedirect: "/ssr-page", + expectedStatus: 200, + expectedRedirectStatus: 302 + } + ].forEach( + ({ path, expectedRedirect, expectedStatus, expectedRedirectStatus }) => { + it(`redirects path ${path} to ${expectedRedirect}, redirect status: ${expectedRedirectStatus}`, () => { + if (expectedRedirect) { + // Verify redirect response + cy.verifyRedirect(path, expectedRedirect, expectedRedirectStatus); + + // Follow redirect without failing on status code + cy.request({ + url: path, + followRedirect: true, + failOnStatusCode: false + }).then((response) => { + expect(response.status).to.equal(expectedStatus); + }); + } else { + // If no redirect is expected, expect a 404 instead + cy.request({ + url: path, + followRedirect: false, + failOnStatusCode: false + }).then((response) => { + expect(response.status).to.equal(404); + }); + } + }); + } + ); + }); }); diff --git a/packages/e2e-tests/next-app/cypress/support/commands.ts b/packages/e2e-tests/next-app/cypress/support/commands.ts index 70e53b8e72..1f62813bd5 100644 --- a/packages/e2e-tests/next-app/cypress/support/commands.ts +++ b/packages/e2e-tests/next-app/cypress/support/commands.ts @@ -35,9 +35,10 @@ declare namespace Cypress { path: string | RegExp, status: number ) => Cypress.Chainable; - verifyPermanentRedirect: ( + verifyRedirect: ( path: string, - redirectedPath: string + redirectedPath: string, + redirectStatusCode: number ) => Cypress.Chainable; verifyResponseCacheStatus: ( response: Cypress.Response, @@ -110,12 +111,18 @@ Cypress.Commands.add( ); Cypress.Commands.add( - "verifyPermanentRedirect", - (path: string, redirectedPath: string) => { + "verifyRedirect", + (path: string, redirectedPath: string, redirectStatusCode: number) => { cy.request({ url: path, followRedirect: false }).then((response) => { - expect(response.status).to.equal(308); + expect(response.status).to.equal(redirectStatusCode); expect(response.headers["location"]).to.equal(redirectedPath); - expect(response.headers["refresh"]).to.equal(`0;url=${redirectedPath}`); + + if (redirectStatusCode === 308) { + // IE11 compatibility + expect(response.headers["refresh"]).to.equal(`0;url=${redirectedPath}`); + } else { + expect(response.headers["refresh"]).to.be.undefined; + } }); } ); diff --git a/packages/e2e-tests/next-app/next.config.js b/packages/e2e-tests/next-app/next.config.js new file mode 100644 index 0000000000..954f808467 --- /dev/null +++ b/packages/e2e-tests/next-app/next.config.js @@ -0,0 +1,41 @@ +module.exports = { + async redirects() { + return [ + { + source: "/permanent-redirect", + destination: "/ssr-page", + permanent: true + }, + { + source: "/temporary-redirect", + destination: "/ssg-page", + permanent: false + }, + { + source: "/custom-status-code-redirect", + destination: "/ssr-page", + statusCode: 302 + }, + { + source: "/wildcard-redirect-1/:slug*", + destination: "/ssg-page", + permanent: true + }, + { + source: "/wildcard-redirect-2/:slug*", + destination: "/wildcard-redirect-2-dest/:slug*", + permanent: true + }, + { + source: "/regex-redirect-1/:slug(\\d{1,})", + destination: "/ssg-page", + permanent: true + }, + { + source: "/regex-redirect-2/:slug(\\d{1,})", + destination: "/regex-redirect-2-dest/:slug", + permanent: true + } + ]; + } +}; diff --git a/packages/libs/lambda-at-edge/src/default-handler.ts b/packages/libs/lambda-at-edge/src/default-handler.ts index 1f7e008709..a66a0631a7 100644 --- a/packages/libs/lambda-at-edge/src/default-handler.ts +++ b/packages/libs/lambda-at-edge/src/default-handler.ts @@ -3,7 +3,7 @@ import PrerenderManifest from "./prerender-manifest.json"; // @ts-ignore import Manifest from "./manifest.json"; // @ts-ignore -import { basePath } from "./routes-manifest.json"; +import RoutesManifestJson from "./routes-manifest.json"; import lambdaAtEdgeCompat from "@sls-next/next-aws-cloudfront"; import cookie from "cookie"; import { @@ -17,13 +17,16 @@ import { OriginRequestDefaultHandlerManifest, PreRenderedManifest as PrerenderManifestType, OriginResponseEvent, - PerfLogger + PerfLogger, + RoutesManifest } from "../types"; import { performance } from "perf_hooks"; import { ServerResponse } from "http"; import jsonwebtoken from "jsonwebtoken"; import type { Readable } from "stream"; +import { createRedirectResponse, getRedirectPath } from "./routing/redirector"; +const basePath = RoutesManifestJson.basePath; const NEXT_PREVIEW_DATA_COOKIE = "__next_preview_data"; const NEXT_PRERENDER_BYPASS_COOKIE = "__prerender_bypass"; const defaultPreviewCookies = { @@ -166,6 +169,7 @@ export const handler = async ( const manifest: OriginRequestDefaultHandlerManifest = Manifest; let response: CloudFrontResultResponse | CloudFrontRequest; const prerenderManifest: PrerenderManifestType = PrerenderManifest; + const routesManifest: RoutesManifest = RoutesManifestJson; const { now, log } = perfLogger(manifest.logLambdaExecutionTimes); @@ -181,7 +185,8 @@ export const handler = async ( response = await handleOriginRequest({ event, manifest, - prerenderManifest + prerenderManifest, + routesManifest }); } @@ -195,19 +200,25 @@ export const handler = async ( const handleOriginRequest = async ({ event, manifest, - prerenderManifest + prerenderManifest, + routesManifest }: { event: OriginRequestEvent; manifest: OriginRequestDefaultHandlerManifest; prerenderManifest: PrerenderManifestType; + routesManifest: RoutesManifest; }) => { + const basePath = routesManifest.basePath; const request = event.Records[0].cf.request; const uri = normaliseUri(request.uri); const { pages, publicFiles } = manifest; const isPublicFile = publicFiles[uri]; const isDataReq = isDataRequest(uri); - // Handle any redirects + // Handle redirects + // TODO: refactor redirect logic to another file since this is getting quite large + + // Handle any trailing slash redirects let newUri = request.uri; if (isDataReq || isPublicFile) { // Data requests and public files with trailing slash URL always get redirected to non-trailing slash URL @@ -217,7 +228,7 @@ const handleOriginRequest = async ({ } else if (request.uri !== "/" && request.uri !== "" && uri !== "/404") { // HTML/SSR pages get redirected based on trailingSlash in next.config.js // We do not redirect: - // 1. Unnormalised URI is"/" or "" as this could cause a redirect loop due to browsers appending trailing slash + // 1. Unnormalised URI is "/" or "" as this could cause a redirect loop due to browsers appending trailing slash // 2. "/404" pages due to basePath normalisation const trailingSlash = manifest.trailingSlash; @@ -231,7 +242,17 @@ const handleOriginRequest = async ({ } if (newUri !== request.uri) { - return createRedirectResponse(newUri, request.querystring); + return createRedirectResponse(newUri, request.querystring, 308); + } + + // Handle other custom redirects on the original URI + const customRedirect = getRedirectPath(request.uri, routesManifest); + if (customRedirect) { + return createRedirectResponse( + customRedirect.redirectPath, + request.querystring, + customRedirect.statusCode + ); } const isStaticPage = pages.html.nonDynamic[uri]; @@ -483,28 +504,6 @@ const hasFallbackForUri = ( }); }; -const createRedirectResponse = (uri: string, querystring: string) => { - const location = querystring ? `${uri}?${querystring}` : uri; - return { - status: "308", - statusDescription: "Permanent Redirect", - headers: { - location: [ - { - key: "Location", - value: location - } - ], - refresh: [ - { - key: "Refresh", - value: `0;url=${location}` - } - ] - } - }; -}; - // This sets CloudFront response for 404 or 500 statuses const setCloudFrontResponseStatus = ( response: CloudFrontResultResponse, diff --git a/packages/libs/lambda-at-edge/src/routing/matcher.ts b/packages/libs/lambda-at-edge/src/routing/matcher.ts new file mode 100644 index 0000000000..2734ee6228 --- /dev/null +++ b/packages/libs/lambda-at-edge/src/routing/matcher.ts @@ -0,0 +1,28 @@ +/** + Provides matching capabilities to support custom redirects, rewrites, and headers. + */ + +import { compile, Match, match } from "path-to-regexp"; + +/** + * Match the given path against a source path. + * @param path + * @param source + */ +export function matchPath(path: string, source: string): Match { + const matcher = match(source, { decode: decodeURIComponent }); + return matcher(path); +} + +/** + * Compile a destination for redirects or rewrites. + * @param destination + * @param params + */ +export function compileDestination( + destination: string, + params: object +): string { + const toPath = compile(destination, { encode: encodeURIComponent }); + return toPath(params); +} diff --git a/packages/libs/lambda-at-edge/src/routing/redirector.ts b/packages/libs/lambda-at-edge/src/routing/redirector.ts new file mode 100644 index 0000000000..d5c523cb9a --- /dev/null +++ b/packages/libs/lambda-at-edge/src/routing/redirector.ts @@ -0,0 +1,109 @@ +import { compileDestination, matchPath } from "./matcher"; +import { RedirectData, RoutesManifest } from "../../types"; +import * as http from "http"; + +/** + * Whether this is the default trailing slash redirect. + * @param redirect + * @param basePath + */ +function isTrailingSlashRedirect(redirect: RedirectData, basePath: string) { + if (basePath !== "") { + return ( + redirect.statusCode === 308 && + ((redirect.source === `${basePath}` && + redirect.destination === `${basePath}/`) || + (redirect.source === `${basePath}/` && + redirect.destination === `${basePath}`) || + (redirect.source === `${basePath}/:path+/` && + redirect.destination === `${basePath}/:path+`) || + (redirect.source === `${basePath}/:file((?:[^/]+/)*[^/]+\\.\\w+)/` && + redirect.destination === `${basePath}/:file`) || + (redirect.source === `${basePath}/:notfile((?:[^/]+/)*[^/\\.]+)` && + redirect.destination === `${basePath}/:notfile/`)) + ); + } else { + return ( + redirect.statusCode === 308 && + ((redirect.source === "/:path+/" && redirect.destination === "/:path+") || + (redirect.source === "/:path+" && + redirect.destination === "/:path+/") || + (redirect.source === "/:file((?:[^/]+/)*[^/]+\\.\\w+)/" && + redirect.destination === "/:file") || + (redirect.source === "/:notfile((?:[^/]+/)*[^/\\.]+)" && + redirect.destination === "/:notfile/")) + ); + } +} + +/** + * Get the redirect of the given path, if it exists. Otherwise return null. + * @param path + * @param routesManifest + */ +export function getRedirectPath( + path: string, + routesManifest: RoutesManifest +): { redirectPath: string; statusCode: number } | null { + const redirects: RedirectData[] = routesManifest.redirects; + + for (const redirect of redirects) { + // Skip default trailing slash redirects as it already handled without using regex + if (isTrailingSlashRedirect(redirect, routesManifest.basePath)) { + continue; + } + + const match = matchPath(path, redirect.source); + + if (match) { + return { + redirectPath: compileDestination(redirect.destination, match.params), + statusCode: redirect.statusCode + }; + } + } + + return null; +} + +/** + * Create a redirect response with the given status code for CloudFront. + * @param uri + * @param querystring + * @param statusCode + */ +export function createRedirectResponse( + uri: string, + querystring: string, + statusCode: number +) { + const location = querystring ? `${uri}?${querystring}` : uri; + + const status = statusCode.toString(); + const statusDescription = http.STATUS_CODES[status]; + + const refresh = + statusCode === 308 + ? [ + // Required for IE11 compatibility + { + key: "Refresh", + value: `0;url=${location}` + } + ] + : []; + + return { + status: status, + statusDescription: statusDescription, + headers: { + location: [ + { + key: "Location", + value: location + } + ], + refresh: refresh + } + }; +} diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json new file mode 100644 index 0000000000..f5d349309b --- /dev/null +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json @@ -0,0 +1,47 @@ +{ + "version": 1, + "pages404": true, + "basePath": "/basepath", + "redirects": [ + { + "source": "/basepath", + "destination": "/basepath/", + "basePath": false, + "statusCode": 308, + "regex": "^/basepath$" + }, + { + "source": "/basepath/:file((?:[^/]+/)*[^/]+\\.\\w+)/", + "destination": "/basepath/:file", + "statusCode": 308, + "regex": "^/basepath(?:/((?:[^/]+/)*[^/]+\\.\\w+))/$" + }, + { + "source": "/basepath/:notfile((?:[^/]+/)*[^/\\.]+)", + "destination": "/basepath/:notfile/", + "statusCode": 308, + "regex": "^/basepath(?:/((?:[^/]+/)*[^/\\.]+))$" + }, + { + "source": "/basepath/old-blog/:slug/", + "destination": "/basepath/news/:slug/", + "statusCode": 308, + "regex": "^/basepath/old-blog(?:/([^/]+?))/$" + }, + { + "source": "/basepath/terms-new/", + "destination": "/basepath/terms/", + "statusCode": 308, + "regex": "^/basepath/terms-new/$" + }, + { + "source": "/basepath/old-users/:post(\\d{1,})/", + "destination": "/basepath/users/:post/", + "statusCode": 307, + "regex": "^/basepath/old-users(?:/(\\d{1,}))/$" + } + ], + "rewrites": [], + "headers": [], + "dynamicRoutes": [] +} diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json index 369e46e2c5..23d730ced1 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json @@ -1 +1,41 @@ -{"version":1,"pages404":true,"basePath":"/basepath","redirects":[],"rewrites":[],"headers":[],"dynamicRoutes":[]} +{ + "version": 1, + "pages404": true, + "basePath": "/basepath", + "redirects": [ + { + "source": "/basepath/", + "destination": "/basepath", + "basePath": false, + "statusCode": 308, + "regex": "^/basepath/$" + }, + { + "source": "/basepath/:path+/", + "destination": "/basepath/:path+", + "statusCode": 308, + "regex": "^/basepath(?:/((?:[^/]+?)(?:/(?:[^/]+?))*))/$" + }, + { + "source": "/basepath/old-blog/:slug", + "destination": "/basepath/news/:slug", + "statusCode": 308, + "regex": "^/basepath/old-blog(?:/([^/]+?))$" + }, + { + "source": "/basepath/terms-new", + "destination": "/basepath/terms", + "statusCode": 308, + "regex": "^/basepath/terms-new$" + }, + { + "source": "/basepath/old-users/:post(\\d{1,})", + "destination": "/basepath/users/:post", + "statusCode": 307, + "regex": "^/basepath/old-users(?:/(\\d{1,}))$" + } + ], + "rewrites": [], + "headers": [], + "dynamicRoutes": [] +} diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts b/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts index 5728a96d56..c354849bb7 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts @@ -4,6 +4,7 @@ import { CloudFrontResultResponse, CloudFrontOrigin } from "aws-lambda"; +import { runRedirectTestWithHandler } from "./utils/runRedirectTest"; jest.mock( "../../src/prerender-manifest.json", @@ -13,14 +14,6 @@ jest.mock( } ); -jest.mock( - "../../src/routes-manifest.json", - () => require("./default-basepath-routes-manifest.json"), - { - virtual: true - } -); - const mockPageRequire = (mockPagePath: string): void => { jest.mock( `../../src/${mockPagePath}`, @@ -50,6 +43,7 @@ describe("Lambda@Edge", () => { let runRedirectTest: ( path: string, expectedRedirect: string, + statusCode: number, querystring?: string ) => Promise; beforeEach(() => { @@ -63,6 +57,15 @@ describe("Lambda@Edge", () => { virtual: true } ); + + jest.mock( + "../../src/routes-manifest.json", + () => + require("./default-basepath-routes-manifest-with-trailing-slash.json"), + { + virtual: true + } + ); } else { jest.mock( "../../src/manifest.json", @@ -71,6 +74,14 @@ describe("Lambda@Edge", () => { virtual: true } ); + + jest.mock( + "../../src/routes-manifest.json", + () => require("./default-basepath-routes-manifest.json"), + { + virtual: true + } + ); } // Handler needs to be dynamically required to use above mocked manifests @@ -80,33 +91,16 @@ describe("Lambda@Edge", () => { runRedirectTest = async ( path: string, expectedRedirect: string, + statusCode: number, querystring?: string ): Promise => { - const event = createCloudFrontEvent({ - uri: path, - host: "mydistribution.cloudfront.net", - config: { eventType: "origin-request" } as any, - querystring: querystring - }); - - const result = await handler(event); - const response = result as CloudFrontResultResponse; - - expect(response.headers).toEqual({ - location: [ - { - key: "Location", - value: expectedRedirect - } - ], - refresh: [ - { - key: "Refresh", - value: `0;url=${expectedRedirect}` - } - ] - }); - expect(response.status).toEqual("308"); + await runRedirectTestWithHandler( + handler, + path, + expectedRedirect, + statusCode, + querystring + ); }; }); @@ -176,7 +170,7 @@ describe("Lambda@Edge", () => { expectedRedirect = path; path += "/"; } - await runRedirectTest(path, expectedRedirect); + await runRedirectTest(path, expectedRedirect, 308); } ); @@ -240,7 +234,7 @@ describe("Lambda@Edge", () => { `( "public files always redirect to path without trailing slash: $path -> $expectedRedirect", async ({ path, expectedRedirect }) => { - await runRedirectTest(path, expectedRedirect); + await runRedirectTest(path, expectedRedirect, 308); } ); }); @@ -303,7 +297,7 @@ describe("Lambda@Edge", () => { expectedRedirect = path; path += "/"; } - await runRedirectTest(path, expectedRedirect); + await runRedirectTest(path, expectedRedirect, 308); } ); @@ -327,7 +321,7 @@ describe("Lambda@Edge", () => { path += "/"; } - await runRedirectTest(path, expectedRedirect, querystring); + await runRedirectTest(path, expectedRedirect, 308, querystring); }); }); @@ -402,7 +396,7 @@ describe("Lambda@Edge", () => { `( "data requests always redirect to path without trailing slash: $path -> $expectedRedirect", async ({ path, expectedRedirect }) => { - await runRedirectTest(path, expectedRedirect); + await runRedirectTest(path, expectedRedirect, 308); } ); }); @@ -514,7 +508,7 @@ describe("Lambda@Edge", () => { expectedRedirect = path; path += "/"; } - await runRedirectTest(path, expectedRedirect); + await runRedirectTest(path, expectedRedirect, 308); }); // Next.js serves 404 on pages that do not have basepath prefix. It doesn't redirect whether there is trailing slash or not. @@ -621,5 +615,41 @@ describe("Lambda@Edge", () => { expect(response.status).toEqual("500"); }); }); + + describe("Custom Redirects", () => { + if (trailingSlash) { + it.each` + path | expectedRedirect | expectedRedirectStatusCode + ${"/basepath/terms-new/"} | ${"/basepath/terms/"} | ${308} + ${"/basepath/old-blog/abc/"} | ${"/basepath/news/abc/"} | ${308} + ${"/basepath/old-users/1234/"} | ${"/basepath/users/1234/"} | ${307} + `( + "redirects path $path to $expectedRedirect, permanent: $permanent, expectedRedirectStatusCode: $expectedRedirectStatusCode", + async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { + await runRedirectTest( + path, + expectedRedirect, + expectedRedirectStatusCode + ); + } + ); + } else { + it.each` + path | expectedRedirect | expectedRedirectStatusCode + ${"/basepath/terms-new"} | ${"/basepath/terms"} | ${308} + ${"/basepath/old-blog/abc"} | ${"/basepath/news/abc"} | ${308} + ${"/basepath/old-users/1234"} | ${"/basepath/users/1234"} | ${307} + `( + "redirects path $path to $expectedRedirect, expectedRedirectStatusCode: $expectedRedirectStatusCode", + async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { + await runRedirectTest( + path, + expectedRedirect, + expectedRedirectStatusCode + ); + } + ); + } + }); }); }); diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts b/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts index aaa38b1807..04d6e95f6e 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts @@ -4,6 +4,7 @@ import { CloudFrontResultResponse, CloudFrontOrigin } from "aws-lambda"; +import { runRedirectTestWithHandler } from "./utils/runRedirectTest"; jest.mock("jsonwebtoken", () => ({ verify: jest.fn() @@ -17,14 +18,6 @@ jest.mock( } ); -jest.mock( - "../../src/routes-manifest.json", - () => require("./default-routes-manifest.json"), - { - virtual: true - } -); - const mockPageRequire = (mockPagePath: string): void => { jest.mock( `../../src/${mockPagePath}`, @@ -54,6 +47,7 @@ describe("Lambda@Edge", () => { let runRedirectTest: ( path: string, expectedRedirect: string, + statusCode: number, querystring?: string ) => Promise; beforeEach(() => { @@ -67,6 +61,14 @@ describe("Lambda@Edge", () => { virtual: true } ); + + jest.mock( + "../../src/routes-manifest.json", + () => require("./default-routes-manifest-with-trailing-slash.json"), + { + virtual: true + } + ); } else { jest.mock( "../../src/manifest.json", @@ -75,6 +77,14 @@ describe("Lambda@Edge", () => { virtual: true } ); + + jest.mock( + "../../src/routes-manifest.json", + () => require("./default-routes-manifest.json"), + { + virtual: true + } + ); } // Handler needs to be dynamically required to use above mocked manifests @@ -84,33 +94,16 @@ describe("Lambda@Edge", () => { runRedirectTest = async ( path: string, expectedRedirect: string, + statusCode: number, querystring?: string ): Promise => { - const event = createCloudFrontEvent({ - uri: path, - host: "mydistribution.cloudfront.net", - config: { eventType: "origin-request" } as any, - querystring: querystring - }); - - const result = await handler(event); - const response = result as CloudFrontResultResponse; - - expect(response.headers).toEqual({ - location: [ - { - key: "Location", - value: expectedRedirect - } - ], - refresh: [ - { - key: "Refresh", - value: `0;url=${expectedRedirect}` - } - ] - }); - expect(response.status).toEqual("308"); + await runRedirectTestWithHandler( + handler, + path, + expectedRedirect, + statusCode, + querystring + ); }; }); @@ -183,7 +176,7 @@ describe("Lambda@Edge", () => { expectedRedirect = path; path += "/"; } - await runRedirectTest(path, expectedRedirect); + await runRedirectTest(path, expectedRedirect, 308); } ); @@ -272,7 +265,7 @@ describe("Lambda@Edge", () => { `( "public files always redirect to path without trailing slash: $path -> $expectedRedirect", async ({ path, expectedRedirect }) => { - await runRedirectTest(path, expectedRedirect); + await runRedirectTest(path, expectedRedirect, 308); } ); }); @@ -336,7 +329,7 @@ describe("Lambda@Edge", () => { expectedRedirect = path; path += "/"; } - await runRedirectTest(path, expectedRedirect); + await runRedirectTest(path, expectedRedirect, 308); } ); @@ -360,7 +353,7 @@ describe("Lambda@Edge", () => { path += "/"; } - await runRedirectTest(path, expectedRedirect, querystring); + await runRedirectTest(path, expectedRedirect, 308, querystring); }); }); @@ -435,7 +428,7 @@ describe("Lambda@Edge", () => { `( "data requests always redirect to path without trailing slash: $path -> $expectedRedirect", async ({ path, expectedRedirect }) => { - await runRedirectTest(path, expectedRedirect); + await runRedirectTest(path, expectedRedirect, 308); } ); @@ -574,7 +567,7 @@ describe("Lambda@Edge", () => { expectedRedirect = path; path += "/"; } - await runRedirectTest(path, expectedRedirect); + await runRedirectTest(path, expectedRedirect, 308); }); it.each` @@ -642,5 +635,41 @@ describe("Lambda@Edge", () => { expect(response.status).toEqual("500"); }); }); + + describe("Custom Redirects", () => { + if (trailingSlash) { + it.each` + path | expectedRedirect | expectedRedirectStatusCode + ${"/terms-new/"} | ${"/terms/"} | ${308} + ${"/old-blog/abc/"} | ${"/news/abc/"} | ${308} + ${"/old-users/1234/"} | ${"/users/1234/"} | ${307} + `( + "redirects path $path to $expectedRedirect, expectedRedirectStatusCode: expectedRedirectStatusCode", + async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { + await runRedirectTest( + path, + expectedRedirect, + expectedRedirectStatusCode + ); + } + ); + } else { + it.each` + path | expectedRedirect | expectedRedirectStatusCode + ${"/terms-new"} | ${"/terms"} | ${308} + ${"/old-blog/abc"} | ${"/news/abc"} | ${308} + ${"/old-users/1234"} | ${"/users/1234"} | ${307} + `( + "redirects path $path to $expectedRedirect, permanent: $permanent, expectedRedirectStatusCode: $expectedRedirectStatusCode", + async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { + await runRedirectTest( + path, + expectedRedirect, + expectedRedirectStatusCode + ); + } + ); + } + }); }); }); diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json new file mode 100644 index 0000000000..828654584a --- /dev/null +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json @@ -0,0 +1,40 @@ +{ + "version": 1, + "pages404": true, + "basePath": "", + "redirects": [ + { + "source": "/:file((?:[^/]+/)*[^/]+\\.\\w+)/", + "destination": "/:file", + "statusCode": 308, + "regex": "^(?:/((?:[^/]+/)*[^/]+\\.\\w+))/$" + }, + { + "source": "/:notfile((?:[^/]+/)*[^/\\.]+)", + "destination": "/:notfile/", + "statusCode": 308, + "regex": "^(?:/((?:[^/]+/)*[^/\\.]+))$" + }, + { + "source": "/old-blog/:slug/", + "destination": "/news/:slug/", + "statusCode": 308, + "regex": "^/old-blog(?:/([^/]+?))/$" + }, + { + "source": "/terms-new/", + "destination": "/terms/", + "statusCode": 308, + "regex": "^/terms/$" + }, + { + "source": "/old-users/:id(\\d{1,})/", + "destination": "/users/:id/", + "statusCode": 307, + "regex": "^/old-users(?:/(\\d{1,}))/$" + } + ], + "rewrites": [], + "headers": [], + "dynamicRoutes": [] +} diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json index 46a422f819..96fcb4411c 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json @@ -1 +1,34 @@ -{"version":1,"pages404":true,"basePath":"","redirects":[],"rewrites":[],"headers":[],"dynamicRoutes":[]} +{ + "version": 1, + "pages404": true, + "basePath": "", + "redirects": [ + { + "source": "/:path+/", + "destination": "/:path+", + "statusCode": 308, + "regex": "^(?:/((?:[^/]+?)(?:/(?:[^/]+?))*))/$" + }, + { + "source": "/old-blog/:slug", + "destination": "/news/:slug", + "statusCode": 308, + "regex": "^/old-blog(?:/([^/]+?))$" + }, + { + "source": "/terms-new", + "destination": "/terms", + "statusCode": 308, + "regex": "^/terms$" + }, + { + "source": "/old-users/:id(\\d{1,})", + "destination": "/users/:id", + "statusCode": 307, + "regex": "^/old-users(?:/(\\d{1,}))$" + } + ], + "rewrites": [], + "headers": [], + "dynamicRoutes": [] +} diff --git a/packages/libs/lambda-at-edge/tests/default-handler/utils/runRedirectTest.ts b/packages/libs/lambda-at-edge/tests/default-handler/utils/runRedirectTest.ts new file mode 100644 index 0000000000..e00ae96ef5 --- /dev/null +++ b/packages/libs/lambda-at-edge/tests/default-handler/utils/runRedirectTest.ts @@ -0,0 +1,41 @@ +import { createCloudFrontEvent } from "../../test-utils"; +import { CloudFrontResultResponse } from "aws-lambda"; + +export async function runRedirectTestWithHandler( + handler: Function, + path: string, + expectedRedirect: string, + statusCode: number, + querystring?: string +): Promise { + const event = createCloudFrontEvent({ + uri: path, + host: "mydistribution.cloudfront.net", + config: { eventType: "origin-request" } as any, + querystring: querystring + }); + + const result = await handler(event); + const response = result as CloudFrontResultResponse; + + const refresh: [{ key: string; value: string }] | [] = + statusCode === 308 + ? [ + { + key: "Refresh", + value: `0;url=${expectedRedirect}` + } + ] + : []; + + expect(response.headers).toEqual({ + location: [ + { + key: "Location", + value: expectedRedirect + } + ], + refresh: refresh + }); + expect(response.status).toEqual(statusCode.toString()); +} diff --git a/packages/libs/lambda-at-edge/tests/routing/matcher.test.ts b/packages/libs/lambda-at-edge/tests/routing/matcher.test.ts new file mode 100644 index 0000000000..d717004348 --- /dev/null +++ b/packages/libs/lambda-at-edge/tests/routing/matcher.test.ts @@ -0,0 +1,55 @@ +import { compileDestination, matchPath } from "../../src/routing/matcher"; + +// We don't really need to test much here since the underlying path-to-regexp is well-tested. +describe("Matcher Tests", () => { + describe("matchPath()", () => { + it("matches simple path", () => { + const match = matchPath("/terms", "/terms"); + expect(match).toEqual({ path: "/terms", index: 0, params: {} }); + }); + + it("matches parameterized path", () => { + const match = matchPath("/user/1234", "/user/:id"); + expect(match).toEqual({ + path: "/user/1234", + index: 0, + params: { id: "1234" } + }); + }); + + it("matches wildcards", () => { + const match = matchPath("/user/a/b/c/d/e", "/user/:slug*"); + expect(match).toEqual({ + path: "/user/a/b/c/d/e", + index: 0, + params: { slug: ["a", "b", "c", "d", "e"] } + }); + }); + + it("matches regex", () => { + const match = matchPath("/user/12345", "/user/:slug(\\d{1,})"); + expect(match).toEqual({ + path: "/user/12345", + index: 0, + params: { slug: "12345" } + }); + }); + + it("does not match regex", () => { + const match = matchPath("/user/abcd", "/user/:slug(\\d{1,})"); + expect(match).toBe(false); + }); + }); + + describe("compileDestination()", () => { + it("compiles simple destination", () => { + const match = compileDestination("/about", {}); + expect(match).toEqual("/about"); + }); + + it("compiles parameterized destination", () => { + const match = compileDestination("/about/:a/:b", { a: "123", b: "456" }); + expect(match).toEqual("/about/123/456"); + }); + }); +}); diff --git a/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts b/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts new file mode 100644 index 0000000000..abe64b7d6d --- /dev/null +++ b/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts @@ -0,0 +1,104 @@ +import { + createRedirectResponse, + getRedirectPath +} from "../../src/routing/redirector"; +import { RoutesManifest } from "../../types"; + +describe("Redirector Tests", () => { + describe("getRedirectPath()", () => { + let routesManifest: RoutesManifest; + + beforeAll(() => { + routesManifest = { + basePath: "", + redirects: [ + { + source: "/:path+/", + destination: "/:path+", + statusCode: 308, + regex: "^(?:/((?:[^/]+?)(?:/(?:[^/]+?))*))/$" + }, + { + source: "/old-blog/:slug", + destination: "/news/:slug", + statusCode: 308, + regex: "^/old-blog(?:/([^/]+?))$" + }, + { source: "/a", destination: "/b", statusCode: 308, regex: "^/a$" }, + { source: "/c", destination: "/d", statusCode: 302, regex: "^/c$" }, + { + source: "/old-users/:id(\\d{1,})", + destination: "/users/:id", + statusCode: 307, + regex: "^/old-users(?:/(\\d{1,}))$" + } + ] + }; + }); + + it.each` + path | expectedRedirect | expectedStatusCode + ${"/a"} | ${"/b"} | ${308} + ${"/c"} | ${"/d"} | ${302} + ${"/old-blog/abc"} | ${"/news/abc"} | ${308} + ${"/old-users/1234"} | ${"/users/1234"} | ${307} + ${"/old-users/abc"} | ${null} | ${null} + `( + "redirects path $path to $expectedRedirect", + ({ path, expectedRedirect, expectedStatusCode }) => { + const redirect = getRedirectPath(path, routesManifest); + + if (expectedRedirect) { + expect(redirect).toEqual({ + redirectPath: expectedRedirect, + statusCode: expectedStatusCode + }); + } else { + expect(redirect).toBeNull(); + } + } + ); + }); + + describe("createRedirectResponse()", () => { + it("does a permanent redirect", () => { + const response = createRedirectResponse("/terms", "", 308); + expect(response).toEqual({ + status: "308", + statusDescription: "Permanent Redirect", + headers: { + location: [ + { + key: "Location", + value: "/terms" + } + ], + refresh: [ + // Required for IE11 compatibility + { + key: "Refresh", + value: `0;url=/terms` + } + ] + } + }); + }); + + it("does a temporary redirect with query parameters", () => { + const response = createRedirectResponse("/terms", "a=123", 307); + expect(response).toEqual({ + status: "307", + statusDescription: "Temporary Redirect", + headers: { + location: [ + { + key: "Location", + value: "/terms?a=123" + } + ], + refresh: [] + } + }); + }); + }); +}); diff --git a/packages/libs/lambda-at-edge/types.d.ts b/packages/libs/lambda-at-edge/types.d.ts index 84264fd1c1..810bdb3efa 100644 --- a/packages/libs/lambda-at-edge/types.d.ts +++ b/packages/libs/lambda-at-edge/types.d.ts @@ -85,8 +85,16 @@ export type PreRenderedManifest = { }; }; +export type RedirectData = { + statusCode: number; + source: string; + destination: string; + regex: string; +}; + export type RoutesManifest = { basePath: string; + redirects: RedirectData[]; }; export type PerfLogger = { From 15558af81e6c0d5b3a2b25b471fdc26380561fbc Mon Sep 17 00:00:00 2001 From: Daniel Phang Date: Sun, 27 Sep 2020 01:41:20 -0700 Subject: [PATCH 2/7] fix up test description --- .../tests/default-handler/default-handler-with-basepath.test.ts | 2 +- .../tests/default-handler/default-handler.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts b/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts index c354849bb7..b8fccbe7b0 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts @@ -624,7 +624,7 @@ describe("Lambda@Edge", () => { ${"/basepath/old-blog/abc/"} | ${"/basepath/news/abc/"} | ${308} ${"/basepath/old-users/1234/"} | ${"/basepath/users/1234/"} | ${307} `( - "redirects path $path to $expectedRedirect, permanent: $permanent, expectedRedirectStatusCode: $expectedRedirectStatusCode", + "redirects path $path to $expectedRedirect, expectedRedirectStatusCode: $expectedRedirectStatusCode", async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { await runRedirectTest( path, diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts b/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts index 04d6e95f6e..97de192984 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts @@ -660,7 +660,7 @@ describe("Lambda@Edge", () => { ${"/old-blog/abc"} | ${"/news/abc"} | ${308} ${"/old-users/1234"} | ${"/users/1234"} | ${307} `( - "redirects path $path to $expectedRedirect, permanent: $permanent, expectedRedirectStatusCode: $expectedRedirectStatusCode", + "redirects path $path to $expectedRedirect, expectedRedirectStatusCode: $expectedRedirectStatusCode", async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { await runRedirectTest( path, From e3780fdb8b8c14e58cc6aa1cde189b1fb6f5f14a Mon Sep 17 00:00:00 2001 From: Daniel Phang Date: Sun, 27 Sep 2020 18:41:31 -0700 Subject: [PATCH 3/7] remove default redirects during build step --- packages/libs/lambda-at-edge/src/build.ts | 22 +++++++++++++++++-- .../lambda-at-edge/src/routing/redirector.ts | 11 +++++----- ...h-routes-manifest-with-trailing-slash.json | 19 ---------------- .../default-basepath-routes-manifest.json | 13 ----------- .../default-handler-with-basepath.test.ts | 2 ++ .../default-handler/default-handler.test.ts | 4 +++- ...t-routes-manifest-with-trailing-slash.json | 12 ---------- .../default-routes-manifest.json | 6 ----- 8 files changed, 30 insertions(+), 59 deletions(-) diff --git a/packages/libs/lambda-at-edge/src/build.ts b/packages/libs/lambda-at-edge/src/build.ts index 58127beec9..ace9544ca8 100644 --- a/packages/libs/lambda-at-edge/src/build.ts +++ b/packages/libs/lambda-at-edge/src/build.ts @@ -7,7 +7,8 @@ import path from "path"; import { getSortedRoutes } from "./lib/sortedRoutes"; import { OriginRequestDefaultHandlerManifest, - OriginRequestApiHandlerManifest + OriginRequestApiHandlerManifest, + RoutesManifest } from "../types"; import isDynamicRoute from "./lib/isDynamicRoute"; import pathToPosix from "./lib/pathToPosix"; @@ -15,6 +16,7 @@ import expressifyDynamicRoute from "./lib/expressifyDynamicRoute"; import pathToRegexStr from "./lib/pathToRegexStr"; import normalizeNodeModules from "./lib/normalizeNodeModules"; import createServerlessConfig from "./lib/createServerlessConfig"; +import { isTrailingSlashRedirect } from "./routing/redirector"; export const DEFAULT_LAMBDA_CODE_DIR = "default-lambda"; export const API_LAMBDA_CODE_DIR = "api-lambda"; @@ -159,6 +161,22 @@ class Builder { return false; } + /** + * Process and copy RoutesManifest. + * @param source + * @param destination + */ + async processAndCopyRoutesManifest(source: string, destination: string) { + const routesManifest = require(source) as RoutesManifest; + + // Remove default trailing slash redirects as they are already handled without regex matching. + routesManifest.redirects = routesManifest.redirects.filter((redirect) => { + return !isTrailingSlashRedirect(redirect, routesManifest.basePath); + }); + + await fse.writeFile(destination, JSON.stringify(routesManifest)); + } + async buildDefaultLambda( buildManifest: OriginRequestDefaultHandlerManifest ): Promise { @@ -245,7 +263,7 @@ class Builder { join(this.dotNextDir, "prerender-manifest.json"), join(this.outputDir, DEFAULT_LAMBDA_CODE_DIR, "prerender-manifest.json") ), - fse.copy( + this.processAndCopyRoutesManifest( join(this.dotNextDir, "routes-manifest.json"), join(this.outputDir, DEFAULT_LAMBDA_CODE_DIR, "routes-manifest.json") ) diff --git a/packages/libs/lambda-at-edge/src/routing/redirector.ts b/packages/libs/lambda-at-edge/src/routing/redirector.ts index d5c523cb9a..a8ab15a61d 100644 --- a/packages/libs/lambda-at-edge/src/routing/redirector.ts +++ b/packages/libs/lambda-at-edge/src/routing/redirector.ts @@ -4,10 +4,14 @@ import * as http from "http"; /** * Whether this is the default trailing slash redirect. + * This should only be used during build step to remove unneeded redirect paths. * @param redirect * @param basePath */ -function isTrailingSlashRedirect(redirect: RedirectData, basePath: string) { +export function isTrailingSlashRedirect( + redirect: RedirectData, + basePath: string +) { if (basePath !== "") { return ( redirect.statusCode === 308 && @@ -48,11 +52,6 @@ export function getRedirectPath( const redirects: RedirectData[] = routesManifest.redirects; for (const redirect of redirects) { - // Skip default trailing slash redirects as it already handled without using regex - if (isTrailingSlashRedirect(redirect, routesManifest.basePath)) { - continue; - } - const match = matchPath(path, redirect.source); if (match) { diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json index f5d349309b..929e9c4264 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json @@ -3,25 +3,6 @@ "pages404": true, "basePath": "/basepath", "redirects": [ - { - "source": "/basepath", - "destination": "/basepath/", - "basePath": false, - "statusCode": 308, - "regex": "^/basepath$" - }, - { - "source": "/basepath/:file((?:[^/]+/)*[^/]+\\.\\w+)/", - "destination": "/basepath/:file", - "statusCode": 308, - "regex": "^/basepath(?:/((?:[^/]+/)*[^/]+\\.\\w+))/$" - }, - { - "source": "/basepath/:notfile((?:[^/]+/)*[^/\\.]+)", - "destination": "/basepath/:notfile/", - "statusCode": 308, - "regex": "^/basepath(?:/((?:[^/]+/)*[^/\\.]+))$" - }, { "source": "/basepath/old-blog/:slug/", "destination": "/basepath/news/:slug/", diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json index 23d730ced1..d8218573f2 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json @@ -3,19 +3,6 @@ "pages404": true, "basePath": "/basepath", "redirects": [ - { - "source": "/basepath/", - "destination": "/basepath", - "basePath": false, - "statusCode": 308, - "regex": "^/basepath/$" - }, - { - "source": "/basepath/:path+/", - "destination": "/basepath/:path+", - "statusCode": 308, - "regex": "^/basepath(?:/((?:[^/]+?)(?:/(?:[^/]+?))*))/$" - }, { "source": "/basepath/old-blog/:slug", "destination": "/basepath/news/:slug", diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts b/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts index b8fccbe7b0..6054a327f5 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts @@ -58,6 +58,7 @@ describe("Lambda@Edge", () => { } ); + // Note that default trailing slash redirects have already been removed from routes-manifest.json (done in deploy step in real app) jest.mock( "../../src/routes-manifest.json", () => @@ -75,6 +76,7 @@ describe("Lambda@Edge", () => { } ); + // Note that default trailing slash redirects have already been removed from routes-manifest.json (done in deploy step in real app) jest.mock( "../../src/routes-manifest.json", () => require("./default-basepath-routes-manifest.json"), diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts b/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts index 97de192984..d245a5c840 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts @@ -62,6 +62,7 @@ describe("Lambda@Edge", () => { } ); + // Note that default trailing slash redirects have already been removed from routes-manifest.json (done in deploy step in real app) jest.mock( "../../src/routes-manifest.json", () => require("./default-routes-manifest-with-trailing-slash.json"), @@ -78,6 +79,7 @@ describe("Lambda@Edge", () => { } ); + // Note that default trailing slash redirects have already been removed from routes-manifest.json (done in deploy step in real app) jest.mock( "../../src/routes-manifest.json", () => require("./default-routes-manifest.json"), @@ -644,7 +646,7 @@ describe("Lambda@Edge", () => { ${"/old-blog/abc/"} | ${"/news/abc/"} | ${308} ${"/old-users/1234/"} | ${"/users/1234/"} | ${307} `( - "redirects path $path to $expectedRedirect, expectedRedirectStatusCode: expectedRedirectStatusCode", + "redirects path $path to $expectedRedirect, expectedRedirectStatusCode: $expectedRedirectStatusCode", async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { await runRedirectTest( path, diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json index 828654584a..fd63eea0f0 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json @@ -3,18 +3,6 @@ "pages404": true, "basePath": "", "redirects": [ - { - "source": "/:file((?:[^/]+/)*[^/]+\\.\\w+)/", - "destination": "/:file", - "statusCode": 308, - "regex": "^(?:/((?:[^/]+/)*[^/]+\\.\\w+))/$" - }, - { - "source": "/:notfile((?:[^/]+/)*[^/\\.]+)", - "destination": "/:notfile/", - "statusCode": 308, - "regex": "^(?:/((?:[^/]+/)*[^/\\.]+))$" - }, { "source": "/old-blog/:slug/", "destination": "/news/:slug/", diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json index 96fcb4411c..1a2a83cb47 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json @@ -3,12 +3,6 @@ "pages404": true, "basePath": "", "redirects": [ - { - "source": "/:path+/", - "destination": "/:path+", - "statusCode": 308, - "regex": "^(?:/((?:[^/]+?)(?:/(?:[^/]+?))*))/$" - }, { "source": "/old-blog/:slug", "destination": "/news/:slug", From 2e6672116f6a5662b34ce1d6ec0de6b331f12823 Mon Sep 17 00:00:00 2001 From: Daniel Phang Date: Sun, 27 Sep 2020 19:49:35 -0700 Subject: [PATCH 4/7] update redirector test --- .../libs/lambda-at-edge/tests/routing/redirector.test.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts b/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts index abe64b7d6d..9091153079 100644 --- a/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts +++ b/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts @@ -12,12 +12,6 @@ describe("Redirector Tests", () => { routesManifest = { basePath: "", redirects: [ - { - source: "/:path+/", - destination: "/:path+", - statusCode: 308, - regex: "^(?:/((?:[^/]+?)(?:/(?:[^/]+?))*))/$" - }, { source: "/old-blog/:slug", destination: "/news/:slug", From f85352592e587d8d84820c59d027aa253245b98d Mon Sep 17 00:00:00 2001 From: Daniel Phang Date: Sun, 27 Sep 2020 20:12:03 -0700 Subject: [PATCH 5/7] update integration test to verify default redirects are removed --- .../with-empty-next-config.test.ts | 11 ++++-- .../next.config.js | 1 + .../pages/index.js | 1 + .../next.config.js | 5 +++ .../pages/index.js | 1 + .../with-trailing-slash-next-config.test.ts | 37 +++++++++++++++---- 6 files changed, 45 insertions(+), 11 deletions(-) create mode 100644 packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath-no-trailing-slash/next.config.js create mode 100644 packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath-no-trailing-slash/pages/index.js create mode 100644 packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/next.config.js create mode 100644 packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/pages/index.js diff --git a/packages/libs/lambda-at-edge/tests/integration/with-empty-next-config/with-empty-next-config.test.ts b/packages/libs/lambda-at-edge/tests/integration/with-empty-next-config/with-empty-next-config.test.ts index 99adb07eb2..64eef9c9fe 100644 --- a/packages/libs/lambda-at-edge/tests/integration/with-empty-next-config/with-empty-next-config.test.ts +++ b/packages/libs/lambda-at-edge/tests/integration/with-empty-next-config/with-empty-next-config.test.ts @@ -1,8 +1,9 @@ import { getNextBinary, removeNewLineChars } from "../../test-utils"; import os from "os"; -import path from "path"; -import Builder from "../../../src/build"; -import { readFile, remove, pathExists } from "fs-extra"; +import path, { join } from "path"; +import Builder, { DEFAULT_LAMBDA_CODE_DIR } from "../../../src/build"; +import fse, { readFile, remove, pathExists } from "fs-extra"; +import { RoutesManifest } from "types"; jest.unmock("execa"); @@ -44,4 +45,8 @@ describe("With Empty Next Config Build", () => { await pathExists(path.join(fixtureDir, "next.config.original.123.js")) ).toBe(false); }); + + it(`default redirects in routes-manifest.json are removed`, () => { + expect(routesManifest.redirects).toEqual([]); + }); }); diff --git a/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath-no-trailing-slash/next.config.js b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath-no-trailing-slash/next.config.js new file mode 100644 index 0000000000..3feee74e58 --- /dev/null +++ b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath-no-trailing-slash/next.config.js @@ -0,0 +1 @@ +module.exports = { basePath: "/basepath", target: "serverless" }; diff --git a/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath-no-trailing-slash/pages/index.js b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath-no-trailing-slash/pages/index.js new file mode 100644 index 0000000000..c63cf14923 --- /dev/null +++ b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath-no-trailing-slash/pages/index.js @@ -0,0 +1 @@ +export default () => "Hello World!"; diff --git a/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/next.config.js b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/next.config.js new file mode 100644 index 0000000000..efd72ce8b1 --- /dev/null +++ b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/next.config.js @@ -0,0 +1,5 @@ +module.exports = { + basePath: "/basepath", + target: "serverless", + trailingSlash: true +}; diff --git a/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/pages/index.js b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/pages/index.js new file mode 100644 index 0000000000..c63cf14923 --- /dev/null +++ b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/pages/index.js @@ -0,0 +1 @@ +export default () => "Hello World!"; diff --git a/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/with-trailing-slash-next-config.test.ts b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/with-trailing-slash-next-config.test.ts index 847b9c5ec0..2cec95f95b 100644 --- a/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/with-trailing-slash-next-config.test.ts +++ b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/with-trailing-slash-next-config.test.ts @@ -3,7 +3,10 @@ import os from "os"; import path, { join } from "path"; import Builder, { DEFAULT_LAMBDA_CODE_DIR } from "../../../src/build"; import fse, { readFile, remove, pathExists } from "fs-extra"; -import { OriginRequestDefaultHandlerManifest } from "../../../types"; +import { + OriginRequestDefaultHandlerManifest, + RoutesManifest +} from "../../../types"; jest.unmock("execa"); @@ -12,17 +15,25 @@ describe("With Trailing Slash Config Build", () => { let outputDir: string; let mockDateNow: jest.SpyInstance; let defaultBuildManifest: OriginRequestDefaultHandlerManifest; + let routesManifest: RoutesManifest; describe.each` - fixture | expectedTrailingSlash | expectedOriginalNextConfig - ${"fixture-next-config-as-obj"} | ${true} | ${'module.exports = { target: "serverless", trailingSlash: true };'} - ${"fixture-next-config-as-func"} | ${true} | ${'module.exports = () => ({ target: "serverless", trailingSlash: true });'} - ${"fixture-next-config-as-obj-no-trailing-slash"} | ${false} | ${'module.exports = { target: "serverless" };'} - ${"fixture-next-config-as-func-no-trailing-slash"} | ${false} | ${'module.exports = () => ({ target: "serverless" });'} - ${"fixture-no-next-config"} | ${false} | ${undefined} + fixture | expectedTrailingSlash | expectedBasePath | expectedOriginalNextConfig + ${"fixture-next-config-as-obj"} | ${true} | ${""} | ${'module.exports = { target: "serverless", trailingSlash: true };'} + ${"fixture-next-config-as-func"} | ${true} | ${""} | ${'module.exports = () => ({ target: "serverless", trailingSlash: true });'} + ${"fixture-next-config-as-obj-no-trailing-slash"} | ${false} | ${""} | ${'module.exports = { target: "serverless" };'} + ${"fixture-next-config-as-func-no-trailing-slash"} | ${false} | ${""} | ${'module.exports = () => ({ target: "serverless" });'} + ${"fixture-no-next-config"} | ${false} | ${""} | ${undefined} + ${"fixture-next-config-as-obj-basepath"} | ${true} | ${"/basepath"} | ${'module.exports = { basePath: "/basepath", target: "serverless", trailingSlash: true };'} + ${"fixture-next-config-as-obj-basepath-no-trailing-slash"} | ${false} | ${"/basepath"} | ${'module.exports = { basePath: "/basepath", target: "serverless" };'} `( "with fixture: $fixture", - ({ fixture, expectedTrailingSlash, expectedOriginalNextConfig }) => { + ({ + fixture, + expectedTrailingSlash, + expectedBasePath, + expectedOriginalNextConfig + }) => { const fixtureDir = path.join(__dirname, `./${fixture}`); beforeAll(async () => { @@ -39,6 +50,10 @@ describe("With Trailing Slash Config Build", () => { defaultBuildManifest = await fse.readJSON( join(outputDir, `${DEFAULT_LAMBDA_CODE_DIR}/manifest.json`) ); + + routesManifest = await fse.readJSON( + join(outputDir, `${DEFAULT_LAMBDA_CODE_DIR}/routes-manifest.json`) + ); }); afterAll(() => { @@ -73,6 +88,12 @@ describe("With Trailing Slash Config Build", () => { const { trailingSlash } = defaultBuildManifest; expect(trailingSlash).toBe(expectedTrailingSlash); }); + + it(`default trailing slash redirects in routes-manifest.json are removed`, () => { + // Default trailing slash redirects are removed since they are handled in non-regex solution + expect(routesManifest.redirects).toEqual([]); + expect(routesManifest.basePath).toEqual(expectedBasePath); + }); } ); }); From 88ccacd131eb8f467b0b1ba62e4d27e69930b696 Mon Sep 17 00:00:00 2001 From: Daniel Phang Date: Sun, 27 Sep 2020 20:15:24 -0700 Subject: [PATCH 6/7] revert with-empty-next-config.test changes --- .../with-empty-next-config.test.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/libs/lambda-at-edge/tests/integration/with-empty-next-config/with-empty-next-config.test.ts b/packages/libs/lambda-at-edge/tests/integration/with-empty-next-config/with-empty-next-config.test.ts index 64eef9c9fe..99adb07eb2 100644 --- a/packages/libs/lambda-at-edge/tests/integration/with-empty-next-config/with-empty-next-config.test.ts +++ b/packages/libs/lambda-at-edge/tests/integration/with-empty-next-config/with-empty-next-config.test.ts @@ -1,9 +1,8 @@ import { getNextBinary, removeNewLineChars } from "../../test-utils"; import os from "os"; -import path, { join } from "path"; -import Builder, { DEFAULT_LAMBDA_CODE_DIR } from "../../../src/build"; -import fse, { readFile, remove, pathExists } from "fs-extra"; -import { RoutesManifest } from "types"; +import path from "path"; +import Builder from "../../../src/build"; +import { readFile, remove, pathExists } from "fs-extra"; jest.unmock("execa"); @@ -45,8 +44,4 @@ describe("With Empty Next Config Build", () => { await pathExists(path.join(fixtureDir, "next.config.original.123.js")) ).toBe(false); }); - - it(`default redirects in routes-manifest.json are removed`, () => { - expect(routesManifest.redirects).toEqual([]); - }); }); From 8aaf6b378b203b3e37cb5cc2677ae78195d42f3b Mon Sep 17 00:00:00 2001 From: Daniel Phang Date: Sun, 27 Sep 2020 20:24:26 -0700 Subject: [PATCH 7/7] fix integ test failure --- .../fixture-next-config-as-obj-basepath/next.config.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/next.config.js b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/next.config.js index efd72ce8b1..2cce0601d6 100644 --- a/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/next.config.js +++ b/packages/libs/lambda-at-edge/tests/integration/with-trailing-slash-next-config/fixture-next-config-as-obj-basepath/next.config.js @@ -1,5 +1 @@ -module.exports = { - basePath: "/basepath", - target: "serverless", - trailingSlash: true -}; +module.exports = { basePath: "/basepath", target: "serverless", trailingSlash: true };