Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to delete URL search params in middleware rewrites #33725

Merged
merged 8 commits into from Jan 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 35 additions & 0 deletions errors/deleting-query-params-in-middlewares.md
@@ -0,0 +1,35 @@
# Deleting Query Parameters In Middlewares

#### Why This Error Occurred

In previous versions of Next.js, we were merging query parameters with the incoming request for rewrites happening in middlewares, to match the behavior of static rewrites declared in the config. This forced Next.js users to use empty query parameters values to delete keys.

We are changing this behavior to allow extra flexibility and a more streamlined experience for users. So from now on, query parameters will not be merged and thus the warning.

```typescript
import type { NextRequest } from 'next/server'
import { NextResponse } from 'next/server'

export default function middleware(request: NextRequest) {
const nextUrl = request.nextUrl
nextUrl.searchParams.delete('key') // this is now possible! 🎉
return NextResponse.rewrite(nextUrl)
}
```

#### Possible Ways to Fix It

If you are relying on the old behavior, please add the query parameters manually to the rewritten URL. Using `request.nextUrl` would do that automatically for you.

```typescript
import type { NextRequest } from 'next/server'
import { NextResponse } from 'next/server'

export default function middleware(request: NextRequest) {
const nextUrl = request.nextUrl
nextUrl.pathname = '/dest'
return NextResponse.rewrite(url)
}
```

This warning will be removed in a next version of Next.js.
4 changes: 4 additions & 0 deletions errors/manifest.json
Expand Up @@ -531,6 +531,10 @@
{
"title": "middleware-relative-urls",
"path": "/errors/middleware-relative-urls.md"
},
{
"title": "deleting-query-params-in-middlewares",
"path": "/errors/deleting-query-params-in-middlewares.md"
}
]
}
Expand Down
36 changes: 33 additions & 3 deletions packages/next/server/next-server.ts
Expand Up @@ -74,6 +74,7 @@ import { getMiddlewareRegex, getRouteMatcher } from '../shared/lib/router/utils'
import { MIDDLEWARE_ROUTE } from '../lib/constants'
import { loadEnvConfig } from '@next/env'
import { getCustomRoute } from './server-route-utils'
import { urlQueryToSearchParams } from '../shared/lib/router/utils/querystring'

export * from './base-server'

Expand Down Expand Up @@ -1005,13 +1006,22 @@ export default class NextNodeServer extends BaseServer {
}

if (result.response.headers.has('x-middleware-rewrite')) {
const rewritePath = result.response.headers.get(
'x-middleware-rewrite'
)!
const { newUrl, parsedDestination } = prepareDestination({
appendParamsToQuery: true,
destination: result.response.headers.get('x-middleware-rewrite')!,
appendParamsToQuery: false,
destination: rewritePath,
params: _params,
query: parsedUrl.query,
query: {},
})

// TODO: remove after next minor version current `v12.0.9`
this.warnIfQueryParametersWereDeleted(
parsedUrl.query,
parsedDestination.query
)

if (
parsedDestination.protocol &&
(parsedDestination.port
Expand Down Expand Up @@ -1192,4 +1202,24 @@ export default class NextNodeServer extends BaseServer {
protected getRoutesManifest() {
return require(join(this.distDir, ROUTES_MANIFEST))
}

// TODO: remove after next minor version current `v12.0.9`
private warnIfQueryParametersWereDeleted(
incoming: ParsedUrlQuery,
rewritten: ParsedUrlQuery
): void {
const incomingQuery = urlQueryToSearchParams(incoming)
const rewrittenQuery = urlQueryToSearchParams(rewritten)

const missingKeys = [...incomingQuery.keys()].filter((key) => {
return !rewrittenQuery.has(key)
})

if (missingKeys.length > 0) {
Log.warn(
`Query params are no longer automatically merged for rewrites in middleware, see more info here: https://nextjs.org/docs/messages/errors/deleting-query-params-in-middlewares`
)
this.warnIfQueryParametersWereDeleted = () => {}
}
}
}
31 changes: 29 additions & 2 deletions packages/next/server/request-meta.ts
Expand Up @@ -53,16 +53,43 @@ export function addRequestMeta<K extends keyof RequestMeta>(
return setRequestMeta(request, meta)
}

export type NextParsedUrlQuery = ParsedUrlQuery & {
type NextQueryMetadata = {
__nextDefaultLocale?: string
__nextFallback?: 'true'
__nextLocale?: string
__nextSsgPath?: string
_nextBubbleNoFallback?: '1'
_nextDataReq?: '1'
amp?: '1'
}

export type NextParsedUrlQuery = ParsedUrlQuery &
NextQueryMetadata & {
amp?: '1'
}

export interface NextUrlWithParsedQuery extends UrlWithParsedQuery {
query: NextParsedUrlQuery
}

export function getNextInternalQuery(
query: NextParsedUrlQuery
): NextQueryMetadata {
const keysToInclude: (keyof NextQueryMetadata)[] = [
'__nextDefaultLocale',
'__nextFallback',
'__nextLocale',
'__nextSsgPath',
'_nextBubbleNoFallback',
'_nextDataReq',
]
const nextInternalQuery: NextQueryMetadata = {}

for (const key of keysToInclude) {
if (key in query) {
// @ts-ignore this can't be typed correctly
nextInternalQuery[key] = query[key]
}
}

return nextInternalQuery
}
4 changes: 2 additions & 2 deletions packages/next/server/router.ts
@@ -1,5 +1,5 @@
import type { ParsedUrlQuery } from 'querystring'
import type { NextUrlWithParsedQuery } from './request-meta'
import { getNextInternalQuery, NextUrlWithParsedQuery } from './request-meta'

import pathMatch from '../shared/lib/router/utils/path-match'
import { removePathTrailingSlash } from '../client/normalize-trailing-slash'
Expand Down Expand Up @@ -400,7 +400,7 @@ export default class Router {

if (result.query) {
parsedUrlUpdated.query = {
...parsedUrlUpdated.query,
...getNextInternalQuery(parsedUrlUpdated.query),
...result.query,
}
}
Expand Down
13 changes: 13 additions & 0 deletions test/integration/middleware/core/pages/rewrites/_middleware.js
@@ -1,5 +1,8 @@
import { NextResponse } from 'next/server'

/**
* @param {import('next/server').NextRequest} request
*/
export async function middleware(request) {
const url = request.nextUrl

Expand Down Expand Up @@ -32,6 +35,16 @@ export async function middleware(request) {
return NextResponse.rewrite('https://vercel.com')
}

if (url.pathname === '/rewrites/clear-query-params') {
const allowedKeys = ['allowed']
for (const key of [...url.searchParams.keys()]) {
if (!allowedKeys.includes(key)) {
url.searchParams.delete(key)
}
}
return NextResponse.rewrite(url)
}

if (url.pathname === '/rewrites/rewrite-me-without-hard-navigation') {
url.searchParams.set('middleware', 'foo')
url.pathname =
Expand Down
@@ -0,0 +1,12 @@
export default function ClearQueryParams(props) {
return <pre id="my-query-params">{JSON.stringify(props.query)}</pre>
}

/** @type {import('next').GetServerSideProps} */
export const getServerSideProps = (req) => {
return {
props: {
query: { ...req.query },
},
}
}
39 changes: 34 additions & 5 deletions test/integration/middleware/core/test/index.test.js
Expand Up @@ -37,8 +37,8 @@ describe('Middleware base tests', () => {
})
})
afterAll(() => killApp(context.app))
rewriteTests()
rewriteTests('/fr')
rewriteTests(log)
rewriteTests(log, '/fr')
redirectTests()
redirectTests('/fr')
responseTests()
Expand Down Expand Up @@ -74,8 +74,8 @@ describe('Middleware base tests', () => {
})
})
afterAll(() => killApp(context.app))
rewriteTests()
rewriteTests('/fr')
rewriteTests(serverOutput)
rewriteTests(serverOutput, '/fr')
redirectTests()
redirectTests('/fr')
responseTests()
Expand Down Expand Up @@ -161,7 +161,7 @@ function urlTests(log, locale = '') {
})
}

function rewriteTests(locale = '') {
function rewriteTests(log, locale = '') {
it('should rewrite to fallback: true page successfully', async () => {
const randomSlug = `another-${Date.now()}`
const res2 = await fetchViaHTTP(
Expand Down Expand Up @@ -212,6 +212,35 @@ function rewriteTests(locale = '') {
expect($('.title').text()).toBe(expectedText)
})

it(`${locale} should clear query parameters`, async () => {
Schniz marked this conversation as resolved.
Show resolved Hide resolved
const res = await fetchViaHTTP(
context.appPort,
`${locale}/rewrites/clear-query-params`,
{
a: '1',
b: '2',
foo: 'bar',
allowed: 'kept',
}
)
const html = await res.text()
const $ = cheerio.load(html)
expect(JSON.parse($('#my-query-params').text())).toEqual({
allowed: 'kept',
})
})

it(`warns about a query param deleted`, async () => {
await fetchViaHTTP(
context.appPort,
`${locale}/rewrites/clear-query-params`,
{ a: '1', allowed: 'kept' }
)
expect(log.output).toContain(
'Query params are no longer automatically merged for rewrites in middleware'
)
})

it(`${locale} should rewrite to about page`, async () => {
const res = await fetchViaHTTP(
context.appPort,
Expand Down