Detect and serialize objects with null prototype; provide shouldJSONSerializeBody hook to override behavior

.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://github.com/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);
+ });
+ });
  });
  });
 });