Preserve body on request object passed to willSendRequest; fix `s…

…tring` and `Buffer` body handling (#89)

The v4 update introduced multiple regressions w.r.t. the `modifiedRequest` object that was added.

* `body` was no longer being added to the intermediary request object before calling `willSendRequest`
* `modifiedRequest.body` was never being set in the case that the incoming body was a `string` or `Buffer`

This change resolves both by reverting to what we previously had in v3 (preserving the properties
on the incoming request object). The types had to be updated accordingly. Due to how `path` is
now handled in v4, the `willSendRequest` signature has been updated to  `(path, request)`.

.changeset/cuddly-cars-sparkle.md

@@ -0,0 +1,12 @@
+---
+'@apollo/datasource-rest': major
+---
+
+This change restores the full functionality of `willSendRequest` which
+previously existed in the v3 version of this package. The v4 change introduced a
+regression where the incoming request's `body` was no longer included in the
+object passed to the `willSendRequest` hook, it was always `undefined`.
+
+For consistency and typings reasons, the `path` argument is now the first
+argument to the `willSendRequest` hook, followed by the `AugmentedRequest`
+request object.

.changeset/tidy-spoons-nail.md

@@ -0,0 +1,8 @@
+---
+'@apollo/datasource-rest': patch
+---
+
+`string` and `Buffer` bodies are now correctly included on the outgoing request.
+Due to a regression in v4, they were ignored and never sent as the `body`.
+`string` and `Buffer` bodies are now passed through to the outgoing request
+(without being JSON stringified).

README.md

@@ -105,7 +105,11 @@ By default, `RESTDatasource` uses the full request URL as the cache key. Overrid
 For example, you could use this to use header fields as part of the cache key. Even though we do validate header fields and don't serve responses from cache when they don't match, new responses overwrite old ones with different header fields.
 
 ##### `willSendRequest`
-This method is invoked just before the fetch call is made. If a `Promise` is returned from this method it will wait until the promise is completed to continue executing the request.
+This method is invoked at the beginning of processing each request. It's called
+with the `path` and `request` provided to `fetch`, with a guaranteed non-empty
+`headers` and `params` objects. If a `Promise` is returned from this method it
+will wait until the promise is completed to continue executing the request. See
+the [intercepting fetches](#intercepting-fetches) section for usage examples.
 
 ##### `cacheOptionsFor`
 Allows setting the `CacheOptions` to be used for each request/response in the HTTPCache. This is separate from the request-only cache. You can use this to set the TTL.
@@ -180,7 +184,7 @@ You can easily set a header on every request:
 
 ```javascript
 class PersonalizationAPI extends RESTDataSource {
-  willSendRequest(request) {
+  willSendRequest(path, request) {
     request.headers['authorization'] = this.context.token;
   }
 }
@@ -190,15 +194,15 @@ Or add a query parameter:
 
 ```javascript
 class PersonalizationAPI extends RESTDataSource {
-  willSendRequest(request) {
+  willSendRequest(path, request) {
     request.params.set('api_key', this.context.token);
   }
 }
 ```
 
-If you're using TypeScript, you can use the `RequestOptions` type to define the `willSendRequest` signature:
+If you're using TypeScript, you can use the `AugmentedRequest` type to define the `willSendRequest` signature:
 ```ts
-import { RESTDataSource, WillSendRequestOptions } from '@apollo/datasource-rest';
+import { RESTDataSource, AugmentedRequest } from '@apollo/datasource-rest';
 
 class PersonalizationAPI extends RESTDataSource {
   override baseURL = 'https://personalization-api.example.com/';
@@ -207,7 +211,7 @@ class PersonalizationAPI extends RESTDataSource {
     super();
   }
 
-  override willSendRequest(request: WillSendRequestOptions) {
+  override willSendRequest(_path: string, request: AugmentedRequest) {
     request.headers['authorization'] = this.token;
   }
 }
@@ -223,7 +227,7 @@ class PersonalizationAPI extends RESTDataSource {
     super();
   }
 
-  override async resolveURL(path: string) {
+  override async resolveURL(path: string, _request: AugmentedRequest) {
     if (!this.baseURL) {
       const addresses = await resolveSrv(path.split("/")[1] + ".service.consul");
       this.baseURL = addresses[0];

src/RESTDataSource.ts

@@ -29,13 +29,6 @@ export type RequestOptions = FetcherRequestInit & {
       ) => CacheOptions | undefined);
 };
 
-export type WillSendRequestOptions = Omit<
-  WithRequired<RequestOptions, 'headers'>,
-  'params'
-> & {
-  params: URLSearchParams;
-};
-
 export interface GetRequest extends RequestOptions {
   method?: 'GET';
   body?: never;
@@ -48,6 +41,21 @@ export interface RequestWithBody extends Omit<RequestOptions, 'body'> {
 
 type DataSourceRequest = GetRequest | RequestWithBody;
 
+// While tempting, this union can't be reduced / factored out to just
+// Omit<WithRequired<GetRequest | RequestWithBody, 'headers'>, 'params'> & { params: URLSearchParams }
+// TS loses its ability to discriminate against the method (and its consequential `body` type)
+/**
+ * This type is for convenience w.r.t. the `willSendRequest` and `resolveURL`
+ * hooks to ensure that headers and params are always present, even if they're
+ * empty.
+ */
+export type AugmentedRequest = (
+  | Omit<WithRequired<GetRequest, 'headers'>, 'params'>
+  | Omit<WithRequired<RequestWithBody, 'headers'>, 'params'>
+) & {
+  params: URLSearchParams;
+};
+
 export interface CacheOptions {
   ttl?: number;
 }
@@ -79,12 +87,13 @@ export abstract class RESTDataSource {
   }
 
   protected willSendRequest?(
-    requestOpts: WillSendRequestOptions,
+    path: string,
+    requestOpts: AugmentedRequest,
   ): ValueOrPromise<void>;
 
   protected resolveURL(
     path: string,
-    _request: RequestOptions,
+    _request: AugmentedRequest,
   ): ValueOrPromise<URL> {
     return new URL(path, this.baseURL);
   }
@@ -205,71 +214,75 @@ export abstract class RESTDataSource {
 
   private async fetch<TResult>(
     path: string,
-    request: DataSourceRequest,
+    incomingRequest: DataSourceRequest,
   ): Promise<TResult> {
-    const modifiedRequest: WillSendRequestOptions = {
-      ...request,
+    const augmentedRequest: AugmentedRequest = {
+      ...incomingRequest,
       // guarantee params and headers objects before calling `willSendRequest` for convenience
       params:
-        request.params instanceof URLSearchParams
-          ? request.params
-          : this.urlSearchParamsFromRecord(request.params),
-      headers: request.headers ?? Object.create(null),
-      body: undefined,
+        incomingRequest.params instanceof URLSearchParams
+          ? incomingRequest.params
+          : this.urlSearchParamsFromRecord(incomingRequest.params),
+      headers: incomingRequest.headers ?? Object.create(null),
     };
 
     if (this.willSendRequest) {
-      await this.willSendRequest(modifiedRequest);
+      await this.willSendRequest(path, augmentedRequest);
     }
 
-    const url = await this.resolveURL(path, modifiedRequest);
+    const url = await this.resolveURL(path, augmentedRequest);
 
-    // Append params from the request to any existing params in the path
-    for (const [name, value] of modifiedRequest.params as URLSearchParams) {
+    // Append params to existing params in the path
+    for (const [name, value] of augmentedRequest.params as URLSearchParams) {
       url.searchParams.append(name, value);
     }
 
-    // We accept arbitrary objects and arrays as body and serialize them as JSON
+    // We accept arbitrary objects and arrays as body and serialize them as JSON.
+    // `string`, `Buffer`, and `undefined` are passed through up above as-is.
     if (
-      request.body !== undefined &&
-      request.body !== null &&
-      (request.body.constructor === Object ||
-        Array.isArray(request.body) ||
-        ((request.body as any).toJSON &&
-          typeof (request.body as any).toJSON === 'function'))
+      augmentedRequest.body != null &&
+      !(augmentedRequest.body instanceof Buffer) &&
+      (augmentedRequest.body.constructor === Object ||
+        Array.isArray(augmentedRequest.body) ||
+        ((augmentedRequest.body as any).toJSON &&
+          typeof (augmentedRequest.body as any).toJSON === 'function'))
     ) {
-      modifiedRequest.body = JSON.stringify(request.body);
+      augmentedRequest.body = JSON.stringify(augmentedRequest.body);
       // If Content-Type header has not been previously set, set to application/json
-      if (!modifiedRequest.headers) {
-        modifiedRequest.headers = { 'content-type': 'application/json' };
-      } else if (!modifiedRequest.headers['content-type']) {
-        modifiedRequest.headers['content-type'] = 'application/json';
+      if (!augmentedRequest.headers) {
+        augmentedRequest.headers = { 'content-type': 'application/json' };
+      } else if (!augmentedRequest.headers['content-type']) {
+        augmentedRequest.headers['content-type'] = 'application/json';
       }
     }
 
-    const cacheKey = this.cacheKeyFor(url, modifiedRequest);
+    // At this point we know the `body` is a `string`, `Buffer`, or `undefined`
+    // (not possibly an `object`).
+    const outgoingRequest = augmentedRequest as RequestOptions;
+
+    const cacheKey = this.cacheKeyFor(url, outgoingRequest);
 
     const performRequest = async () => {
-      return this.trace(url, modifiedRequest, async () => {
-        const cacheOptions = modifiedRequest.cacheOptions
-          ? modifiedRequest.cacheOptions
+      return this.trace(url, outgoingRequest, async () => {
+        const cacheOptions = outgoingRequest.cacheOptions
+          ? outgoingRequest.cacheOptions
           : this.cacheOptionsFor?.bind(this);
         try {
-          const response = await this.httpCache.fetch(url, modifiedRequest, {
+          const response = await this.httpCache.fetch(url, outgoingRequest, {
             cacheKey,
             cacheOptions,
           });
-          return await this.didReceiveResponse(response, modifiedRequest);
+          return await this.didReceiveResponse(response, outgoingRequest);
         } catch (error) {
-          this.didEncounterError(error as Error, modifiedRequest);
+          this.didEncounterError(error as Error, outgoingRequest);
         }
       });
     };
 
     // Cache GET requests based on the calculated cache key
     // Disabling the request cache does not disable the response cache
     if (this.memoizeGetRequests) {
-      if (request.method === 'GET') {
+      if (outgoingRequest.method === 'GET') {
         let promise = this.memoizedResults.get(cacheKey);
         if (promise) return promise;