Skip to content

Commit

Permalink
Rename url to path for onRequestError request arg (#68672)
Browse files Browse the repository at this point in the history
### What

* Rename the `request.url` in onRequestError callback arg to
`request.path`
* Align `routeType: "middleware"` with others, only contain the resource
path in the `request.path`, stripped out request origin.

### Why

For non middleware case, the `req.url` in next-server is actual resource
path, which only contains the pathname and search but without request
origin. For middleware case, the origin is included in the `req.url`.
This might be a implementation detail, but for users, having request
origin here might not be critical and super helpful since they will know
the deployment as a global context for error tracking.

Hence we align the property to a variable that not holding request
origin, given a better naming `request.path`. The idea is from `:path`
request header, which represents the resource path, only containing
pathname and search. e.g. `/a/b?c=2`

x-ref:
https://greenbytes.de/tech/webdav/draft-ietf-httpbis-http2-09.html#HttpRequest
Closes NDX-54
  • Loading branch information
huozhi authored Aug 12, 2024
1 parent 90ccf2e commit 11fa91b
Show file tree
Hide file tree
Showing 7 changed files with 13 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ The function accepts three parameters: `error`, `request`, and `context`.
export function onRequestError(
error: { digest: string } & Error,
request: {
url: string // full URL
path: string // resource path, e.g. /blog?name=foo
method: string // request method. e.g. GET, POST, etc
headers: { [key: string]: string }
},
Expand Down
4 changes: 3 additions & 1 deletion packages/next/src/build/templates/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@ function errorHandledHandler(fn: AdapterOptions['handler']) {
return await fn(...args)
} catch (err) {
const req = args[0]
const url = new URL(req.url)
const resource = url.pathname + url.search
await edgeInstrumentationOnRequestError(
err,
{
url: req.url,
path: resource,
method: req.method,
headers: Object.fromEntries(req.headers.entries()),
},
Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ export default abstract class Server<
await this.instrumentation.onRequestError?.(
err,
{
url: req.url || '',
path: req.url || '',
method: req.method || 'GET',
// Normalize middleware headers and other server request headers
headers:
Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/server/instrumentation/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ export type RequestErrorContext = {
export type InstrumentationOnRequestError = (
error: unknown,
errorRequest: Readonly<{
path: string
method: string
url: string
headers: NodeJS.Dict<string | string[]>
}>,
errorContext: Readonly<RequestErrorContext>
Expand Down
11 changes: 1 addition & 10 deletions test/e2e/on-request-error/basic/basic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,10 @@ describe('on-request-error - basic', () => {
errorMessage,
url,
renderSource,
isMiddleware = false,
}: {
errorMessage: string
url: string
renderSource: string | undefined
isMiddleware?: boolean
}) {
// Assert the instrumentation is called
await retry(async () => {
Expand All @@ -40,14 +38,8 @@ describe('on-request-error - basic', () => {

const { payload } = record
const { request } = payload
if (isMiddleware) {
// For middleware, the URL is absolute url with host
expect(request.url).toMatch(/^http:\/\//)
expect(request.url).toMatch(url)
} else {
expect(request.url).toBe(url)
}

expect(request.path).toBe(url)
expect(record).toMatchObject({
count: 1,
payload: {
Expand Down Expand Up @@ -164,7 +156,6 @@ describe('on-request-error - basic', () => {
await validateErrorRecord({
errorMessage: 'middleware-error',
url: '/middleware-error',
isMiddleware: true,
renderSource: undefined,
})
})
Expand Down
10 changes: 5 additions & 5 deletions test/e2e/on-request-error/dynamic-routes/dynamic-routes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ describe('on-request-error - dynamic-routes', () => {
payload: {
message: 'server-dynamic-page-node-error',
request: {
url: '/app-page/dynamic/123?apple=dope',
path: '/app-page/dynamic/123?apple=dope',
},
context: {
routerKind: 'App Router',
Expand All @@ -65,7 +65,7 @@ describe('on-request-error - dynamic-routes', () => {
payload: {
message: 'server-dynamic-route-node-error',
request: {
url: '/app-route/dynamic/123?apple=dope',
path: '/app-route/dynamic/123?apple=dope',
},
context: {
routerKind: 'App Router',
Expand All @@ -86,7 +86,7 @@ describe('on-request-error - dynamic-routes', () => {
payload: {
message: 'server-suspense-page-node-error',
request: {
url: '/app-page/suspense',
path: '/app-page/suspense',
},
context: {
routerKind: 'App Router',
Expand All @@ -109,7 +109,7 @@ describe('on-request-error - dynamic-routes', () => {
payload: {
message: 'pages-page-node-error',
request: {
url: '/pages-page/dynamic/123?apple=dope',
path: '/pages-page/dynamic/123?apple=dope',
},
context: {
routerKind: 'Pages Router',
Expand All @@ -130,7 +130,7 @@ describe('on-request-error - dynamic-routes', () => {
payload: {
message: 'pages-api-node-error',
request: {
url: '/api/dynamic/123?apple=dope',
path: '/api/dynamic/123?apple=dope',
},
context: {
routerKind: 'Pages Router',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('on-request-error - server-action-error', () => {
payload: {
message: errorMessage,
request: {
url,
path: url,
method: 'POST',
headers: expect.objectContaining({
accept: 'text/x-component',
Expand Down

0 comments on commit 11fa91b

Please sign in to comment.