From 09dc8bd682d10f1928252a6cc2133d599955753f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 3 Nov 2022 10:39:03 +0000 Subject: [PATCH 1/7] Add failing tests --- .../nextjs/test/integration/next.config.js | 4 ++ .../test/integration/next10.config.template | 4 ++ .../test/integration/next11.config.template | 4 ++ .../excludedEndpoints/excludedWithRegExp.tsx | 6 ++ .../excludedEndpoints/excludedWithString.tsx | 6 ++ .../test/server/excludedApiEndpoints.js | 67 +++++++++++++++++++ 6 files changed, 91 insertions(+) create mode 100644 packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithRegExp.tsx create mode 100644 packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithString.tsx create mode 100644 packages/nextjs/test/integration/test/server/excludedApiEndpoints.js diff --git a/packages/nextjs/test/integration/next.config.js b/packages/nextjs/test/integration/next.config.js index 8992ed63d0e5..384d7b51ce51 100644 --- a/packages/nextjs/test/integration/next.config.js +++ b/packages/nextjs/test/integration/next.config.js @@ -9,6 +9,10 @@ const moduleExports = { // Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts` // TODO (v8): This can come out in v8, because this option will get a default value hideSourceMaps: false, + excludedServersideEntrypoints: [ + 'pages/api/excludedEndpoints/excludedWithString', + /pages\/api\/excludedEndpoints\/excludedWithRegExp/, + ], }, }; const SentryWebpackPluginOptions = { diff --git a/packages/nextjs/test/integration/next10.config.template b/packages/nextjs/test/integration/next10.config.template index 31c332cd25cd..1d91bfb42c0f 100644 --- a/packages/nextjs/test/integration/next10.config.template +++ b/packages/nextjs/test/integration/next10.config.template @@ -10,6 +10,10 @@ const moduleExports = { // Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts` // TODO (v8): This can come out in v8, because this option will get a default value hideSourceMaps: false, + excludedServersideEntrypoints: [ + 'pages/api/excludedEndpoints/excludedWithString', + /pages\/api\/excludedEndpoints\/excludedWithRegExp/, + ], }, }; diff --git a/packages/nextjs/test/integration/next11.config.template b/packages/nextjs/test/integration/next11.config.template index 6a7e849067b1..03a65530813a 100644 --- a/packages/nextjs/test/integration/next11.config.template +++ b/packages/nextjs/test/integration/next11.config.template @@ -11,6 +11,10 @@ const moduleExports = { // Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts` // TODO (v8): This can come out in v8, because this option will get a default value hideSourceMaps: false, + excludedServersideEntrypoints: [ + 'pages/api/excludedEndpoints/excludedWithString', + /pages\/api\/excludedEndpoints\/excludedWithRegExp/, + ], }, }; diff --git a/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithRegExp.tsx b/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithRegExp.tsx new file mode 100644 index 000000000000..05bc0178f20f --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithRegExp.tsx @@ -0,0 +1,6 @@ +// This file will test the `excludedServersideEntrypoints` option when a route is provided as a RegExp. +const handler = async (): Promise => { + throw new Error('API Error'); +}; + +export default handler; diff --git a/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithString.tsx b/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithString.tsx new file mode 100644 index 000000000000..faecef304955 --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithString.tsx @@ -0,0 +1,6 @@ +// This file will test the `excludedServersideEntrypoints` option when a route is provided as a string. +const handler = async (): Promise => { + throw new Error('API Error'); +}; + +export default handler; diff --git a/packages/nextjs/test/integration/test/server/excludedApiEndpoints.js b/packages/nextjs/test/integration/test/server/excludedApiEndpoints.js new file mode 100644 index 000000000000..db49bbbc57cf --- /dev/null +++ b/packages/nextjs/test/integration/test/server/excludedApiEndpoints.js @@ -0,0 +1,67 @@ +const assert = require('assert'); + +const { sleep } = require('../utils/common'); +const { getAsync, interceptEventRequest, interceptTracingRequest } = require('../utils/server'); + +module.exports = async ({ url: urlBase, argv }) => { + const regExpUrl = `${urlBase}/api/excludedEndpoints/excludedWithRegExp`; + const stringUrl = `${urlBase}/api/excludedEndpoints/excludedWithString`; + + const capturedRegExpErrorRequest = interceptEventRequest( + { + exception: { + values: [ + { + type: 'Error', + value: 'API Error', + }, + ], + }, + tags: { + runtime: 'node', + }, + request: { + url: regExpUrl, + method: 'GET', + }, + transaction: 'GET /api/excludedEndpoints/excludedWithRegExp', + }, + argv, + 'excluded API endpoint via RegExp', + ); + + const capturedStringErrorRequest = interceptEventRequest( + { + exception: { + values: [ + { + type: 'Error', + value: 'API Error', + }, + ], + }, + tags: { + runtime: 'node', + }, + request: { + url: regExpUrl, + method: 'GET', + }, + transaction: 'GET /api/excludedEndpoints/excludedWithString', + }, + argv, + 'excluded API endpoint via String', + ); + + await Promise.all([getAsync(regExpUrl), getAsync(stringUrl)]); + await sleep(250); + + assert.ok( + !capturedRegExpErrorRequest.isDone(), + 'Did intercept error request even though route should be excluded (RegExp)', + ); + assert.ok( + !capturedStringErrorRequest.isDone(), + 'Did intercept error request even though route should be excluded (String)', + ); +}; From dbf772802d1903e93190613afb843a3fa276a002 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 3 Nov 2022 10:55:22 +0000 Subject: [PATCH 2/7] Add `excludedServersideEntrypoints` option --- .../nextjs/src/config/loaders/proxyLoader.ts | 20 +++++++++-- packages/nextjs/src/config/types.ts | 9 +++++ packages/nextjs/src/config/webpack.ts | 36 ++++++++++++++----- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/proxyLoader.ts index 323294a38504..061836575311 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/proxyLoader.ts @@ -8,6 +8,7 @@ import { LoaderThis } from './types'; type LoaderOptions = { pagesDir: string; pageExtensionRegex: string; + excludedServersideEntrypoints: (RegExp | string)[]; }; /** @@ -17,7 +18,8 @@ type LoaderOptions = { */ export default async function proxyLoader(this: LoaderThis, userCode: string): Promise { // We know one or the other will be defined, depending on the version of webpack being used - const { pagesDir, pageExtensionRegex } = 'getOptions' in this ? this.getOptions() : this.query; + const { pagesDir, pageExtensionRegex, excludedServersideEntrypoints } = + 'getOptions' in this ? this.getOptions() : this.query; // Get the parameterized route name from this page's filepath const parameterizedRoute = path @@ -34,11 +36,25 @@ export default async function proxyLoader(this: LoaderThis, userC // homepage), sub back in the root route .replace(/^$/, '/'); + // For the `excludedServersideEntrypoints` option we need the calculate the relative path to the file in question without file extension. + const relativePagePath = path + .join('path', path.relative(pagesDir, this.resourcePath)) + // Pull off the file extension + .replace(new RegExp(`\\.(${pageExtensionRegex})`), ''); + + const isExcluded = excludedServersideEntrypoints.some(exludeEntry => { + if (typeof exludeEntry === 'string') { + return relativePagePath === exludeEntry; + } else { + return relativePagePath.match(exludeEntry); + } + }); + // We don't want to wrap twice (or infinitely), so in the proxy we add this query string onto references to the // wrapped file, so that we know that it's already been processed. (Adding this query string is also necessary to // convince webpack that it's a different file than the one it's in the middle of loading now, so that the originals // themselves will have a chance to load.) - if (this.resourceQuery.includes('__sentry_wrapped__')) { + if (isExcluded || this.resourceQuery.includes('__sentry_wrapped__')) { return userCode; } diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts index 6324b35fa36e..ff79f7466c46 100644 --- a/packages/nextjs/src/config/types.ts +++ b/packages/nextjs/src/config/types.ts @@ -59,6 +59,15 @@ export type UserSentryOptions = { // Automatically instrument Next.js data fetching methods and Next.js API routes autoInstrumentServerFunctions?: boolean; + + // Used to exclude certain serverside API routes or pages from being instrumented with Sentry. This option takes an + // array of strings or regular expressions - strings will exactly match a route. Matches are made against routes in + // the follwoing form: + // - "pages/home/index" + // - "pages/about" + // - "pages/posts/[postId]" + // - "pages/posts/[postId]/comments" + excludedServersideEntrypoints?: (RegExp | string)[]; }; export type NextConfigFunction = (phase: string, defaults: { defaultConfig: NextConfigObject }) => NextConfigObject; diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 659a1689273a..a694a1b117fc 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -91,7 +91,11 @@ export function constructWebpackConfigFunction( use: [ { loader: path.resolve(__dirname, 'loaders/proxyLoader.js'), - options: { pagesDir, pageExtensionRegex }, + options: { + pagesDir, + pageExtensionRegex, + excludedServersideEntrypoints: userSentryOptions.excludedServersideEntrypoints ?? [], + }, }, ], }); @@ -135,7 +139,7 @@ export function constructWebpackConfigFunction( // will call the callback which will call `f` which will call `x.y`... and on and on. Theoretically this could also // be fixed by using `bind`, but this is way simpler.) const origEntryProperty = newConfig.entry; - newConfig.entry = async () => addSentryToEntryProperty(origEntryProperty, buildContext); + newConfig.entry = async () => addSentryToEntryProperty(origEntryProperty, buildContext, userSentryOptions); // Enable the Sentry plugin (which uploads source maps to Sentry when not in dev) by default if (shouldEnableWebpackPlugin(buildContext, userSentryOptions)) { @@ -248,6 +252,7 @@ function findTranspilationRules(rules: WebpackModuleRule[] | undefined, projectD async function addSentryToEntryProperty( currentEntryProperty: WebpackEntryProperty, buildContext: BuildContext, + userSentryOptions: UserSentryOptions, ): Promise { // The `entry` entry in a webpack config can be a string, array of strings, object, or function. By default, nextjs // sets it to an async function which returns the promise of an object of string arrays. Because we don't know whether @@ -268,7 +273,7 @@ async function addSentryToEntryProperty( // inject into all entry points which might contain user's code for (const entryPointName in newEntryProperty) { - if (shouldAddSentryToEntryPoint(entryPointName, isServer)) { + if (shouldAddSentryToEntryPoint(entryPointName, isServer, userSentryOptions.excludedServersideEntrypoints ?? [])) { addFilesToExistingEntryPoint(newEntryProperty, entryPointName, filesToInject); } } @@ -377,13 +382,27 @@ function checkWebpackPluginOverrides( * * @param entryPointName The name of the entry point in question * @param isServer Whether or not this function is being called in the context of a server build + * @param excludedServersideEntrypoints A list of excluded serverside entrypoints * @returns `true` if sentry code should be injected, and `false` otherwise */ -function shouldAddSentryToEntryPoint(entryPointName: string, isServer: boolean): boolean { +function shouldAddSentryToEntryPoint( + entryPointName: string, + isServer: boolean, + excludedServersideEntrypoints: (string | RegExp)[], +): boolean { + const isServerSideExcluded = excludedServersideEntrypoints.some(serverSideExclude => { + if (typeof serverSideExclude === 'string') { + return entryPointName === serverSideExclude; + } else { + return entryPointName.match(serverSideExclude); + } + }); + return ( - entryPointName === 'pages/_app' || - (entryPointName.includes('pages/api') && !entryPointName.includes('_middleware')) || - (isServer && entryPointName === 'pages/_error') + (!isServer || !isServerSideExcluded) && + (entryPointName === 'pages/_app' || + (entryPointName.includes('pages/api') && !entryPointName.includes('_middleware')) || + (isServer && entryPointName === 'pages/_error')) ); } @@ -436,7 +455,8 @@ export function getWebpackPluginOptions( configFile: hasSentryProperties ? 'sentry.properties' : undefined, stripPrefix: ['webpack://_N_E/'], urlPrefix, - entries: (entryPointName: string) => shouldAddSentryToEntryPoint(entryPointName, isServer), + entries: (entryPointName: string) => + shouldAddSentryToEntryPoint(entryPointName, isServer, userSentryOptions.excludedServersideEntrypoints ?? []), release: getSentryRelease(buildId), dryRun: isDev, }); From 480198b4caec04cc10cfb5abe62e777b803e315a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 3 Nov 2022 12:17:00 +0000 Subject: [PATCH 3/7] Typo --- packages/nextjs/src/config/loaders/proxyLoader.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/proxyLoader.ts index 061836575311..d848d28ef844 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/proxyLoader.ts @@ -38,7 +38,7 @@ export default async function proxyLoader(this: LoaderThis, userC // For the `excludedServersideEntrypoints` option we need the calculate the relative path to the file in question without file extension. const relativePagePath = path - .join('path', path.relative(pagesDir, this.resourcePath)) + .join('pages', path.relative(pagesDir, this.resourcePath)) // Pull off the file extension .replace(new RegExp(`\\.(${pageExtensionRegex})`), ''); From 509387f1a23e4e2558c2c21912d6a6f19fa62095 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 3 Nov 2022 13:51:56 +0100 Subject: [PATCH 4/7] Update packages/nextjs/src/config/types.ts Co-authored-by: Abhijeet Prasad --- packages/nextjs/src/config/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts index ff79f7466c46..87528aa45407 100644 --- a/packages/nextjs/src/config/types.ts +++ b/packages/nextjs/src/config/types.ts @@ -62,7 +62,7 @@ export type UserSentryOptions = { // Used to exclude certain serverside API routes or pages from being instrumented with Sentry. This option takes an // array of strings or regular expressions - strings will exactly match a route. Matches are made against routes in - // the follwoing form: + // the following form: // - "pages/home/index" // - "pages/about" // - "pages/posts/[postId]" From a03a09ebf3742c05016d2ca0a6b6e2e1d9d6dd47 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 3 Nov 2022 13:46:07 +0000 Subject: [PATCH 5/7] Convert path to posix before matching --- packages/nextjs/src/config/loaders/proxyLoader.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/proxyLoader.ts index d848d28ef844..56eabd8056e9 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/proxyLoader.ts @@ -37,16 +37,19 @@ export default async function proxyLoader(this: LoaderThis, userC .replace(/^$/, '/'); // For the `excludedServersideEntrypoints` option we need the calculate the relative path to the file in question without file extension. - const relativePagePath = path + const relativePosixPagePath = path .join('pages', path.relative(pagesDir, this.resourcePath)) + // Make sure that path is in posix style - this should make it easiser for users to configure and copy & paste from docs + .split(path.sep) + .join(path.posix.sep) // Pull off the file extension .replace(new RegExp(`\\.(${pageExtensionRegex})`), ''); const isExcluded = excludedServersideEntrypoints.some(exludeEntry => { if (typeof exludeEntry === 'string') { - return relativePagePath === exludeEntry; + return relativePosixPagePath === exludeEntry; } else { - return relativePagePath.match(exludeEntry); + return relativePosixPagePath.match(exludeEntry); } }); From 63b713c1bc1669203ea3577e465fc6f7e1c73e82 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 4 Nov 2022 15:35:19 +0000 Subject: [PATCH 6/7] Clean up include/exclude logic --- packages/nextjs/src/config/webpack.ts | 35 ++++++++++++++++++--------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index a694a1b117fc..9790f99cb754 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -390,20 +390,31 @@ function shouldAddSentryToEntryPoint( isServer: boolean, excludedServersideEntrypoints: (string | RegExp)[], ): boolean { - const isServerSideExcluded = excludedServersideEntrypoints.some(serverSideExclude => { - if (typeof serverSideExclude === 'string') { - return entryPointName === serverSideExclude; - } else { - return entryPointName.match(serverSideExclude); + if (isServer) { + const isExcluded = excludedServersideEntrypoints.some(serverSideExclude => { + if (typeof serverSideExclude === 'string') { + return entryPointName === serverSideExclude; + } else { + return entryPointName.match(serverSideExclude); + } + }); + + if (isExcluded) { + return false; + } else if (entryPointName === 'pages/_error') { + return true; } - }); + } - return ( - (!isServer || !isServerSideExcluded) && - (entryPointName === 'pages/_app' || - (entryPointName.includes('pages/api') && !entryPointName.includes('_middleware')) || - (isServer && entryPointName === 'pages/_error')) - ); + if (entryPointName === 'pages/_app') { + return true; + } + + if (entryPointName.includes('pages/api') && !entryPointName.includes('_middleware')) { + return true; + } + + return false; } /** From 21d70c444bbf56a5de62734824849d1add8b9564 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 4 Nov 2022 15:36:57 +0000 Subject: [PATCH 7/7] Make option optional --- packages/nextjs/src/config/webpack.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 9790f99cb754..359e05a7af16 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -94,7 +94,7 @@ export function constructWebpackConfigFunction( options: { pagesDir, pageExtensionRegex, - excludedServersideEntrypoints: userSentryOptions.excludedServersideEntrypoints ?? [], + excludedServersideEntrypoints: userSentryOptions.excludedServersideEntrypoints, }, }, ], @@ -388,7 +388,7 @@ function checkWebpackPluginOverrides( function shouldAddSentryToEntryPoint( entryPointName: string, isServer: boolean, - excludedServersideEntrypoints: (string | RegExp)[], + excludedServersideEntrypoints: (string | RegExp)[] = [], ): boolean { if (isServer) { const isExcluded = excludedServersideEntrypoints.some(serverSideExclude => { @@ -467,7 +467,7 @@ export function getWebpackPluginOptions( stripPrefix: ['webpack://_N_E/'], urlPrefix, entries: (entryPointName: string) => - shouldAddSentryToEntryPoint(entryPointName, isServer, userSentryOptions.excludedServersideEntrypoints ?? []), + shouldAddSentryToEntryPoint(entryPointName, isServer, userSentryOptions.excludedServersideEntrypoints), release: getSentryRelease(buildId), dryRun: isDev, });