Detect and serialize objects with null prototype; provide `shouldJS…

…ONSerializeBody` hook to override behavior (#90)

Simplify the JSON serialization conditional to the inverse:
Serialize if...
* it's an array or plain object
* if it has a toJSON() property on it (and isn't a Buffer or FormData-like)

Test explicitly that FormData is passed through rather than
all non-plain-objects. Passing in a random class is unexpected
unless it's meant to be serialized with a toJSON function.

Provide `shouldJSONSerializeBody` as an overridable method: 

The logic to decide on JSON serialization was previously baked in
to RESTDataSource. In case customization is desired, we've provided
this overridable method as an escape hatch for other use cases.

.changeset/funny-dolls-cheer.md

@@ -0,0 +1,5 @@
+---
+'@apollo/datasource-rest': patch
+---
+
+Correctly identify and serialize all plain objects (like those with a null prototype)

.changeset/wet-birds-occur.md

@@ -0,0 +1,5 @@
+---
+'@apollo/datasource-rest': minor
+---
+
+Add a new overridable method `shouldJSONSerializeBody` for customizing body serialization behavior. This method should return a `boolean` in order to inform RESTDataSource as to whether or not it should call `JSON.stringify` on the request body.

README.md

@@ -162,6 +162,13 @@ If you override this behavior, be sure to implement the proper error handling.
 ##### `didEncounterError`
 By default, this method just throws the `error` it was given. If you override this method, you can choose to either perform some additional logic and still throw, or to swallow the error by not throwing the error result.
 
+#### `shouldJSONSerializeBody`
+By default, this method returns `true` if the request body is:
+- a plain object or an array
+- an object with a `toJSON` method (which isn't a `Buffer` or an instance of a class named `FormData`)
+
+You can override this method in order to serialize other objects such as custom classes as JSON.
+
 ### HTTP Methods
 
 The `get` method on the [`RESTDataSource`](https:/apollographql/datasource-rest/tree/main/src/RESTDataSource.ts) makes an HTTP `GET` request. Similarly, there are methods built-in to allow for `POST`, `PUT`, `PATCH`, and `DELETE` requests.

cspell-dict.txt

@@ -9,7 +9,9 @@ circleci
 codesandbox
 datasource
 direnv
+falsey
 httpcache
+isplainobject
 keyvaluecache
 opde
 revalidates

package.json

@@ -37,8 +37,10 @@
  "@changesets/cli": "2.25.2",
  "@types/http-cache-semantics": "4.0.1",
  "@types/jest": "29.2.3",
+ "@types/lodash.isplainobject": "4.0.7",
  "@types/node": "14.18.33",
  "cspell": "6.15.0",
+ "form-data": "4.0.0",
  "graphql": "16.6.0",
  "jest": "29.3.1",
  "jest-junit": "14.0.1",
@@ -52,6 +54,7 @@
  "@apollo/utils.fetcher": "^1.0.0",
  "@apollo/utils.keyvaluecache": "1.0.2",
  "http-cache-semantics": "^4.1.0",
+ "lodash.isplainobject": "^4.0.6",
  "node-fetch": "^2.6.7"
  },
  "peerDependencies": {

src/RESTDataSource.ts

@@ -1,12 +1,13 @@
-import { HTTPCache } from './HTTPCache';
-import { GraphQLError } from 'graphql';
-import type { KeyValueCache } from '@apollo/utils.keyvaluecache';
 import type {
  Fetcher,
  FetcherRequestInit,
  FetcherResponse,
 } from '@apollo/utils.fetcher';
+import type { KeyValueCache } from '@apollo/utils.keyvaluecache';
 import type { WithRequired } from '@apollo/utils.withrequired';
+import { GraphQLError } from 'graphql';
+import isPlainObject from 'lodash.isplainobject';
+import { HTTPCache } from './HTTPCache';
 
 type ValueOrPromise<T> = T | Promise<T>;
 
@@ -208,6 +209,25 @@ export abstract class RESTDataSource {
  }
  }
 
+ protected shouldJSONSerializeBody(body: RequestWithBody['body']): boolean {
+ return !!(
+ // We accept arbitrary objects and arrays as body and serialize them as JSON.
+ (
+ Array.isArray(body) ||
+ isPlainObject(body) ||
+ // We serialize any objects that have a toJSON method (except Buffers or things that look like FormData)
+ (body &&
+ typeof body === 'object' &&
+ 'toJSON' in body &&
+ typeof (body as any).toJSON === 'function' &&
+ !(body instanceof Buffer) &&
+ // XXX this is a bit of a hacky check for FormData-like objects (in
+ // case a FormData implementation has a toJSON method on it)
+ (body as any).constructor?.name !== 'FormData')
+ )
+ );
+ }
+
  protected async errorFromResponse(response: FetcherResponse) {
  const message = `${response.status}: ${response.statusText}`;
 
@@ -308,16 +328,7 @@ export abstract class RESTDataSource {
  url.searchParams.append(name, value);
  }
 
- // 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 (
- 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'))
- ) {
+ if (this.shouldJSONSerializeBody(augmentedRequest.body)) {
  augmentedRequest.body = JSON.stringify(augmentedRequest.body);
  // If Content-Type header has not been previously set, set to application/json
  if (!augmentedRequest.headers) {

src/__tests__/RESTDataSource.test.ts

@@ -3,15 +3,16 @@ import {
  AuthenticationError,
  CacheOptions,
  DataSourceConfig,
- RequestDeduplicationPolicy,
  ForbiddenError,
+ RequestDeduplicationPolicy,
  RequestOptions,
  RESTDataSource,
 } from '../RESTDataSource';
 
-import { nockAfterEach, nockBeforeEach } from './nockAssertions';
-import nock from 'nock';
+import FormData from 'form-data';
 import { GraphQLError } from 'graphql';
+import nock from 'nock';
+import { nockAfterEach, nockBeforeEach } from './nockAssertions';
 
 const apiUrl = 'https://api.example.com';
 
@@ -322,7 +323,7 @@ describe('RESTDataSource', () => {
  await dataSource.postFoo(model);
  });
 
- it('does not serialize a request body that is not an object', async () => {
+ it('does not serialize FormData', async () => {
  const dataSource = new (class extends RESTDataSource {
  override baseURL = 'https://api.example.com';
 
@@ -331,10 +332,17 @@ describe('RESTDataSource', () => {
  }
  })();
 
- class FormData {}
  const form = new FormData();
+ form.append('foo', 'bar');
 
- nock(apiUrl).post('/foo').reply(200);
+ nock(apiUrl)
+ .post('/foo', (body) => {
+ expect(body).toMatch(
+ 'Content-Disposition: form-data; name="foo"\r\n\r\nbar\r\n',
+ );
+ return true;
+ })
+ .reply(200);
 
  await dataSource.postFoo(form);
  });
@@ -382,6 +390,26 @@ describe('RESTDataSource', () => {
  await dataSource.updateFoo(1, Buffer.from(expectedData));
  });
 
+ it('serializes a request body that is an object with a null prototype', async () => {
+ interface Foo {
+ hello: string;
+ }
+ const dataSource = new (class extends RESTDataSource {
+ override baseURL = 'https://api.example.com';
+
+ postFoo(foo: Foo) {
+ return this.post('foo', { body: foo });
+ }
+ })();
+
+ const foo: Foo = Object.create(null);
+ foo.hello = 'world';
+
+ nock(apiUrl).post('/foo', { hello: 'world' }).reply(200);
+
+ await dataSource.postFoo(foo);
+ });
+
  describe('all methods', () => {
  const dataSource = new (class extends RESTDataSource {
  override baseURL = 'https://api.example.com';
@@ -1093,6 +1121,30 @@ describe('RESTDataSource', () => {
  await dataSource.updateFoo(1, { name: 'blah' });
  });
  });
+
+ describe('shouldJSONSerializeBody', () => {
+ it('can be overridden', async () => {
+ let calls = 0;
+ const dataSource = new (class extends RESTDataSource {
+ override baseURL = apiUrl;
+
+ updateFoo(id: number, foo: { name: string }) {
+ return this.post(`foo/${id}`, { body: foo });
+ }
+
+ override shouldJSONSerializeBody(
+ body: string | object | Buffer | undefined,
+ ) {
+ calls++;
+ return super.shouldJSONSerializeBody(body);
+ }
+ })();
+
+ nock(apiUrl).post('/foo/1', { name: 'bar' }).reply(200);
+ await dataSource.updateFoo(1, { name: 'bar' });
+ expect(calls).toBe(1);
+ });
+ });
  });
  });
 });