Allow used body replacement in Request constructor #675

harrishancock · 2018-02-28T18:22:29Z

Currently, the following code snippet replaces request's body as one would expect:

let request = new Request(url, { method: "POST", body: "foo" })
request = new Request(request, { body: "bar" })

But this snippet throws a TypeError early in Request's constructor:

let request = new Request(url, { method: "POST", body: "foo" })
await request.text()  // disturb the body
request = new Request(request, { body: "bar" })  // throws

This commit's changes allows the latter code snippet to work like the first one.

Fixes #674.

Preview | Diff

annevk · 2018-03-01T09:13:03Z

@terinjokes could you add @harrishancock as a public member of the cloudflare-whatwg organization?

annevk · 2018-03-01T09:20:16Z

I wonder if we should move step 7.1 down to the other body checks (and I also wonder if we could simplify those somewhat).

terinjokes · 2018-03-01T17:58:33Z

@annevk This is in progress.

terinjokes · 2018-03-01T20:36:11Z

@annevk Done. Sorry for the delay.

harrishancock · 2018-03-01T21:45:40Z

I wonder if we should move step 7.1 down to the other body checks (and I also wonder if we could simplify those somewhat).

Agreed about moving the check down with the others. I'll find a nice spot for it and amend the commit.

I'm not sure there's a better factorization of the body setup steps (38-44). :/ It seems largely complicated by the fact that nothing after the input Request object body stealing (setting up rs and piping to it) is allowed to throw (to enforce the strong exception safety guarantee, I suppose). Maybe the best thing to do would be to defer the piping action until after all the checks are complete?

Let action be null.
Let body be null.
[Step 41] If init's body member is present, run these substeps:
1. Let Content-Type be null.
2. If init's keepalive member is present and is true, then set body
  and Content-Type to the result of extracting init's body member,
  with keepalive flag set.
3. Otherwise, set body and Content-Type to the result of extracting
  init's body member.
4. If Content-Type is non-null and r's headers's header list does not
  contain 'Content-Type', then append 'Content-Type'/Content-Type to
  r's headers.
Otherwise, if input is a Request object, run these substeps:
1. [Step 7.1] If input is disturbed or locked, throw a TypeError.
2. Set body to input's request's body.
3. Set action to an action that runs the following substeps:
  1. [Step 43.1] Let rs be a ReadableStream object from which one can read the exact
    same data as one could read from body's stream.
  2. [Step 43.2] Set body to a new body whose stream is rs, whose source is body's
    source and whose total bytes is body's total bytes.
[Step 39] If body is non-null and request's method is 'GET' or 'HEAD', then
throw a TypeError.
[Step 42] If body is non-null and body's source is null, then run these substeps:
1. If r's request's mode is neither "same-origin" nor "cors", then
  throw a TypeError.
2. Set r's request's use-CORS-preflight flag.
If action is non-null, run action immediately.
[Step 44] Set r's request's body to body.

I'm not really sure that's "better". :) @yutakahirano?

harrishancock · 2018-03-01T22:14:07Z

I wonder if we should move step 7.1 down to the other body checks

Actually, is the check in 7.1 even necessary? The logical place for it is to add it to the beginning of step 43's substeps, but it seems like it's already implicit in step 43.1 -- piping to rs.

annevk · 2018-03-02T08:46:45Z

Good question, I guess it depends on if it's observably different. Maybe not, though we'll need to fix #463 to be sure.

yutakahirano · 2018-03-02T09:28:14Z

My intention has been they are same.

yutakahirano · 2018-03-02T09:31:42Z

Whether we should keep step 7.1 depends on whether we want to separate the read-only part and state-modifying part (currently we don't throw an exception from this part).

Currently, the following code snippet replaces `request`'s body as one would expect: let request = new Request(url, { method: "POST", body: "foo" }) request = new Request(request, { body: "bar" }) But this snippet throws a TypeError early in Request's constructor: let request = new Request(url, { method: "POST", body: "foo" }) await request.text() // disturb the body request = new Request(request, { body: "bar" }) // throws This commit's changes allows the latter code snippet to work like the first one. Fixes whatwg#674.

harrishancock · 2018-03-05T19:20:02Z

Amended to move the disturbed or locked check closer to where it's relevant: right before the inputBody piping. To me, it would make most sense to either put the check in the inputBody piping substeps, or make it implicit in the piping itself, as I mentioned. Until that decision is finalized, this seems like the best we can do: the "critical line" between the read-only part of the constructor and the state-modifying part remains in the top-level steps (as opposed to the piping substeps, which are conditional); the piping operation is guaranteed not to throw; and the check and the piping operation are written as close to each other as possible to make future maintenance easier.

yutakahirano · 2018-03-06T03:42:19Z

Can you add a web platform test?

harrishancock · 2018-03-07T00:56:23Z

Can you add a web platform test?

Will do. I had hoped to get to it today, but ran out of time. Soon!

harrishancock · 2018-03-15T00:20:42Z

Ping. :) WPT PR here: web-platform-tests/wpt#9931

annevk · 2018-03-15T08:01:11Z

fetch.bs

@@ -5348,10 +5345,14 @@ constructor must run these steps:
   <a for=request>use-CORS-preflight flag</a>.
  </ol>

+ <li><p>If <var>inputBody</var> is <var>body</var> and <var>input</var> is <a for=Body>disturbed</a>
+ or <a for=Body>locked</a> then <a>throw</a> a <code>TypeError</code>.


Needs a comma before then.

annevk

I can fix these nits if you don't want to, FWIW.

annevk · 2018-03-15T08:01:49Z

fetch.bs

 <!-- Any steps after this must not throw. -->

 <li>
-  <p>If <var>inputBody</var> is non-null, then run these substeps:
+  <p>If <var>inputBody</var> is non-null and <var>inputBody</var> is <var>body</var>, then run these
+  substeps:


This should be just ", then:"

annevk · 2018-03-15T08:02:55Z

fetch.bs

-   <var>inputBody</var>'s <a for=body>total bytes</a>.
+   <li><p>Set <var>body</var> to a new <a for=/>body</a> whose <a for=body>stream</a> is
+   <var>rs</var>, whose <a for=body>source</a> is <var>inputBody</var>'s <a for=body>source</a> and
+   whose <a for=body>total bytes</a> is <var>inputBody</var>'s <a for=body>total bytes</a>.


This needs an Oxford comma. (Also a problem in the original it seems.)

annevk · 2018-03-15T08:07:18Z

It seems this feature isn't implemented by browsers yet so no impl bugs to file.

annevk · 2018-03-15T08:11:41Z

If someone could confirm the above I think we're good to go.

ricea · 2018-03-15T09:14:56Z

@annevk Safari tech preview has it, does that count? It throws TypeError: "Request input is disturbed or locked.".

Edit: It's also in stable version 11.0.3.

annevk · 2018-03-15T09:29:09Z

That certainly counts, thanks! @harrishancock could you file a bug at https://bugs.webkit.org/enter_bug.cgi?product=WebKit&component=HTML%20DOM?

harrishancock · 2018-03-16T18:05:31Z

@annevk, thank you for fixing the nits, I appreciate it!

I've submitted a WebKit bug here: https://bugs.webkit.org/show_bug.cgi?id=183703

See Fetch change whatwg/fetch#675. Specifically, that PR allows Requests with disturbed bodies to be used as the first parameter to the Request constructor, as long as the RequestInit dictionary contains a body member with which to replace the used body. As of this writing, no browser implements this change.

annevk · 2018-03-17T06:03:11Z

Thanks, all merged now!

…t constructor, a=testonly Automatic update from web-platform-testsFetch: add used body replacement test for Request constructor See Fetch change whatwg/fetch#675. Specifically, that PR allows Requests with disturbed bodies to be used as the first parameter to the Request constructor, as long as the RequestInit dictionary contains a body member with which to replace the used body. As of this writing, no browser implements this change. wpt-commits: e87f38097902e16348d4e17f4fe3bc2d0112bff1 wpt-pr: 9931 wpt-commits: e87f38097902e16348d4e17f4fe3bc2d0112bff1 wpt-pr: 9931

https://bugs.webkit.org/show_bug.cgi?id=183703 <rdar://problem/49425609> Reviewed by Youenn Fablet. LayoutTests/imported/w3c: Rebaseline WPT test now that one more check is passing. * web-platform-tests/fetch/api/request/request-disturbed-expected.txt: Source/WebCore: Allow used body replacement in Request constructor as per: - whatwg/fetch#675 No new tests, rebaseline existing test. * Modules/fetch/FetchRequest.cpp: (WebCore::FetchRequest::initializeWith): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@243757 268f45cc-cd09-0410-ab3c-d52691b4dbfc

…t constructor, a=testonly Automatic update from web-platform-testsFetch: add used body replacement test for Request constructor See Fetch change whatwg/fetch#675. Specifically, that PR allows Requests with disturbed bodies to be used as the first parameter to the Request constructor, as long as the RequestInit dictionary contains a body member with which to replace the used body. As of this writing, no browser implements this change. wpt-commits: e87f38097902e16348d4e17f4fe3bc2d0112bff1 wpt-pr: 9931 wpt-commits: e87f38097902e16348d4e17f4fe3bc2d0112bff1 wpt-pr: 9931 UltraBlame original commit: ad7b8e6c381646d827339d34e4ac9db07654bcc8

harrishancock mentioned this pull request Feb 28, 2018

Inconsistency in Request constructor when rewriting a used body #674

Closed

harrishancock force-pushed the harrishancock/allow-used-request-body-replacement branch from e0049c6 to 121a61b Compare March 5, 2018 19:13

yutakahirano approved these changes Mar 6, 2018

View reviewed changes

harrishancock mentioned this pull request Mar 8, 2018

Add used body replacement test for Request constructor web-platform-tests/wpt#9931

Merged

annevk reviewed Mar 15, 2018

View reviewed changes

fix my own nits

03a8ac0

annevk merged commit 5b7dae0 into whatwg:master Mar 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow used body replacement in Request constructor #675

Allow used body replacement in Request constructor #675

harrishancock commented Feb 28, 2018 •

edited by pr-preview bot

annevk commented Mar 1, 2018

annevk commented Mar 1, 2018

terinjokes commented Mar 1, 2018

terinjokes commented Mar 1, 2018

harrishancock commented Mar 1, 2018

harrishancock commented Mar 1, 2018

annevk commented Mar 2, 2018

yutakahirano commented Mar 2, 2018

yutakahirano commented Mar 2, 2018

harrishancock commented Mar 5, 2018

yutakahirano commented Mar 6, 2018

harrishancock commented Mar 7, 2018

harrishancock commented Mar 15, 2018

annevk Mar 15, 2018

annevk left a comment

annevk Mar 15, 2018

annevk Mar 15, 2018

annevk commented Mar 15, 2018

annevk commented Mar 15, 2018

ricea commented Mar 15, 2018 •

edited

annevk commented Mar 15, 2018

harrishancock commented Mar 16, 2018

annevk commented Mar 17, 2018

Allow used body replacement in Request constructor #675

Allow used body replacement in Request constructor #675

Conversation

harrishancock commented Feb 28, 2018 • edited by pr-preview bot

annevk commented Mar 1, 2018

annevk commented Mar 1, 2018

terinjokes commented Mar 1, 2018

terinjokes commented Mar 1, 2018

harrishancock commented Mar 1, 2018

harrishancock commented Mar 1, 2018

annevk commented Mar 2, 2018

yutakahirano commented Mar 2, 2018

yutakahirano commented Mar 2, 2018

harrishancock commented Mar 5, 2018

yutakahirano commented Mar 6, 2018

harrishancock commented Mar 7, 2018

harrishancock commented Mar 15, 2018

annevk Mar 15, 2018

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

annevk Mar 15, 2018

Choose a reason for hiding this comment

annevk Mar 15, 2018

Choose a reason for hiding this comment

annevk commented Mar 15, 2018

annevk commented Mar 15, 2018

ricea commented Mar 15, 2018 • edited

annevk commented Mar 15, 2018

harrishancock commented Mar 16, 2018

annevk commented Mar 17, 2018

harrishancock commented Feb 28, 2018 •

edited by pr-preview bot

ricea commented Mar 15, 2018 •

edited