Preserve body on request object passed to willSendRequest; fix string and Buffer body handling

.changeset/cuddly-cars-sparkle.md

@@ -0,0 +1,17 @@
+---
+'@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.
+
+Related to the regression mentioned above, `string` and `Buffer` bodies were no
+longer being included at all on the outgoing request since they were just
+ignored and never appended to the `body`. `string` and `Buffer` bodies are now
+passed through to the outgoing request (without being JSON stringified).

README.md

@@ -198,7 +198,7 @@ class PersonalizationAPI extends RESTDataSource {
 
 If you're using TypeScript, you can use the `RequestOptions` 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 +207,7 @@ class PersonalizationAPI extends RESTDataSource {
  super();
  }
 
- override willSendRequest(request: WillSendRequestOptions) {
+ override willSendRequest(request: AugmentedRequest) {
  request.headers['authorization'] = this.token;
  }
 }

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,16 @@ 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)
+export type AugmentedRequest = (
+ | Omit<WithRequired<GetRequest, 'headers'>, 'params'>
+ | Omit<WithRequired<RequestWithBody, 'headers'>, 'params'>
+) & {
+ params: URLSearchParams;
+};
+
 export interface CacheOptions {
  ttl?: number;
 }
@@ -79,12 +82,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 +209,74 @@ 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.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;