req.body gets populated to empty object even when it shouldn't

[mkartashov]

Problem

https://github.com/expressjs/body-parser/blob/master/lib/types/json.js#L99-L121

(seems to echo #146 ?)

Expected behaviour

If the body was not parsed by some other parser before bodyParser.json, and if the current request is NOT to be parsed with current parser, I expect the req.body to be undefined.

Current behaviour

The shouldParse check happens after the body has been populated with an empty object, thus polluting the req.

My usecase

Currently I am using express as a backend proxy for my react application.

The vast majority of my requests are fetch GET with missing content-type and with accept: */*. There are some user-profile requests (GET /user, for example, that expects json in return).

But two of my requests are very different. One is POST /save-something, which sends over content-type: application/json, and the other one is POST /upload-a-file-to-another-system with content-type: application/binary and content-transfer-encoding: binary.

The server serving as a proxy SHOULD NOT look into the request to understand how to parse/reparse it. So, my expected behaviour would be for all requests that were not parsed by JSON to leave the req.body = undefined, so that I can make a general check on req.is("json") or req.is("binary"), should I need some custom behaviour.

Example

I decided I could write up a simple script to illustrate the point, if you wish. Mostly it contains logs on the console :D

package.json

{
  "name": "express-example",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "start": "node index.js"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "express": "^4.16.4"
  }
}

index.js

const http = require("http");
const express = require("express");

const PORT = Number(process.env.PORT) || 8082;
const DELAY = Number(process.env.DELAY) || 1000;

const app = express();

function shouldParse(req){
  console.log("shouldParse invoked. Before parsing: body: ", req.body);
  return req.get("content-type") === "application/json";
}

function call(method, path, body, isJson, expectedBody){
  const req = http.request({
    host: "localhost",
    port: PORT,
    method: method,
    path: path,
    headers: {
      "content-type": isJson ? "application/json" : "random/content",
      "expected-body": expectedBody,
    }
  }, (res) => {
    var data = "";
    res.on("data", (ch) => {data += ch});
    res.on("end", () => {});
  });

  return () => {
    if(body){
      req.write(body);
    }
    req.end();
  };
}

app.use(express.json({type: shouldParse }));

app.use((req, res) => {
  console.log(req.method + " " + req.path + ", content: " + req.headers["content-type"]);
  console.log("body: " + JSON.stringify(req.body));
  console.log("expected body: " + req.headers["expected-body"]);
  console.log("");
});


const server = http.createServer(app);
const jsonBody = '{"egg": ["bacon", "spam"]}';
const calls = [
  call("GET", "/no-body", undefined, true, "undefined"),
  call("GET", "/no-body", undefined, false, "undefined"),
  call("POST", "/no-body", undefined, true, "undefined"),
  call("POST", "/no-body", undefined, false, "undefined",),
  call("POST", "/text-body", "egg, bacon, spam", false, "undefined"),
  call("POST", "/json-body", jsonBody, true, jsonBody),
  () => {
    server.close();
    console.log("done.");
    process.exit(0);
  }
];

var counter = 1;
calls.forEach((func) => {
  setTimeout(func, counter * DELAY);
  counter++;
});


server.listen(PORT, () => {
  console.log("listening on " + PORT + "...\n");
});

Output

$ npm install && npm start
npm WARN [email protected] No description
npm WARN [email protected] No repository field.

added 51 packages in 1.384s

> [email protected] start /home/mkartashov/express-example
> node index.js

listening on 8082...

GET /no-body, content: application/json
body: {}
expected body: undefined

GET /no-body, content: random/content
body: {}
expected body: undefined

shouldParse invoked. Before parsing: body:  {}
POST /no-body, content: application/json
body: {}
expected body: undefined

shouldParse invoked. Before parsing: body:  {}
POST /no-body, content: random/content
body: {}
expected body: undefined

shouldParse invoked. Before parsing: body:  {}
POST /text-body, content: random/content
body: {}
expected body: undefined

shouldParse invoked. Before parsing: body:  {}
POST /json-body, content: application/json
body: {"egg":["bacon","spam"]}
expected body: {"egg": ["bacon", "spam"]}

done.

[dougwilson]

Thank you for the detailed report! I agree this should change, which is why it has on the 2.0 branch.

Duplicate of #123

That issue has a comment of how to workaround the 1.0 behavior.

[mkartashov]

@dougwilson I saw mentions of the branch 2.0, but somehow it didn't cross my mind that it was the body-parser repo (whereas the express itself is v4 already 😆 ) ah well, my bad.

Thank you for work-around.

[dougwilson]

No problem. In fact, I will plan to work to rebase that branch today and release it has a rc or something to npm to get it rolling as a real release 👍