From 4a249ec48e7d32a564ff7805af4435d76dc9cab1 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 29 Nov 2022 15:15:19 -0800 Subject: [PATCH] Preserve `body` on request object passed to `willSendRequest`; fix `string` 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 | 12 +++ .changeset/tidy-spoons-nail.md | 8 ++ README.md | 18 ++-- src/RESTDataSource.ts | 95 +++++++++------- src/__tests__/RESTDataSource.test.ts | 156 ++++++++++++++++++++++----- src/index.ts | 2 +- 6 files changed, 214 insertions(+), 77 deletions(-) create mode 100644 .changeset/cuddly-cars-sparkle.md create mode 100644 .changeset/tidy-spoons-nail.md diff --git a/.changeset/cuddly-cars-sparkle.md b/.changeset/cuddly-cars-sparkle.md new file mode 100644 index 0000000..6175559 --- /dev/null +++ b/.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. diff --git a/.changeset/tidy-spoons-nail.md b/.changeset/tidy-spoons-nail.md new file mode 100644 index 0000000..0dcf1e4 --- /dev/null +++ b/.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). diff --git a/README.md b/README.md index 3211120..c32c50e 100644 --- a/README.md +++ b/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]; diff --git a/src/RESTDataSource.ts b/src/RESTDataSource.ts index d15a036..8580847 100644 --- a/src/RESTDataSource.ts +++ b/src/RESTDataSource.ts @@ -29,13 +29,6 @@ export type RequestOptions = FetcherRequestInit & { ) => CacheOptions | undefined); }; -export type WillSendRequestOptions = Omit< - WithRequired, - 'params' -> & { - params: URLSearchParams; -}; - export interface GetRequest extends RequestOptions { method?: 'GET'; body?: never; @@ -48,6 +41,21 @@ export interface RequestWithBody extends Omit { type DataSourceRequest = GetRequest | RequestWithBody; +// While tempting, this union can't be reduced / factored out to just +// Omit, '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, 'params'> + | Omit, '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; protected resolveURL( path: string, - _request: RequestOptions, + _request: AugmentedRequest, ): ValueOrPromise { return new URL(path, this.baseURL); } @@ -205,63 +214,67 @@ export abstract class RESTDataSource { private async fetch( path: string, - request: DataSourceRequest, + incomingRequest: DataSourceRequest, ): Promise { - 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); } }); }; @@ -269,7 +282,7 @@ export abstract class RESTDataSource { // 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; diff --git a/src/__tests__/RESTDataSource.test.ts b/src/__tests__/RESTDataSource.test.ts index 4835c7a..e12b65b 100644 --- a/src/__tests__/RESTDataSource.test.ts +++ b/src/__tests__/RESTDataSource.test.ts @@ -1,17 +1,13 @@ -// 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'; @@ -19,13 +15,7 @@ import { GraphQLError } from 'graphql'; const apiUrl = 'https://api.example.com'; describe('RESTDataSource', () => { - // let httpCache: HTTPCache; - - beforeEach(() => { - nockBeforeEach(); - // httpCache = new HTTPCache(new MapKeyValueCache()); - }); - + 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(_path: string, 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(_path: string, request: AugmentedRequest) { request.headers = { ...request.headers, credentials: 'include' }; } @@ -247,7 +237,7 @@ describe('RESTDataSource', () => { super(config); } - override willSendRequest(request: WillSendRequestOptions) { + override willSendRequest(_path: string, 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,49 @@ 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 expectedData = 'id=1&name=bar'; + 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 body === expectedData; + }) + .reply(200, 'ok', { 'content-type': 'text/plain' }); + + await dataSource.updateFoo(1, Buffer.from(expectedData)); + }); + describe('all methods', () => { const dataSource = new (class extends RESTDataSource { override baseURL = 'https://api.example.com'; @@ -868,5 +889,84 @@ describe('RESTDataSource', () => { jest.useRealTimers(); }); }); + + describe('user hooks', () => { + describe('willSendRequest', () => { + const obj = { foo: 'bar' }; + const str = 'foo=bar'; + const buffer = Buffer.from(str); + + it.each([ + ['object', obj, obj], + ['string', str, str], + ['buffer', buffer, str], + ])(`can set the body to a %s`, async (_, body, expected) => { + const dataSource = new (class extends RESTDataSource { + override baseURL = apiUrl; + + updateFoo(id: number, foo: string | Buffer | { foo: string }) { + return this.post(`foo/${id}`, { body: foo }); + } + + override async willSendRequest( + path: string, + requestOpts: AugmentedRequest, + ) { + expect(path).toMatch('foo/1'); + expect(requestOpts.body).toEqual(body); + } + })(); + + nock(apiUrl).post('/foo/1', expected).reply(200); + await dataSource.updateFoo(1, body); + }); + + it('is called with the correct path', async () => { + const dataSource = new (class extends RESTDataSource { + override baseURL = apiUrl; + + updateFoo(id: number, foo: { foo: string }) { + return this.post(`foo/${id}`, { body: foo }); + } + + override async willSendRequest( + path: string, + _requestOpts: AugmentedRequest, + ) { + expect(path).toMatch('foo/1'); + } + })(); + + nock(apiUrl).post('/foo/1', obj).reply(200); + await dataSource.updateFoo(1, obj); + }); + }); + + 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' }); + }); + }); + }); }); }); diff --git a/src/index.ts b/src/index.ts index 31328e6..2565813 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,5 +1,5 @@ export { RESTDataSource, RequestOptions, - WillSendRequestOptions, + AugmentedRequest, } from './RESTDataSource';