fix(remix): Resolve Remix Request API compatibility issues. (#6215)

onurtemizkan · web-flow · commit 0ee35f070927 · 2022-11-16T17:53:52.000-08:00
Ref: #6139 and #6139 (comment) This PR is not a complete solution for all issues mentioned in #6139, but it aims to solve the parsing issue, which should fix auto-instrumented usage. Remix uses its own implementation of Fetch API [1] on both back-end and front-end, which has different structures storing `headers` and `url`. That has caused `RequestData` integration to not work properly on Remix projects. I first attempted adding support to the core parser, and `PolymorphicRequest`. But it seemed very complex (if possible), because we have to type Proxy objects and Symbols, which our current TypeScript version doesn't fully support. So, I vendored / modified a couple of unexported utilities from `@remix-run/web-fetch` [2], to convert request objects to a compatible structure that we can consume in `RequestData` integration. [1] https://github.com/remix-run/web-std-io/tree/main/packages/fetch [2] https://www.npmjs.com/package/@remix-run/web-fetch
diff --git a/packages/remix/src/performance/client.tsx b/packages/remix/src/performance/client.tsx
@@ -1,4 +1,4 @@
-import { ErrorBoundaryProps, WINDOW , withErrorBoundary } from '@sentry/react';
+import { ErrorBoundaryProps, WINDOW, withErrorBoundary } from '@sentry/react';
 import { Transaction, TransactionContext } from '@sentry/types';
 import { logger } from '@sentry/utils';
 import * as React from 'react';
diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts
@@ -21,12 +21,14 @@ import {
   DataFunctionArgs,
   HandleDocumentRequestFunction,
   ReactRouterDomPkg,
+  RemixRequest,
   RequestHandler,
   RouteMatch,
   ServerBuild,
   ServerRoute,
   ServerRouteManifest,
 } from './types';
+import { normalizeRemixRequest } from './web-fetch';
 
 // Flag to track if the core request handler is instrumented.
 export let isRequestHandlerWrapped = false;
@@ -91,8 +93,10 @@ async function captureRemixServerException(err: Error, name: string, request: Re
     return;
   }
 
+  const normalizedRequest = normalizeRemixRequest(request as unknown as any);
+
   captureException(isResponse(err) ? await extractResponseError(err) : err, scope => {
-    scope.setSDKProcessingMetadata({ request });
+    scope.setSDKProcessingMetadata({ request: normalizedRequest });
     scope.addEventProcessor(event => {
       addExceptionMechanism(event, {
         type: 'instrument',
@@ -252,6 +256,7 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction {
     // Note: `redirect` and `catch` responses do not have bodies to extract
     if (isResponse(res) && !isRedirectResponse(res) && !isCatchResponse(res)) {
       const data = await extractData(res);
+
       if (typeof data === 'object') {
         return json(
           { ...data, ...traceAndBaggage },
@@ -363,15 +368,20 @@ export function getTransactionName(
 function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler {
   const routes = createRoutes(build.routes);
   const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');
-  return async function (this: unknown, request: Request, loadContext?: unknown): Promise<Response> {
+
+  return async function (this: unknown, request: RemixRequest, loadContext?: unknown): Promise<Response> {
     const local = domain.create();
     return local.bind(async () => {
       const hub = getCurrentHub();
       const options = hub.getClient()?.getOptions();
       const scope = hub.getScope();
 
+      const normalizedRequest = normalizeRemixRequest(request);
+
       if (scope) {
-        scope.setSDKProcessingMetadata({ request });
+        scope.setSDKProcessingMetadata({
+          request: normalizedRequest,
+        });
       }
 
       if (!options || !hasTracingEnabled(options)) {
diff --git a/packages/remix/src/utils/types.ts b/packages/remix/src/utils/types.ts
@@ -3,11 +3,26 @@
 // Types vendored from @remix-run/server-runtime@1.6.0:
 // https://github.com/remix-run/remix/blob/f3691d51027b93caa3fd2cdfe146d7b62a6eb8f2/packages/remix-server-runtime/server.ts
 import type * as Express from 'express';
+import { Agent } from 'https';
 import type { ComponentType } from 'react';
 
+export type RemixRequestState = {
+  method: string;
+  redirect: RequestRedirect;
+  headers: Headers;
+  parsedURL: URL;
+  signal: AbortSignal | null;
+  size: number | null;
+};
+
+export type RemixRequest = Request &
+  Record<symbol | string, RemixRequestState> & {
+    agent: Agent | ((parsedURL: URL) => Agent) | undefined;
+  };
+
 export type AppLoadContext = any;
 export type AppData = any;
-export type RequestHandler = (request: Request, loadContext?: AppLoadContext) => Promise<Response>;
+export type RequestHandler = (request: RemixRequest, loadContext?: AppLoadContext) => Promise<Response>;
 export type CreateRequestHandlerFunction = (this: unknown, build: ServerBuild, ...args: any[]) => RequestHandler;
 export type ServerRouteManifest = RouteManifest<Omit<ServerRoute, 'children'>>;
 export type Params<Key extends string = string> = {
@@ -104,7 +119,7 @@ export interface ServerBuild {
 }
 
 export interface HandleDocumentRequestFunction {
-  (request: Request, responseStatusCode: number, responseHeaders: Headers, context: EntryContext):
+  (request: RemixRequest, responseStatusCode: number, responseHeaders: Headers, context: EntryContext):
     | Promise<Response>
     | Response;
 }
@@ -119,7 +134,7 @@ interface ServerEntryModule {
 }
 
 export interface DataFunctionArgs {
-  request: Request;
+  request: RemixRequest;
   context: AppLoadContext;
   params: Params;
 }
diff --git a/packages/remix/src/utils/web-fetch.ts b/packages/remix/src/utils/web-fetch.ts
@@ -0,0 +1,122 @@
+// Based on Remix's implementation of Fetch API
+// https://github.com/remix-run/web-std-io/tree/main/packages/fetch
+
+import { RemixRequest } from './types';
+
+/*
+ * Symbol extractor utility to be able to access internal fields of Remix requests.
+ */
+const getInternalSymbols = (
+  request: Record<string, unknown>,
+): {
+  bodyInternalsSymbol: string;
+  requestInternalsSymbol: string;
+} => {
+  const symbols = Object.getOwnPropertySymbols(request);
+  return {
+    bodyInternalsSymbol: symbols.find(symbol => symbol.toString().includes('Body internals')) as any,
+    requestInternalsSymbol: symbols.find(symbol => symbol.toString().includes('Request internals')) as any,
+  };
+};
+
+/**
+ * Vendored from:
+ * https://github.com/remix-run/web-std-io/blob/f715b354c8c5b8edc550c5442dec5712705e25e7/packages/fetch/src/utils/get-search.js#L5
+ */
+export const getSearch = (parsedURL: URL): string => {
+  if (parsedURL.search) {
+    return parsedURL.search;
+  }
+
+  const lastOffset = parsedURL.href.length - 1;
+  const hash = parsedURL.hash || (parsedURL.href[lastOffset] === '#' ? '#' : '');
+  return parsedURL.href[lastOffset - hash.length] === '?' ? '?' : '';
+};
+
+/**
+ * Convert a Request to Node.js http request options.
+ * The options object to be passed to http.request
+ * Vendored / modified from:
+ * https://github.com/remix-run/web-std-io/blob/f715b354c8c5b8edc550c5442dec5712705e25e7/packages/fetch/src/request.js#L259
+ */
+export const normalizeRemixRequest = (request: RemixRequest): Record<string, any> => {
+  const { requestInternalsSymbol, bodyInternalsSymbol } = getInternalSymbols(request);
+
+  if (!requestInternalsSymbol) {
+    throw new Error('Could not find request internals symbol');
+  }
+
+  const { parsedURL } = request[requestInternalsSymbol];
+  const headers = new Headers(request[requestInternalsSymbol].headers);
+
+  // Fetch step 1.3
+  if (!headers.has('Accept')) {
+    headers.set('Accept', '*/*');
+  }
+
+  // HTTP-network-or-cache fetch steps 2.4-2.7
+  let contentLengthValue = null;
+  if (request.body === null && /^(post|put)$/i.test(request.method)) {
+    contentLengthValue = '0';
+  }
+
+  if (request.body !== null) {
+    const totalBytes = request[bodyInternalsSymbol].size;
+    // Set Content-Length if totalBytes is a number (that is not NaN)
+    if (typeof totalBytes === 'number' && !Number.isNaN(totalBytes)) {
+      contentLengthValue = String(totalBytes);
+    }
+  }
+
+  if (contentLengthValue) {
+    headers.set('Content-Length', contentLengthValue);
+  }
+
+  // HTTP-network-or-cache fetch step 2.11
+  if (!headers.has('User-Agent')) {
+    headers.set('User-Agent', 'node-fetch');
+  }
+
+  // HTTP-network-or-cache fetch step 2.15
+  if (request.compress && !headers.has('Accept-Encoding')) {
+    headers.set('Accept-Encoding', 'gzip,deflate,br');
+  }
+
+  let { agent } = request;
+
+  if (typeof agent === 'function') {
+    agent = agent(parsedURL);
+  }
+
+  if (!headers.has('Connection') && !agent) {
+    headers.set('Connection', 'close');
+  }
+
+  // HTTP-network fetch step 4.2
+  // chunked encoding is handled by Node.js
+  const search = getSearch(parsedURL);
+
+  // Manually spread the URL object instead of spread syntax
+  const requestOptions = {
+    path: parsedURL.pathname + search,
+    pathname: parsedURL.pathname,
+    hostname: parsedURL.hostname,
+    protocol: parsedURL.protocol,
+    port: parsedURL.port,
+    hash: parsedURL.hash,
+    search: parsedURL.search,
+    // @ts-ignore - it does not has a query
+    query: parsedURL.query,
+    href: parsedURL.href,
+    method: request.method,
+    // @ts-ignore - not sure what this supposed to do
+    headers: headers[Symbol.for('nodejs.util.inspect.custom')](),
+    insecureHTTPParser: request.insecureHTTPParser,
+    agent,
+
+    // [SENTRY] For compatibility with Sentry SDK RequestData parser, adding `originalUrl` property.
+    originalUrl: parsedURL.href,
+  };
+
+  return requestOptions;
+};
diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts
@@ -33,6 +33,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
       request: {
         method: 'POST',
         url,
+        cookies: expect.any(Object),
+        headers: {
+          'user-agent': expect.any(String),
+          host: 'localhost:8000',
+        },
       },
     });
   });
@@ -101,6 +106,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
       request: {
         method: 'POST',
         url,
+        cookies: expect.any(Object),
+        headers: {
+          'user-agent': expect.any(String),
+          host: 'localhost:8000',
+        },
       },
     });
 
@@ -116,6 +126,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
       request: {
         method: 'POST',
         url,
+        cookies: expect.any(Object),
+        headers: {
+          'user-agent': expect.any(String),
+          host: 'localhost:8000',
+        },
       },
     });
   });

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-import { ErrorBoundaryProps, WINDOW , withErrorBoundary } from '@sentry/react';`
	`1`	`+import { ErrorBoundaryProps, WINDOW, withErrorBoundary } from '@sentry/react';`
`2`	`2`	`import { Transaction, TransactionContext } from '@sentry/types';`
`3`	`3`	`import { logger } from '@sentry/utils';`
`4`	`4`	`import * as React from 'react';`