Skip to content

Commit

Permalink
feat(nextjs): Auto-wrap API routes (#5778)
Browse files Browse the repository at this point in the history
As part of #5505, this applies to API route handlers the same kind of auto-wrapping we've done with the data fetchers (`withServerSideProps` and the like). Though the general idea is the same, the one extra complicating factor here is that there's a good chance the handlers get to us already wrapped in `withSentry`, which we've up until now been telling users to use as a manual wrapper. This is handled by making `withSentry` idempotent - if it detects that it's already been run on the current request, it simply acts as a pass-through to the function it wraps.

Notes:

- A new template has been created to act as a proxy module for API routes, but the proxying work itself is done by the same `proxyLoader` as before - it just loads one template or the other depending on an individual page's path.

- Doing this auto-wrapping gives us a chance to do one thing manual `withSentry` wrapping isn't able to do, which is set the [route config](https://nextjs.org/docs/api-routes/request-helpers) to use an external resolver, which will prevent next's dev server from throwing warnings about API routes not sending responses. (In other words, it should solve #3852.)
  • Loading branch information
lobsterkatie committed Sep 26, 2022
1 parent 036e2a0 commit 1d8370e
Show file tree
Hide file tree
Showing 16 changed files with 260 additions and 20 deletions.
6 changes: 5 additions & 1 deletion packages/nextjs/rollup.npm.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ export default [
),
...makeNPMConfigVariants(
makeBaseNPMConfig({
entrypoints: ['src/config/templates/prefixLoaderTemplate.ts', 'src/config/templates/proxyLoaderTemplate.ts'],
entrypoints: [
'src/config/templates/prefixLoaderTemplate.ts',
'src/config/templates/pageProxyLoaderTemplate.ts',
'src/config/templates/apiProxyLoaderTemplate.ts',
],

packageSpecificConfig: {
plugins: [plugins.makeRemoveMultiLineCommentsPlugin()],
Expand Down
11 changes: 4 additions & 7 deletions packages/nextjs/src/config/loaders/proxyLoader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,6 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userC
// homepage), sub back in the root route
.replace(/^$/, '/');

// TODO: For the moment we skip API routes. Those will need to be handled slightly differently because of the manual
// wrapping we've already been having people do using `withSentry`.
if (parameterizedRoute.startsWith('api')) {
return userCode;
}

// 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
Expand All @@ -47,7 +41,10 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userC
return userCode;
}

const templatePath = path.resolve(__dirname, '../templates/proxyLoaderTemplate.js');
const templateFile = parameterizedRoute.startsWith('/api')
? 'apiProxyLoaderTemplate.js'
: 'pageProxyLoaderTemplate.js';
const templatePath = path.resolve(__dirname, `../templates/${templateFile}`);
let templateCode = fs.readFileSync(templatePath).toString();
// Make sure the template is included when runing `webpack watch`
this.addDependency(templatePath);
Expand Down
47 changes: 47 additions & 0 deletions packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* This file is a template for the code which will be substituted when our webpack loader handles API files in the
* `pages/` directory.
*
* We use `__RESOURCE_PATH__` as a placeholder for the path to the file being wrapped. Because it's not a real package,
* this causes both TS and ESLint to complain, hence the pragma comments below.
*/

// @ts-ignore See above
// eslint-disable-next-line import/no-unresolved
import * as origModule from '__RESOURCE_PATH__';
import * as Sentry from '@sentry/nextjs';
import type { PageConfig } from 'next';

// We import this from `withSentry` rather than directly from `next` because our version can work simultaneously with
// multiple versions of next. See note in `withSentry` for more.
import type { NextApiHandler } from '../../utils/withSentry';

type NextApiModule = {
default: NextApiHandler;
config?: PageConfig;
};

const userApiModule = origModule as NextApiModule;

const maybeWrappedHandler = userApiModule.default;
const origConfig = userApiModule.config || {};

// Setting `externalResolver` to `true` prevents nextjs from throwing a warning in dev about API routes resolving
// without sending a response. It's a false positive (a response is sent, but only after we flush our send queue), and
// we throw a warning of our own to tell folks that, but it's better if we just don't have to deal with it in the first
// place.
export const config = {
...origConfig,
api: {
...origConfig.api,
externalResolver: true,
},
};

export default Sentry.withSentryAPI(maybeWrappedHandler, '__ROUTE__');

// Re-export anything exported by the page module we're wrapping. When processing this code, Rollup is smart enough to
// not include anything whose name matchs something we've explicitly exported above.
// @ts-ignore See above
// eslint-disable-next-line import/no-unresolved
export * from '__RESOURCE_PATH__';
1 change: 1 addition & 0 deletions packages/nextjs/src/config/wrappers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ export { withSentryServerSideAppGetInitialProps } from './withSentryServerSideAp
export { withSentryServerSideDocumentGetInitialProps } from './withSentryServerSideDocumentGetInitialProps';
export { withSentryServerSideErrorGetInitialProps } from './withSentryServerSideErrorGetInitialProps';
export { withSentryGetServerSideProps } from './withSentryGetServerSideProps';
export { withSentryAPI } from './withSentryAPI';
39 changes: 39 additions & 0 deletions packages/nextjs/src/config/wrappers/withSentryAPI.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { formatAsCode, nextLogger } from '../../utils/nextLogger';
// We import these types from `withSentry` rather than directly from `next` because our version can work simultaneously
// with multiple versions of next. See note in `withSentry` for more.
import type { NextApiHandler, WrappedNextApiHandler } from '../../utils/withSentry';
import { withSentry } from '../../utils/withSentry';

/**
* Wrap the given API route handler for tracing and error capturing. Thin wrapper around `withSentry`, which only
* applies it if it hasn't already been applied.
*
* @param maybeWrappedHandler The handler exported from the user's API page route file, which may or may not already be
* wrapped with `withSentry`
* @param parameterizedRoute The page's route, passed in via the proxy loader
* @returns The wrapped handler
*/
export function withSentryAPI(
maybeWrappedHandler: NextApiHandler | WrappedNextApiHandler,
parameterizedRoute: string,
): WrappedNextApiHandler {
// Log a warning if the user is still manually wrapping their route in `withSentry`. Doesn't work in cases where
// there's been an intermediate wrapper (like `withSentryAPI(someOtherWrapper(withSentry(handler)))`) but should catch
// most cases. Only runs once per route. (Note: Such double-wrapping isn't harmful, but we'll eventually deprecate and remove `withSentry`, so
// best to get people to stop using it.)
if (maybeWrappedHandler.name === 'sentryWrappedHandler') {
const [_sentryNextjs_, _autoWrapOption_, _withSentry_, _route_] = [
'@sentry/nextjs',
'autoInstrumentServerFunctions',
'withSentry',
parameterizedRoute,
].map(phrase => formatAsCode(phrase));

nextLogger.info(
`${_sentryNextjs_} is running with the ${_autoWrapOption_} flag set, which means API routes no longer need to ` +
`be manually wrapped with ${_withSentry_}. Detected manual wrapping in ${_route_}.`,
);
}

return withSentry(maybeWrappedHandler, parameterizedRoute);
}
1 change: 1 addition & 0 deletions packages/nextjs/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ export {
withSentryServerSideAppGetInitialProps,
withSentryServerSideDocumentGetInitialProps,
withSentryServerSideErrorGetInitialProps,
withSentryAPI,
} from './config/wrappers';
export { withSentry } from './utils/withSentry';

Expand Down
31 changes: 31 additions & 0 deletions packages/nextjs/src/utils/nextLogger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/* eslint-disable no-console */
import * as chalk from 'chalk';

// This is nextjs's own logging formatting, vendored since it's not exported. See
// https://github.com/vercel/next.js/blob/c3ceeb03abb1b262032bd96457e224497d3bbcef/packages/next/build/output/log.ts#L3-L11
// and
// https://github.com/vercel/next.js/blob/de7aa2d6e486c40b8be95a1327639cbed75a8782/packages/next/lib/eslint/runLintCheck.ts#L321-L323.

const prefixes = {
wait: `${chalk.cyan('wait')} -`,
error: `${chalk.red('error')} -`,
warn: `${chalk.yellow('warn')} -`,
ready: `${chalk.green('ready')} -`,
info: `${chalk.cyan('info')} -`,
event: `${chalk.magenta('event')} -`,
trace: `${chalk.magenta('trace')} -`,
};

export const formatAsCode = (str: string): string => chalk.bold.cyan(str);

export const nextLogger: {
[key: string]: (...message: unknown[]) => void;
} = {
wait: (...message) => console.log(prefixes.wait, ...message),
error: (...message) => console.error(prefixes.error, ...message),
warn: (...message) => console.warn(prefixes.warn, ...message),
ready: (...message) => console.log(prefixes.ready, ...message),
info: (...message) => console.log(prefixes.info, ...message),
event: (...message) => console.log(prefixes.event, ...message),
trace: (...message) => console.log(prefixes.trace, ...message),
};
45 changes: 33 additions & 12 deletions packages/nextjs/src/utils/withSentry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,32 @@ import { NextApiRequest, NextApiResponse } from 'next';
// the test app would refer to the other version of the type (from the test app's `node_modules`). By using a custom
// version of the type compatible with both the old and new official versions, we can use any Next version we want in
// a test app without worrying about type errors.
type NextApiHandler = (req: NextApiRequest, res: NextApiResponse) => void | Promise<void> | unknown | Promise<unknown>;
export type NextApiHandler = (
req: NextApiRequest,
res: NextApiResponse,
) => void | Promise<void> | unknown | Promise<unknown>;
export type WrappedNextApiHandler = (req: NextApiRequest, res: NextApiResponse) => Promise<void> | Promise<unknown>;

type AugmentedNextApiRequest = NextApiRequest & {
__withSentry_applied__?: boolean;
};

export type AugmentedNextApiResponse = NextApiResponse & {
__sentryTransaction?: Transaction;
};

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler => {
export const withSentry = (origHandler: NextApiHandler, parameterizedRoute?: string): WrappedNextApiHandler => {
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
return async (req, res) => {
return async function sentryWrappedHandler(req: AugmentedNextApiRequest, res: NextApiResponse) {
// We're now auto-wrapping API route handlers using `withSentryAPI` (which uses `withSentry` under the hood), but
// users still may have their routes manually wrapped with `withSentry`. This check makes `sentryWrappedHandler`
// idempotent so that those cases don't break anything.
if (req.__withSentry_applied__) {
return origHandler(req, res);
}
req.__withSentry_applied__ = true;

// first order of business: monkeypatch `res.end()` so that it will wait for us to send events to sentry before it
// fires (if we don't do this, the lambda will close too early and events will be either delayed or lost)
// eslint-disable-next-line @typescript-eslint/unbound-method
Expand Down Expand Up @@ -69,17 +84,23 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
const baggageHeader = req.headers && req.headers.baggage;
const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader);

const url = `${req.url}`;
// pull off query string, if any
let reqPath = stripUrlQueryAndFragment(url);
// Replace with placeholder
if (req.query) {
// TODO get this from next if possible, to avoid accidentally replacing non-dynamic parts of the path if
// they happen to match the values of any of the dynamic parts
for (const [key, value] of Object.entries(req.query)) {
reqPath = reqPath.replace(`${value}`, `[${key}]`);
// prefer the parameterized route, if we have it (which we will if we've auto-wrapped the route handler)
let reqPath = parameterizedRoute;

// If not, fake it by just replacing parameter values with their names, hoping that none of them match either
// each other or any hard-coded parts of the path
if (!reqPath) {
const url = `${req.url}`;
// pull off query string, if any
reqPath = stripUrlQueryAndFragment(url);
// Replace with placeholder
if (req.query) {
for (const [key, value] of Object.entries(req.query)) {
reqPath = reqPath.replace(`${value}`, `[${key}]`);
}
}
}

const reqMethod = `${(req.method || 'GET').toUpperCase()} `;

const transaction = startTransaction(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default handler;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default handler;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default handler;
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { withSentry } from '@sentry/nextjs';
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default withSentry(handler);
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { withSentry } from '@sentry/nextjs';
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default withSentry(handler);
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { withSentry } from '@sentry/nextjs';
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default withSentry(handler);
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
const assert = require('assert');

const { sleep } = require('../utils/common');
const { getAsync, interceptTracingRequest } = require('../utils/server');

module.exports = async ({ url: urlBase, argv }) => {
const urls = {
// testName: [url, route]
unwrappedNoParamURL: [`/api/withSentryAPI/unwrapped/noParams`, '/api/withSentryAPI/unwrapped/noParams'],
unwrappedDynamicURL: [`/api/withSentryAPI/unwrapped/dog`, '/api/withSentryAPI/unwrapped/[animal]'],
unwrappedCatchAllURL: [`/api/withSentryAPI/unwrapped/dog/facts`, '/api/withSentryAPI/unwrapped/[...pathParts]'],
wrappedNoParamURL: [`/api/withSentryAPI/wrapped/noParams`, '/api/withSentryAPI/wrapped/noParams'],
wrappedDynamicURL: [`/api/withSentryAPI/wrapped/dog`, '/api/withSentryAPI/wrapped/[animal]'],
wrappedCatchAllURL: [`/api/withSentryAPI/wrapped/dog/facts`, '/api/withSentryAPI/wrapped/[...pathParts]'],
};

const interceptedRequests = {};

Object.entries(urls).forEach(([testName, [url, route]]) => {
interceptedRequests[testName] = interceptTracingRequest(
{
contexts: {
trace: {
op: 'http.server',
status: 'ok',
tags: { 'http.status_code': '200' },
},
},
transaction: `GET ${route}`,
type: 'transaction',
request: {
url: `${urlBase}${url}`,
},
},
argv,
testName,
);
});

// Wait until all requests have completed
await Promise.all(Object.values(urls).map(([url]) => getAsync(`${urlBase}${url}`)));

await sleep(250);

const failingTests = Object.entries(interceptedRequests).reduce(
(failures, [testName, request]) => (!request.isDone() ? failures.concat(testName) : failures),
[],
);

assert.ok(
failingTests.length === 0,
`Did not intercept transaction request for the following tests: ${failingTests.join(', ')}.`,
);
};

0 comments on commit 1d8370e

Please sign in to comment.