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

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,13 @@ export interface RequestWithBody extends Omit<RequestOptions, 'body'> {
 
 type DataSourceRequest = GetRequest | RequestWithBody;
 
+export type AugmentedRequest = (
+ | Omit<WithRequired<GetRequest, 'headers'>, 'params'>
+ | Omit<WithRequired<RequestWithBody, 'headers'>, 'params'>
+) & {
+ params: URLSearchParams;
+};
+
 export interface CacheOptions {
  ttl?: number;
 }
@@ -79,12 +79,12 @@ export abstract class RESTDataSource {
  }
 
  protected willSendRequest?(
- requestOpts: WillSendRequestOptions,
+ requestOpts: AugmentedRequest,
  ): ValueOrPromise<void>;
 
  protected resolveURL(
  path: string,
- _request: RequestOptions,
+ _request: AugmentedRequest,
  ): ValueOrPromise<URL> {
  return new URL(path, this.baseURL);
  }
@@ -205,71 +205,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(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 = <RequestOptions>augmentedRequest;
+
+ 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;
 

src/__tests__/RESTDataSource.test.ts

@@ -1,31 +1,21 @@
-// import fetch, { Request } from 'node-fetch';
 import {
+ AugmentedRequest,
  AuthenticationError,
  CacheOptions,
  DataSourceConfig,
  ForbiddenError,
  RequestOptions,
  RESTDataSource,
- WillSendRequestOptions,
- // RequestOptions
 } from '../RESTDataSource';
 
-// import { HTTPCache } from '../HTTPCache';
-// import { MapKeyValueCache } from './MapKeyValueCache';
 import { nockAfterEach, nockBeforeEach } from './nockAssertions';
 import nock from 'nock';
 import { GraphQLError } from 'graphql';
 
 const apiUrl = 'https://api.example.com';
 
 describe('RESTDataSource', () => {
- // let httpCache: HTTPCache;
-
- beforeEach(() => {
- nockBeforeEach();
- // httpCache = new HTTPCache(new MapKeyValueCache<string>());
- });
-
+ beforeEach(nockBeforeEach);
  afterEach(nockAfterEach);
 
  describe('constructing requests', () => {
@@ -123,7 +113,7 @@ describe('RESTDataSource', () => {
 
  it('allows resolving a base URL asynchronously', async () => {
  const dataSource = new (class extends RESTDataSource {
- override async resolveURL(path: string, request: RequestOptions) {
+ override async resolveURL(path: string, request: AugmentedRequest) {
  if (!this.baseURL) {
  this.baseURL = 'https://api.example.com';
  }
@@ -205,7 +195,7 @@ describe('RESTDataSource', () => {
  super(config);
  }
 
- override willSendRequest(request: WillSendRequestOptions) {
+ override willSendRequest(request: AugmentedRequest) {
  request.params.set('apiKey', this.token);
  }
 
@@ -225,7 +215,7 @@ describe('RESTDataSource', () => {
  const dataSource = new (class extends RESTDataSource {
  override baseURL = 'https://api.example.com';
 
- override willSendRequest(request: WillSendRequestOptions) {
+ override willSendRequest(request: AugmentedRequest) {
  request.headers = { ...request.headers, credentials: 'include' };
  }
 
@@ -247,7 +237,7 @@ describe('RESTDataSource', () => {
  super(config);
  }
 
- override willSendRequest(request: WillSendRequestOptions) {
+ override willSendRequest(request: AugmentedRequest) {
  request.headers = { ...request.headers, authorization: this.token };
  }
 
@@ -267,18 +257,6 @@ describe('RESTDataSource', () => {
  await dataSource.getFoo('1');
  });
 
- // function expectJSONFetch(url: string, bodyJSON: unknown) {
- // expect(fetch).toBeCalledTimes(1);
- // const request = fetch.mock.calls[0][0] as Request;
- // expect(request.url).toEqual(url);
- // // request.body is a node-fetch extension which we aren't properly
- // // capturing in our TS types.
- // expect((request as any).body.toString()).toEqual(
- // JSON.stringify(bodyJSON),
- // );
- // expect(request.headers.get('Content-Type')).toEqual('application/json');
- // }
-
  it('serializes a request body that is an object as JSON', async () => {
  const expectedFoo = { foo: 'bar' };
  const dataSource = new (class extends RESTDataSource {
@@ -360,6 +338,48 @@ describe('RESTDataSource', () => {
  await dataSource.postFoo(form);
  });
 
+ it('does not serialize (but does include) string request bodies', async () => {
+ const dataSource = new (class extends RESTDataSource {
+ override baseURL = 'https://api.example.com';
+
+ updateFoo(id: number, urlEncodedFoo: string) {
+ return this.post(`foo/${id}`, {
+ headers: { 'content-type': 'application/x-www-urlencoded' },
+ body: urlEncodedFoo,
+ });
+ }
+ })();
+
+ nock(apiUrl)
+ .post('/foo/1', (body) => {
+ return body === 'id=1&name=bar';
+ })
+ .reply(200, 'ok', { 'content-type': 'text/plain' });
+
+ await dataSource.updateFoo(1, 'id=1&name=bar');
+ });
+
+ it('does not serialize (but does include) `Buffer` request bodies', async () => {
+ const dataSource = new (class extends RESTDataSource {
+ override baseURL = 'https://api.example.com';
+
+ updateFoo(id: number, fooBuf: Buffer) {
+ return this.post(`foo/${id}`, {
+ headers: { 'content-type': 'application/octet-stream' },
+ body: fooBuf,
+ });
+ }
+ })();
+
+ nock(apiUrl)
+ .post('/foo/1', (body) => {
+ return Buffer.from(body.data).toString() === 'id=1&name=bar';
+ })
+ .reply(200, 'ok', { 'content-type': 'text/plain' });
+
+ await dataSource.updateFoo(1, Buffer.from('id=1&name=bar'));
+ });
+
  describe('all methods', () => {
  const dataSource = new (class extends RESTDataSource {
  override baseURL = 'https://api.example.com';
@@ -868,5 +888,58 @@ describe('RESTDataSource', () => {
  jest.useRealTimers();
  });
  });
+
+ describe('user hooks', () => {
+ describe('willSendRequest', () => {
+ it('sees the same request body as provided by the caller', async () => {
+ const dataSource = new (class extends RESTDataSource {
+ override baseURL = apiUrl;
+
+ updateFoo(id: number, foo: { name: string }) {
+ return this.post(`foo/${id}`, { body: foo });
+ }
+
+ override async willSendRequest(requestOpts: AugmentedRequest) {
+ expect(requestOpts.body).toMatchInlineSnapshot(`
+ {
+ "name": "blah",
+ }
+ `);
+ }
+ })();
+
+ nock(apiUrl)
+ .post('/foo/1', JSON.stringify({ name: 'blah' }))
+ .reply(200);
+ await dataSource.updateFoo(1, { name: 'blah' });
+ });
+ });
+
+ describe('resolveURL', () => {
+ it('sees the same request body as provided by the caller', async () => {
+ const dataSource = new (class extends RESTDataSource {
+ override baseURL = apiUrl;
+
+ updateFoo(id: number, foo: { name: string }) {
+ return this.post(`foo/${id}`, { body: foo });
+ }
+
+ override resolveURL(path: string, requestOpts: AugmentedRequest) {
+ expect(requestOpts.body).toMatchInlineSnapshot(`
+ {
+ "name": "blah",
+ }
+ `);
+ return super.resolveURL(path, requestOpts);
+ }
+ })();
+
+ nock(apiUrl)
+ .post('/foo/1', JSON.stringify({ name: 'blah' }))
+ .reply(200);
+ await dataSource.updateFoo(1, { name: 'blah' });
+ });
+ });
+ });
  });
 });

src/index.ts

@@ -1,5 +1,5 @@
 export {
  RESTDataSource,
  RequestOptions,
- WillSendRequestOptions,
+ AugmentedRequest,
 } from './RESTDataSource';