-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve body
on request object passed to willSendRequest
; fix string
and Buffer
body handling
#89
Changes from 10 commits
9f339b8
3a014ab
2f8c68f
b1f9182
7bbbdfa
3a223f1
896c0cb
bf9ca45
d77f7ff
177e437
e2a241c
66979be
da3bd0d
7ee320d
75c40f9
5666808
e7f914f
4d1d4d2
e685e90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
--- | ||
'@apollo/datasource-rest': patch | ||
--- | ||
|
||
The v4 update introduced multiple regressions w.r.t. the intermediary `modifiedRequest` object that was added. | ||
|
||
1. The `body` was no longer being added to the intermediary request object before calling `willSendRequest` | ||
2. `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 have been updated accordingly. | ||
trevor-scheer marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = ( | ||
trevor-scheer marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be worth a comment that the point here is basically, this is like DataSourceRequest but with BTW unrelated but it seems like |
||
| Omit<WithRequired<GetRequest, 'headers'>, 'params'> | ||
| Omit<WithRequired<RequestWithBody, 'headers'>, 'params'> | ||
) & { | ||
params: URLSearchParams; | ||
}; | ||
|
||
export interface CacheOptions { | ||
ttl?: number; | ||
} | ||
|
@@ -79,12 +82,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 +208,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); | ||
trevor-scheer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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 || | ||
trevor-scheer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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; | ||
trevor-scheer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
export { | ||
RESTDataSource, | ||
RequestOptions, | ||
WillSendRequestOptions, | ||
AugmentedRequest, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name change here alone makes this a v5 change. Could continue exporting this as an alias, but maybe the naming is still up in the air anyway. |
||
} from './RESTDataSource'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this changeset should be more clear that this is about
willSendRequest
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I agree, up above we agreed that the
string | Buffer
problem was a glaring regression. In any case, I've updated the changeset to call out both separately and a bit more clearly.