Skip to content

Commit

Permalink
Fix failing E2E deployment test cases (#36368)
Browse files Browse the repository at this point in the history
This continues off of #36285 fixing some of the failing test cases noticed when running the E2E tests against deployments. After these are resolved the tests will be added to our CI flow after each canary release. 

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

x-ref: #36285
  • Loading branch information
ijjk committed Apr 22, 2022
1 parent b4d4f24 commit 7c6052a
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 15 deletions.
Expand Up @@ -30,7 +30,8 @@ export function getApiHandler(ctx: ServerlessHandlerCtx) {
// We need to trust the dynamic route params from the proxy
// to ensure we are using the correct values
const trustQuery = req.headers[vercelHeader]
const parsedUrl = handleRewrites(req, parseUrl(req.url!, true))
const parsedUrl = parseUrl(req.url!, true)
handleRewrites(req, parsedUrl)

if (parsedUrl.query.nextInternalLocale) {
delete parsedUrl.query.nextInternalLocale
Expand Down
Expand Up @@ -139,7 +139,7 @@ export function getPageHandler(ctx: ServerlessHandlerCtx) {
}
const origQuery = Object.assign({}, parsedUrl.query)

parsedUrl = handleRewrites(req, parsedUrl)
handleRewrites(req, parsedUrl)
handleBasePath(req, parsedUrl)

// remove ?amp=1 from request URL if rendering for export
Expand Down
Expand Up @@ -91,6 +91,8 @@ export function getUtils({
req: BaseNextRequest | IncomingMessage,
parsedUrl: UrlWithParsedQuery
) {
const rewriteParams = {}

for (const rewrite of rewrites) {
const matcher = getCustomRouteMatcher(rewrite.source)
let params = matcher(parsedUrl.pathname)
Expand All @@ -106,13 +108,14 @@ export function getUtils({
}

if (params) {
const { parsedDestination } = prepareDestination({
const { parsedDestination, destQuery } = prepareDestination({
appendParamsToQuery: true,
destination: rewrite.destination,
params: params,
query: parsedUrl.query,
})

Object.assign(rewriteParams, destQuery, params)
Object.assign(parsedUrl.query, parsedDestination.query)
delete (parsedDestination as any).query

Expand Down Expand Up @@ -152,7 +155,7 @@ export function getUtils({
}
}

return parsedUrl
return rewriteParams
}

function handleBasePath(
Expand Down Expand Up @@ -285,15 +288,16 @@ export function getUtils({

function normalizeVercelUrl(
req: BaseNextRequest | IncomingMessage,
trustQuery: boolean
trustQuery: boolean,
paramKeys?: string[]
) {
// make sure to normalize req.url on Vercel to strip dynamic params
// from the query which are added during routing
if (pageIsDynamic && trustQuery && defaultRouteRegex) {
const _parsedUrl = parseUrl(req.url!, true)
delete (_parsedUrl as any).search

for (const param of Object.keys(defaultRouteRegex.groups)) {
for (const param of paramKeys || Object.keys(defaultRouteRegex.groups)) {
delete _parsedUrl.query[param]
}
req.url = formatUrl(_parsedUrl)
Expand Down
41 changes: 34 additions & 7 deletions packages/next/server/base-server.ts
Expand Up @@ -440,21 +440,26 @@ export default abstract class Server {
typeof req.headers['x-matched-path'] === 'string'
) {
const reqUrlIsDataUrl = req.url?.includes('/_next/data')
const parsedMatchedPath = parseUrl(req.headers['x-matched-path'] || '')
const matchedPathIsDataUrl =
req.headers['x-matched-path']?.includes('/_next/data')
parsedMatchedPath.pathname?.includes('/_next/data')
const isDataUrl = reqUrlIsDataUrl || matchedPathIsDataUrl

let parsedPath = parseUrl(
isDataUrl ? req.url! : (req.headers['x-matched-path'] as string),
true
)

let matchedPathname = parsedPath.pathname!

let matchedPathnameNoExt = isDataUrl
? matchedPathname.replace(/\.json$/, '')
: matchedPathname

let srcPathname = isDataUrl
? parsedMatchedPath.pathname?.replace(/\.json$/, '') ||
matchedPathnameNoExt
: matchedPathnameNoExt

if (this.nextConfig.i18n) {
const localePathResult = normalizeLocalePath(
matchedPathname || '/',
Expand All @@ -469,9 +474,10 @@ export default abstract class Server {
if (isDataUrl) {
matchedPathname = denormalizePagePath(matchedPathname)
matchedPathnameNoExt = denormalizePagePath(matchedPathnameNoExt)
srcPathname = denormalizePagePath(srcPathname)
}

const pageIsDynamic = isDynamicRoute(matchedPathnameNoExt)
const pageIsDynamic = isDynamicRoute(srcPathname)
const combinedRewrites: Rewrite[] = []

combinedRewrites.push(...this.customRoutes.rewrites.beforeFiles)
Expand All @@ -480,7 +486,7 @@ export default abstract class Server {

const utils = getUtils({
pageIsDynamic,
page: matchedPathnameNoExt,
page: srcPathname,
i18n: this.nextConfig.i18n,
basePath: this.nextConfig.basePath,
rewrites: combinedRewrites,
Expand All @@ -492,7 +498,15 @@ export default abstract class Server {
if (this.nextConfig.i18n && !url.locale?.path.detectedLocale) {
parsedUrl.pathname = `/${url.locale?.locale}${parsedUrl.pathname}`
}
utils.handleRewrites(req, parsedUrl)
const pathnameBeforeRewrite = parsedUrl.pathname
const rewriteParams = utils.handleRewrites(req, parsedUrl)
const rewriteParamKeys = Object.keys(rewriteParams)
const didRewrite = pathnameBeforeRewrite !== parsedUrl.pathname

if (didRewrite) {
addRequestMeta(req, '_nextRewroteUrl', parsedUrl.pathname!)
addRequestMeta(req, '_nextDidRewrite', true)
}

// interpolate dynamic params and normalize URL if needed
if (pageIsDynamic) {
Expand Down Expand Up @@ -538,9 +552,14 @@ export default abstract class Server {
pathname: matchedPathname,
})
}

Object.assign(parsedUrl.query, params)
utils.normalizeVercelUrl(req, true)
}

if (pageIsDynamic || didRewrite) {
utils.normalizeVercelUrl(req, true, [
...rewriteParamKeys,
...Object.keys(utils.defaultRouteRegex?.groups || {}),
])
}
} catch (err) {
if (err instanceof DecodeError) {
Expand Down Expand Up @@ -1368,6 +1387,14 @@ export default abstract class Server {
isRedirect = renderResult.renderOpts.isRedirect
} else {
const origQuery = parseUrl(req.url || '', true).query

// clear any dynamic route params so they aren't in
// the resolvedUrl
if (opts.params) {
Object.keys(opts.params).forEach((key) => {
delete origQuery[key]
})
}
const hadTrailingSlash =
urlPathname !== '/' && this.nextConfig.trailingSlash

Expand Down
Expand Up @@ -217,6 +217,7 @@ export function prepareDestination(args: {

return {
newUrl,
destQuery,
parsedDestination,
}
}
Expand Down
7 changes: 6 additions & 1 deletion test/e2e/prerender.test.ts
Expand Up @@ -844,7 +844,12 @@ describe('Prerender', () => {

it('should 404 for an invalid data url', async () => {
const res = await fetchViaHTTP(next.url, `/_next/data/${next.buildId}`)
expect(res.status).toBe(404)

// when deployed this will match due to `index.json` matching the
// directory itself
if (!isDeploy) {
expect(res.status).toBe(404)
}
})

it('should allow rewriting to SSG page with fallback: false', async () => {
Expand Down
42 changes: 42 additions & 0 deletions test/production/required-server-files.test.ts
Expand Up @@ -54,6 +54,10 @@ describe('should set-up next', () => {
source: '/some-catch-all/:path*',
destination: '/',
},
{
source: '/to-dynamic/post-2',
destination: '/dynamic/post-2?hello=world',
},
{
source: '/to-dynamic/:path',
destination: '/dynamic/:path',
Expand Down Expand Up @@ -667,6 +671,44 @@ describe('should set-up next', () => {
expect(await res.text()).toContain('Bad Request')
})

it('should have correct resolvedUrl from rewrite', async () => {
const res = await fetchViaHTTP(appPort, '/to-dynamic/post-1', undefined, {
headers: {
'x-matched-path': '/dynamic/[slug]',
},
})
expect(res.status).toBe(200)
const $ = cheerio.load(await res.text())
expect($('#resolved-url').text()).toBe('/dynamic/post-1')
})

it('should have correct resolvedUrl from rewrite with added query', async () => {
const res = await fetchViaHTTP(appPort, '/to-dynamic/post-2', undefined, {
headers: {
'x-matched-path': '/dynamic/[slug]',
},
})
expect(res.status).toBe(200)
const $ = cheerio.load(await res.text())
expect($('#resolved-url').text()).toBe('/dynamic/post-2')
})

it('should have correct resolvedUrl from dynamic route', async () => {
const res = await fetchViaHTTP(
appPort,
`/_next/data/${next.buildId}/dynamic/post-2.json`,
{ slug: 'post-2' },
{
headers: {
'x-matched-path': '/dynamic/[slug]',
},
}
)
expect(res.status).toBe(200)
const json = await res.json()
expect(json.pageProps.resolvedUrl).toBe('/dynamic/post-2')
})

it('should bubble error correctly for gip page', async () => {
errors = []
const res = await fetchViaHTTP(appPort, '/errors/gip', { crash: '1' })
Expand Down
@@ -1,8 +1,9 @@
import { useRouter } from 'next/router'

export const getServerSideProps = ({ params }) => {
export const getServerSideProps = ({ params, resolvedUrl }) => {
return {
props: {
resolvedUrl,
hello: 'world',
slug: params.slug,
random: Math.random(),
Expand All @@ -16,6 +17,7 @@ export default function Page(props) {
<>
<p id="dynamic">dynamic page</p>
<p id="slug">{props.slug}</p>
<p id="resolved-url">{props.resolvedUrl}</p>
<p id="router">{JSON.stringify(router)}</p>
<p id="props">{JSON.stringify(props)}</p>
</>
Expand Down

0 comments on commit 7c6052a

Please sign in to comment.