Support for JSON merge-patch

[t3hmrman]

It looks like super agent doesn't support JSON merge-patches with Content-Type: application/merge-patch+json. When I try to send(...) a JSON value it comes out on the other side empty:

    const req = request(app)
        .patch(url)
        .set("Content-Type", "application/merge-patch+json")
        .send(entity);
        .expect('Content-Type', /json/)
        .expect(200);

To reiterate, on the server side, req.body is actually empty.

I haven't looked at the code but if I had to guess I'd gues sthat send(..) is doing some conversion stuff depending on what Content-Type is set to, which makes sense, but it would be nice in general if I could turn it off, or specify what the body is concretely to avoid this problem.

[kornelski]

That .expect() looks like supertest, which is a different project. Can you reproduce the bug with pure superagent?

[t3hmrman]

Hey @kornelski you're right it is supertest -- I thought supertest was a relatively thin layer over superagent which is why I posted it here, I will test it with superagent alone and let you know within the hour.

[EDIT] - Just realized superagent is actually not at all a testing library... It's a just the request library bit, my apologies!

[t3hmrman]

After the discussion in supertest it looks like this is a superagent problem -- Once I can produce a testcase I'll re-open

[t3hmrman]

Hey I finally got around to making a testcase for this issue.

Here is the highlight:

const request = require("superagent");
const express = require("express");
const bodyParser = require("body-parser");

// Create trivial express server to test with
const app = express();
app.use(bodyParser.json());
app.patch("/json-merge-patch", (req, res) => res.json({requestBody: req.body}));
app.post("/json-merge-patch", (req, res) => res.json({requestBody: req.body}));

if (require.main === module) {
  app
    .listen(3000, () => {
      const runPost = () => request
            .post("localhost:3000/json-merge-patch")
            .send({data: "some data"}) // sends a JSON post body
            .set("Content-Type", "application/json")
            .set("Accept", "json")
            .then(res => res.body);

      const runPatch = () => request
            .patch("localhost:3000/json-merge-patch")
            .send({data: "some data"}) // sends a JSON post body
            .set("Content-Type", "application/merge-patch+json")
            .set("Accept", "json")
            .then(res => res.body);

      Promise.all([
        runPost(),
        runPatch(),
      ])
            .then(([postBody, patchBody]) => {
              console.log("[POST] returned JSON?", postBody);
              console.log("[PATCH] returned JSON?", patchBody);

              process.exit(JSON.stringify(postBody) === JSON.stringify(patchBody) ? 0 : 1);
            });
    });
}

And when I run it:

$ npm run test

> superagent-json-merge-patch@1.0.0 test /home/mrman/Projects/open-source-contrib/superagent-json-merge-patch-testcase
> node index.js

superagent: Enable experimental feature http2
[POST] returned JSON? { requestBody: { data: 'some data' } }
[PATCH] returned JSON? { requestBody: {} }

[deiga]

@t3hmrman I started a PR to address this, but I can't get a test to reproduce the issue. Could you check if the test I wrote is wrong somehow?
#1441

[t3hmrman]

Hey @deiga thanks for the hard work! I'm looking at the patch now, I'm going to go back and ensure that my test still works with the version you're using.

[EDIT] - looks like my test is still failing on 4.1.0-beta.1, I'm going to take a closer look at the code for both tests

[EDIT2] - The only difference I can see is that you don't have the content type checks? My request looks like this:

      const runPost = () => request
            .post("localhost:3000/json-merge-patch")
            .send({data: "some data"}) // sends a JSON post body
            .set("Content-Type", "application/json")
            .set("Accept", "json")
            .then(res => res.body);

For a second I thought the Accept header that I was specifying was wrong but I changed that to application/json and the test still fails.

[deiga]

@t3hmrman Since this seems to only happen when using app.patch(..) as app.all(..) does work.
Can we somehow verify that this actually happens in Superagent and not Express?

[t3hmrman]

@deiga I just tested with curl and I think you are right -- I get different results if I completely ignore the Accept header -- just specifying json-merge-patch+json as the Content-Type is enough to trigger the misbehavior:

$ curl -X PATCH -H 'Content-Type: application/merge-patch+json' localhost:3000/json-merge-patch --data '{"data":"some data"}' | jq                                                                                                                                
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100    38  100    18  100    20   9000  10000 --:--:-- --:--:-- --:--:-- 19000
{
  "requestBody": {}
}
$ curl -X PATCH -H 'Content-Type: application/json' localhost:3000/json-merge-patch --data '{"data":"some data"}' | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100    56  100    36  100    20  36000  20000 --:--:-- --:--:-- --:--:-- 56000
{
  "requestBody": {
    "data": "some data"
  }
}

I think you can definitely close this, this must be an express bug. I thought I tested curl/sending the request myself before this but clearly I didn't, my apologies.

[t3hmrman]

Just found out it's a bug feature in body-parser from a ticket in 2015.

My apologies @deiga thanks for looking at this