Skip to content

Commit

Permalink
Allow to delete URL search params in middleware rewrites (#33725)
Browse files Browse the repository at this point in the history
This allows to have a more streamlined API for rewrites.
Since middlewares are code and not static configurations, we can allow people to
delete query params and not just overwrite them.

**⚠️ Warning ⚠️** this is a breaking change in query parameter behavior with middlewares, but the API will make more sense now. Compare the following:

```diff
  import {NextResponse} from 'next/server'
  export default function middleware({ nextUrl }) {
-   nextUrl.searchParams.set('ignored-query-param', '');
+   nextUrl.searchParams.delete('ignored-query-param'); // 🎉
    return NextResponse.rewrite(nextUrl);
  }
```

Since this is breaking change, we're adding a warning every time we find a query param that is missing, and eventually--the warning will be deleted.
I don't think we need to worry about older versions of Next.js as the current behavior is misleading: merging query parameters make sense for static rewrites, but not for middlewares where you have access to all the query parameters and can manipulate them freely.

Related:
* This is the opposite of #33454
* This supersedes #33574
  • Loading branch information
Schniz committed Jan 27, 2022
1 parent 16b5bfa commit 7b2ea48
Show file tree
Hide file tree
Showing 8 changed files with 162 additions and 12 deletions.
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 () => {
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

0 comments on commit 7b2ea48

Please sign in to comment.