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

trevor-scheer · 2022-11-10T19:44:36Z

The v4 update introduced multiple regressions w.r.t. the modifiedRequest object that I 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 is now (path, request).

I'm a bit disappointed in the complexity of the typings in here. We make multiple incremental refinements to the request object:

first by ensuring headers/params, effectively adding one level of WithRequired to our incoming request type
second by refining the body type to drop the object from the union since those are stringified

Our hooks expose the request object that we're building at various points in the construction of the request so these types are part of the public API - kind of confusing and difficult to name in ways that are simple and sensible.
i.e. RequestWithHeadersAndParams and RequestWithHeadersAndParamsAndBodyIsCompatibleWithFetch

Fixes #83

codesandbox-ci · 2022-11-10T19:45:01Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

trevor-scheer · 2022-11-10T19:50:50Z

@marcin-zajac would you mind testing out the fix here and letting me know if it resolves the issue for you? Builds are in the codesandbox ci link (my most recent build at the time of writing this can be installed via npm i https://pkg.csb.dev/apollographql/datasource-rest/commit/36c5b4ec/@apollo/datasource-rest)

trevor-scheer · 2022-11-10T20:19:01Z

One drawback to this implementation: hooks get the stringified version of things, meaning they'll have to re-JSON.parse() the content if they're interested in it. We could optionally provide an additional argument (like originalBody) or something if that seems valuable.

leonbe02 · 2022-11-15T01:25:25Z

@trevor-scheer Can you also include in this PR the ability to send POST requests with Content-Type: application/x-www-form-urlencoded with the body being a string ? Currently the code is ignoring the body if it is a url encoded form which is a very common use case for older APIs that don't use JSON.

trevor-scheer · 2022-11-15T01:44:13Z

@leonbe02 that sounds like a reasonable suggestion, but do you think it belongs with this PR for a particular reason? I think it's a separate concern which I'd be happy to address in a different PR unless you have an argument for these pieces of work being the same concern.

Opening a separate issue would be helpful for me if you don't mind. Please be as descriptive as possible. Are there any current workarounds you're employing?

leonbe02 · 2022-11-15T01:52:02Z

@trevor-scheer I bring it up in this PR because this PR claims to resolve #83 which mentions the application/x-www-form-urlencoded bug and you're touching the same lines of code that would need to be updated to resolve the bug. Simply removing the body: undefined part of modifiedRequest would allow for the body to be a string and pass thru to the fetch function.

trevor-scheer · 2022-11-16T00:48:18Z

@leonbe02 let me know if the changes I've made match your expectation. For typing reasons, it's not as simple as passing the body through straight from the request object, but I can append it without serializing when it's a string which seems reasonable. Of the possible types (undefined, object, Buffer, string), these should all be accounted for now.

I added 2 commits: one with a test to show what was failing, and a second which implements the fix and brings the test to passing.

leonbe02

Looks good, thanks for doing that!

trevor-scheer · 2022-11-16T00:58:29Z

Thanks for calling it out and taking the time to review 👍

glasser · 2022-11-18T00:33:29Z

didn't full review yet, but make sure to update the commit message with the second change that was added, and to write a changeset.

src/RESTDataSource.ts

The v3 implementation of RESTDataSource did not have the concept of a "modified" request. The params and headers were updated in place and the body was passed directly through to the willSendRequest and resolveURL hooks. The body was then updated if necessary, meaning neither of those hooks actually saw the final outgoing request. The updated implementation for v4 was indeed a regression, where the original body was no longer included on the request object before those hooks were called. To remediate this, let's fully build our modified request object (including final updates to the body and headers) before we call any hooks. This resolves the undefined body regression, and does a better job of reporting the actual outgoing request to the willSendRequest and resolveURL hooks`. This does not restore original v3 behavior, but it is not a breaking change against v4 since it is strictly additive. There is a slight change in behavior in that when the body is JSON stringified (from an array or object), the response headers' content-type will be updated to reflect that if not already set. I would consider this also additive. Fixes #83

This commit undoes destructive changes to the semantics of `willSendRequest` i.e. when it is called and with what parameters. The original `body` is now passed through to the `augmentedRequest` as soon as it is created and before calling `willSendRequest`, so the body can now always be observed when the hook is called. Additionally, `Buffer`s are now passed through (and tested against)

src/RESTDataSource.ts

…gs for willSendRequest

trevor-scheer · 2022-11-29T01:22:36Z

src/index.ts

@@ -1,5 +1,5 @@
 export {
 RESTDataSource,
 RequestOptions,
- WillSendRequestOptions,
+ AugmentedRequest,


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.

glasser · 2022-11-29T01:50:11Z

.changeset/cuddly-cars-sparkle.md

+'@apollo/datasource-rest': patch
+---
+
+The v4 update introduced multiple regressions w.r.t. the intermediary `modifiedRequest` object that was added.


I think this changeset should be more clear that this is about willSendRequest?

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.

.changeset/cuddly-cars-sparkle.md

src/RESTDataSource.ts

Update changeset

trevor-scheer · 2022-11-29T19:50:44Z

@glasser I can't find out how to reply to your comment about the type name.

Worth mentioning this type is also part of the resolveURL signature, so it's used for both hooks. Maybe IntermediaryRequest makes sense? We now have the concept (in code) of an incoming vs. outgoing request.

glasser

Think this looks good modulo these concerns — lmk if you want me to take another look after any adjustments.

.changeset/cuddly-cars-sparkle.md

glasser · 2022-11-29T21:28:32Z

src/RESTDataSource.ts

+// 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 = (


Might be worth a comment that the point here is basically, this is like DataSourceRequest but with headers and params normalized for convenience in your callbacks.

BTW unrelated but it seems like method shouldn't be optional in GetRequest/RequestWithBody?

README.md

src/RESTDataSource.ts

README.md

.changeset/tidy-spoons-nail.md

trevor-scheer requested a review from glasser November 10, 2022 19:44

trevor-scheer force-pushed the trevor/update-modifiedrequest-body branch from 36c5b4e to dcbcc1d Compare November 10, 2022 20:16

trevor-scheer force-pushed the trevor/update-modifiedrequest-body branch from 1aecfc7 to 8758734 Compare November 16, 2022 00:43

leonbe02 approved these changes Nov 16, 2022

View reviewed changes

glasser reviewed Nov 23, 2022

View reviewed changes

src/RESTDataSource.ts Outdated Show resolved Hide resolved

glasser reviewed Nov 23, 2022

View reviewed changes

src/RESTDataSource.ts Outdated Show resolved Hide resolved

trevor-scheer added 5 commits November 28, 2022 16:36

Test demonstrating bad behavior

9f339b8

failing test using urlencoded body - body is not matched by nock

2f8c68f

Append string-type bodies to the modified request

b1f9182

trevor-scheer force-pushed the trevor/update-modifiedrequest-body branch from a2e1461 to 7bbbdfa Compare November 29, 2022 01:07

Update type name

3a223f1

trevor-scheer commented Nov 29, 2022

View reviewed changes

src/RESTDataSource.ts Show resolved Hide resolved

trevor-scheer added 2 commits November 28, 2022 17:14

Stop exporting DataSourceRequest type (private API) and correct typin…

896c0cb

…gs for willSendRequest

Restore previous resolveURL semantics

bf9ca45

trevor-scheer commented Nov 29, 2022

View reviewed changes

add comment about type

d77f7ff

trevor-scheer changed the title ~~Update modifiedRequest.body before it's used by hooks~~ Preserve body on request object passed to willSendRequest; fix string and Buffer body Nov 29, 2022

changeset

177e437

glasser reviewed Nov 29, 2022

View reviewed changes

trevor-scheer added 2 commits November 29, 2022 11:43

Include path in willSendRequest call

e2a241c

Update changeset

use common syntax

66979be

trevor-scheer requested a review from glasser November 29, 2022 19:51

trevor-scheer added 2 commits November 29, 2022 11:52

prettier

da3bd0d

Stop serializing Buffer (it has a toJSON and must be explicitly ignored)

7ee320d

glasser approved these changes Nov 29, 2022

View reviewed changes

trevor-scheer added 4 commits November 29, 2022 13:42

separate changesets

75c40f9

Update comment

5666808

README updates

e7f914f

Add link to intercepting fetches section

4d1d4d2

glasser reviewed Nov 29, 2022

View reviewed changes

src/RESTDataSource.ts Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

.changeset/tidy-spoons-nail.md Outdated Show resolved Hide resolved

final feedback

e685e90

trevor-scheer changed the title ~~Preserve body on request object passed to willSendRequest; fix string and Buffer body~~ Preserve body on request object passed to willSendRequest; fix string and Buffer body handling Nov 29, 2022

trevor-scheer merged commit 4a249ec into main Nov 29, 2022

trevor-scheer deleted the trevor/update-modifiedrequest-body branch November 29, 2022 23:15

github-actions bot mentioned this pull request Nov 29, 2022

Version Packages #93

Merged

trevor-scheer mentioned this pull request Dec 7, 2022

Can't make POST request with 'Content-Type': 'application/x-www-form-urlencoded' #113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

trevor-scheer commented Nov 10, 2022 •

edited

Loading

codesandbox-ci bot commented Nov 10, 2022 •

edited

Loading

trevor-scheer commented Nov 10, 2022

trevor-scheer commented Nov 10, 2022

leonbe02 commented Nov 15, 2022

trevor-scheer commented Nov 15, 2022

leonbe02 commented Nov 15, 2022 •

edited

Loading

trevor-scheer commented Nov 16, 2022

leonbe02 left a comment

trevor-scheer commented Nov 16, 2022

glasser commented Nov 18, 2022

trevor-scheer Nov 29, 2022

glasser Nov 29, 2022

trevor-scheer Nov 29, 2022

trevor-scheer commented Nov 29, 2022

glasser left a comment

glasser Nov 29, 2022

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

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

Conversation

trevor-scheer commented Nov 10, 2022 • edited Loading

codesandbox-ci bot commented Nov 10, 2022 • edited Loading

trevor-scheer commented Nov 10, 2022

trevor-scheer commented Nov 10, 2022

leonbe02 commented Nov 15, 2022

trevor-scheer commented Nov 15, 2022

leonbe02 commented Nov 15, 2022 • edited Loading

trevor-scheer commented Nov 16, 2022

leonbe02 left a comment

Choose a reason for hiding this comment

trevor-scheer commented Nov 16, 2022

glasser commented Nov 18, 2022

trevor-scheer Nov 29, 2022

Choose a reason for hiding this comment

glasser Nov 29, 2022

Choose a reason for hiding this comment

trevor-scheer Nov 29, 2022

Choose a reason for hiding this comment

trevor-scheer commented Nov 29, 2022

glasser left a comment

Choose a reason for hiding this comment

glasser Nov 29, 2022

Choose a reason for hiding this comment

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

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

trevor-scheer commented Nov 10, 2022 •

edited

Loading

codesandbox-ci bot commented Nov 10, 2022 •

edited

Loading

leonbe02 commented Nov 15, 2022 •

edited

Loading