From 9dce6f445ff2ec4d5770a124fda008a9235ba65e Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Fri, 15 Nov 2024 15:54:55 +0100 Subject: [PATCH 1/3] Make non-owned props of express wrapped functions accessible --- library/sources/Express.test.ts | 99 +++++++++++++++++++ library/sources/express/wrapRequestHandler.ts | 51 +++++++--- 2 files changed, 137 insertions(+), 13 deletions(-) diff --git a/library/sources/Express.test.ts b/library/sources/Express.test.ts index fdb6351d..29319e26 100644 --- a/library/sources/Express.test.ts +++ b/library/sources/Express.test.ts @@ -124,6 +124,20 @@ function getApp(userMiddleware = true) { // A middleware that is used as a route app.use("/api/*path", apiMiddleware); + const newRouter = express.Router(); + newRouter.get("/nested-router", (req, res) => { + res.send(getContext()); + }); + + app.use(newRouter); + + const nestedApp = express(); + nestedApp.get("/", (req, res) => { + res.send(getContext()); + }); + + app.use("/nested-app", nestedApp); + app.get("/", (req, res) => { const context = getContext(); @@ -554,3 +568,88 @@ t.test("it preserves original function name in Layer object", async () => { 1 ); }); + +t.test("it supports nested router", async () => { + const response = await request(getApp()).get("/nested-router"); + + t.match(response.body, { + method: "GET", + source: "express", + route: "/nested-router", + }); +}); + +t.test("it supports nested app", async (t) => { + const response = await request(getApp()).get("/nested-app"); + + t.match(response.body, { + method: "GET", + source: "express", + route: "/nested-app", + }); +}); + +// Express instrumentation results in routes with no stack, crashing Ghost +// https://github.com/open-telemetry/opentelemetry-js-contrib/issues/2271 +// https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2294 +t.test( + "it keeps handle properties even if router is patched before instrumentation does it", + async () => { + const { createServer } = require("http") as typeof import("http"); + const expressApp = express(); + const router = express.Router(); + + let routerLayer: { name: string; handle: { stack: any[] } } | undefined = + undefined; + + const CustomRouter: (...p: Parameters) => void = ( + req, + res, + next + ) => router(req, res, next); + + router.use("/:slug", (req, res, next) => { + // On express v4, the router is available as `app._router` + // On express v5, the router is available as `app.router` + // @ts-expect-error stack is private + const stack = req.app.router.stack as any[]; + routerLayer = stack.find((router) => router.name === "CustomRouter"); + return res.status(200).send("bar"); + }); + + // The patched router now has express router's own properties in its prototype so + // they are not accessible through `Object.keys(...)` + // https://github.com/TryGhost/Ghost/blob/fefb9ec395df8695d06442b6ecd3130dae374d94/ghost/core/core/frontend/web/site.js#L192 + Object.setPrototypeOf(CustomRouter, router); + expressApp.use(CustomRouter); + + const server = createServer(expressApp); + await new Promise((resolve) => { + server.listen(0, resolve); + }); + + if (!server) { + throw new Error("server not found"); + } + + const address = server.address(); + + if (typeof address === "string") { + throw new Error("address is a string"); + } + + const response = await fetch(`http://localhost:${address!.port}/foo`); + t.same(await response.text(), "bar"); + server.close(); + + if (!routerLayer) { + throw new Error("router layer not found"); + } + + t.ok( + // @ts-expect-error handle is private + routerLayer.handle.stack.length === 1, + "router layer stack is accessible" + ); + } +); diff --git a/library/sources/express/wrapRequestHandler.ts b/library/sources/express/wrapRequestHandler.ts index fc0fa4ef..7811dbb5 100644 --- a/library/sources/express/wrapRequestHandler.ts +++ b/library/sources/express/wrapRequestHandler.ts @@ -1,29 +1,54 @@ +/* eslint-disable prefer-rest-params */ import type { RequestHandler } from "express"; import { runWithContext } from "../../agent/Context"; +import { createWrappedFunction } from "../../helpers/wrap"; import { contextFromRequest } from "./contextFromRequest"; export function wrapRequestHandler(handler: RequestHandler): RequestHandler { - const fn: RequestHandler = (req, res, next) => { - const context = contextFromRequest(req); + const fn = createWrappedFunction(handler, function wrap(handler) { + return function wrap(this: RequestHandler) { + if (arguments.length === 0) { + return handler.apply(this); + } - return runWithContext(context, () => { - return handler(req, res, next); - }); - }; + const context = contextFromRequest(arguments[0]); + + return runWithContext(context, () => { + return handler.apply(this, arguments); + }); + }; + }) as RequestHandler; + + // Some libraries/apps have properties on the handler functions that are not copied by our createWrappedFunction function + // (createWrappedFunction only copies properties when hasOwnProperty is true) + // Let's set up a proxy to forward the property access to the original handler + // e.g. https://github.com/TryGhost/Ghost/blob/fefb9ec395df8695d06442b6ecd3130dae374d94/ghost/core/core/frontend/web/site.js#L192 + for (const key in handler) { + if (handler.hasOwnProperty(key)) { + continue; + } - if (handler.name) { - preserveFunctionName(fn, handler.name); + Object.defineProperty(fn, key, { + get() { + // @ts-expect-error Types unknown + return handler[key]; + }, + set(value) { + // @ts-expect-error Types unknown + handler[key] = value; + }, + }); } + // For some libraries/apps it's important to preserve the function name + // e.g. Ghost looks up a middleware function by name in the router stack + preserveLayerName(fn, handler.name); + return fn; } /** - * Preserve the original function name - * e.g. Ghost looks up a middleware function by name in the router stack - * * Object.getOwnPropertyDescriptor(function myFunction() {}, "name") - * * { * value: 'myFunction', * writable: false, @@ -31,7 +56,7 @@ export function wrapRequestHandler(handler: RequestHandler): RequestHandler { * configurable: true * } */ -function preserveFunctionName(wrappedFunction: Function, originalName: string) { +function preserveLayerName(wrappedFunction: Function, originalName: string) { try { Object.defineProperty(wrappedFunction, "name", { value: originalName, From 26667fa372c04aa9ee9b9ce243f6a931fe9165ff Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Fri, 15 Nov 2024 16:00:16 +0100 Subject: [PATCH 2/3] Add comment why we use createServer --- library/sources/Express.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/library/sources/Express.test.ts b/library/sources/Express.test.ts index 29319e26..8f415c11 100644 --- a/library/sources/Express.test.ts +++ b/library/sources/Express.test.ts @@ -623,6 +623,7 @@ t.test( Object.setPrototypeOf(CustomRouter, router); expressApp.use(CustomRouter); + // supertest acts weird with the custom router, so we need to create a server manually const server = createServer(expressApp); await new Promise((resolve) => { server.listen(0, resolve); From b6370d20a488dd6b2a9838f1b70536deff20d626 Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Mon, 18 Nov 2024 09:54:36 +0100 Subject: [PATCH 3/3] Use fetch helper function instead of native otherwise we cannot run the test on v16 supertest acts weird with the custom router, so we need to create a server manually --- library/helpers/fetch.ts | 12 ++++++------ library/sources/Express.test.ts | 7 +++++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/library/helpers/fetch.ts b/library/helpers/fetch.ts index 1c76c9d9..f0b642f7 100644 --- a/library/helpers/fetch.ts +++ b/library/helpers/fetch.ts @@ -52,16 +52,16 @@ async function request({ export async function fetch({ url, - method, - headers, + method = "GET", + headers = {}, body = "", - timeoutInMS, + timeoutInMS = 5000, }: { url: URL; - method: string; - headers: Record; + method?: string; + headers?: Record; body?: string; - timeoutInMS: number; + timeoutInMS?: number; }): Promise<{ body: string; statusCode: number }> { const abort = new AbortController(); diff --git a/library/sources/Express.test.ts b/library/sources/Express.test.ts index 8f415c11..d6ffcc12 100644 --- a/library/sources/Express.test.ts +++ b/library/sources/Express.test.ts @@ -5,6 +5,7 @@ import { Express } from "./Express"; import { FileSystem } from "../sinks/FileSystem"; import { HTTPServer } from "./HTTPServer"; import { createTestAgent } from "../helpers/createTestAgent"; +import { fetch } from "../helpers/fetch"; // Before require("express") const agent = createTestAgent({ @@ -639,8 +640,10 @@ t.test( throw new Error("address is a string"); } - const response = await fetch(`http://localhost:${address!.port}/foo`); - t.same(await response.text(), "bar"); + const response = await fetch({ + url: new URL(`http://localhost:${address!.port}/foo`), + }); + t.same(response.body, "bar"); server.close(); if (!routerLayer) {