Deno: Possible broken test case "responseFormData"

[lowlighter]

Hi,
I'm currently working on a #8063 to make assertions handle symbols correctly, but it seems to reveal a broken test case.

The test in question is responseFormData which is below :

unitTest(async function responseFormData() {
  const input = new FormData();
  input.append("hello", "world");
  const response = new Response(input, {
    headers: { "content-type": "application/x-www-form-urlencoded" },
  });
  const formDataPromise = response.formData();
  assert(formDataPromise instanceof Promise);
  const formData = await formDataPromise;
  assert(formData instanceof FormData);
  assertEquals(formData, input);
});

It fails with the following result when using symbol checks :

AssertionError: Values are not equal:
    [Diff] Actual / Expected
    FormData {
      [Symbol(data)]: [
        [
-         "[object FormData]",
-         "",
+         "hello",
+         "world",
        ],
      ],
    }

With Deno.inspect , input returns FormData { [Symbol(data)]: [ [ "hello", "world" ] ] while formData returns FormData { [Symbol(data)]: [ [ "[object FormData]", "" ] ] } (like if the formData was casted to string mistakenly).

My guess is that since data are indexed with a symbol instead of a regular key, the assertEquals was actually ignoring it (because it uses for (const key in object) which doesn't include symbols).

It could possibly mean that response.formData() or this test case is actually broken.

[kitsonk]

It looks like a bad test... comparing FormData what way doesn't make any sense, and your change highlighted that it "wrong".

[lucacasonato]

Whoops. That is my doing. Should be fixed relatively easily by replacing the last assert with

  assertEquals([...formData.entries()], [...input.entries()]);

[lowlighter]

Whoops. That is my doing. Should be fixed relatively easily by replacing the last assert with

  assertEquals([...formData.entries()], [...input.entries()]);

It doesn't seem to work because the value of [...formData.entries()] is actually [ [ "[object FormData]", "" ] ] so it'll still throws on the assertion. Not sure why it ended up like this though.

[lucacasonato]

Interesting. Looks like a bug in our Body::formData method. I will investigate.

[ariain]

hey @lucacasonato , I would like to give this issue a try, can I work on it??

[lucacasonato]

Sure. I looked into this a few days ago, and it looks more complicated than I initially expected. But feel free to give it a shot.

[ariain]

@lucacasonato thanks...

[ariain]

@lowlighter can you tell what command you used to test these test cases...

[lowlighter]

@lowlighter can you tell what command you used to test these test cases...

You can checkout on #8063 to test.

Basically I attempted to run the unit test I provided earlier with Deno.test but for debugging you can just copy-paste the code and add a console.log([...formData.entries()], [...input.entries()]) before the assertion, it should be enough to check whether you fixed it or not.

I tried to fix it when I encoutered this error but I reckon that I had no clue from where to start so that's why I opened this issue x)

[ariain]

Hi @lucacasonato , I was testing this code and is it necessary here to send the header in Response params in following code??

const temp = async () => {
  const input = new FormData();
  input.append("hello", "world");
  const response = new Response(input, {
    headers: { "content-type": "application/x-www-form-urlencoded" },
  });
  const formDataPromise = response.formData();
  const formData = await formDataPromise;
  console.log(...input.entries());
  console.log(...formData.entries());
};

temp();

I am asking this because the data in console is

["hello", "world"]

["------WebKitFormBoundaryCoOAzj6Jc8KdKUPk
Content-Disposition: form-data; name", "'hello'

world
------WebKitFormBoundaryCoOAzj6Jc8KdKUPk--
"]

I think this is the problem here...

[lucacasonato]

Are you running this in Deno?

[ariain]

I ran this in browser actually..

[ariain]

Ran in deno and got

[ "hello", "world" ]

[ "[object FormData]", "" ]

I think this is a much bigger issue than just a wrong test case...

[ariain]

@lucacasonato "good first bug" tag shouldn’t be here in this issue...

[ariain]

@lucacasonato should there any difference between values from chrome and deno??

[ariain]

Hi @lucacasonato and @lowlighter , I tried to look into this but its too complex for me as I am a beginner in open source. Please look into this. I believe the code needed is here somewhere...
https://github.com/denoland/deno/blob/c4d33e8d778aa1b86197f1c54ff8e4e61a2ebf53/op_crates/fetch/26_fetch.js#L406-L491
Also I checked working of append function in this and its working fine...

[lowlighter]

Ok I think I figured out.

So like @ariain said, the error comes from op_crates/fetch/26_fetch.js

---

The test case consists of passing a FormData instance as input of Response instance.

The Response class inherits from Body, which defines the body.formData() we're calling.

Since we passed "application/x-www-form-urlencoded" as header, we get in this case :
https://github.com/denoland/deno/blob/c4d33e8d778aa1b86197f1c54ff8e4e61a2ebf53/op_crates/fetch/26_fetch.js#L823-L828

So the body.text() method is called :
https://github.com/denoland/deno/blob/c4d33e8d778aa1b86197f1c54ff8e4e61a2ebf53/op_crates/fetch/26_fetch.js#L853-L861

Since we passed a FormData instance, the body.arrayBuffer() method get called :
https://github.com/denoland/deno/blob/c4d33e8d778aa1b86197f1c54ff8e4e61a2ebf53/op_crates/fetch/26_fetch.js#L868-L873

Since FormData is not a ReadableStream, we resolve it as bodyToArrayBuffer(this._bodySource).

So we get in bodyToArrayBuffer() :
https://github.com/denoland/deno/blob/c4d33e8d778aa1b86197f1c54ff8e4e61a2ebf53/op_crates/fetch/26_fetch.js#L729-L753

Since we passed a FormData instance, the following part is executed :
https://github.com/denoland/deno/blob/c4d33e8d778aa1b86197f1c54ff8e4e61a2ebf53/op_crates/fetch/26_fetch.js#L741-L746

So basically, we're encoding bodySource.toString().

Made a little test :

const input = new FormData();
input.append("hello", "world");
console.log(input.toString()) // [object FormData]

So not sure if it's intended or not, but the toString() method of bodySource is returning [object FormData] but it's pretty clear that body.formData() expected it to be in the format key0=value0&key1=value1&... :

https://github.com/denoland/deno/blob/c4d33e8d778aa1b86197f1c54ff8e4e61a2ebf53/op_crates/fetch/26_fetch.js#L828-L841

I think this is the whole stacktrace, so the fix should be somewhere in it. @lucacasonato could you give use more hindsight about this ?

[lucacasonato]

Great analysis! I think instead of taking the branch that leads to .toString in bodyToArrayBuffer, we should be doing the same thing that happens in the fetch implementation here: https://github.com/denoland/deno/blob/c4d33e8d778aa1b86197f1c54ff8e4e61a2ebf53/op_crates/fetch/26_fetch.js#L1236-L1246

Its unfortunate that this is going to duplicated code, but we will address that soon when we refactor fetch to enable streaming uploads.

[lowlighter]

So looking more into it I don't think it's possible to update directly the behaviour of bodyToArrayBuffer since we need the response header to get the boundary, but putting the handler in Body.arrayBuffer() seems a good alternative until the refactor :

arrayBuffer() {
  if (this._bodySource instanceof ReadableStream) {
    return bufferFromStream(this._bodySource.getReader(), this.#size);
  }
+  if (this._bodySource instanceof FormData) {
+    const params = getHeaderValueParams(this.#contentType);
+    const boundary = params.get("boundary");
+    const multipartBuilder = new MultipartBuilder(this._bodySource, boundary);
+    return Promise.resolve(multipartBuilder.getBody());
+ }
  return Promise.resolve(bodyToArrayBuffer(this._bodySource));
}

I don't know if we moved forward or backwards, but now #8063 is failing with :

-         "------------he0nr25x5cuid9mcuda5hf9icikmwi1f\r\nContent-Disposition: form-data; name",
-         '"hello"\r\n\r\nworld\r\n------------he0nr25x5cuid9mcuda5hf9icikmwi1f--',
+         "hello",
+         "world",

We can see the body content, but it still doesn't format as we would like.