Request options.json and options.form inconsistencies

[daniel-carvalho]

The Problem

There's two ways to send JSON, as stated in the docs (readme):

1. .json = object
2. .json = true AND .body = object

In the json function, when using the "pair method" (.json = true AND body = object), there is a check to see if there isn't already a form-urlencoded content-type header already set before trying to send JSON, if there is, it actually treats the request as a .form request, and encodes body right in the JSON function, without the various checks the .form function provides to ensure the body is encoded correctly (for example, in cases where it's an object).

However, with the .json = object method, it doesn't care about existing headers that may or may not be there, and goes ahead and stringify's the object to send JSON regardless. One flaw with this, is that if there IS a form-urlencoded content-type header present, this is maintained despite that it's sending a stringify'ied object.

There's actually a heap of issues that come up because of this that are really hard to articulate without writing a book, because what we have now is that neither .json or .form behave in a consistent fashion. I can see why the JSON stuff was done this way, but when you're a user using this library, chasing some of these seemingly random variances in output is really tricky and time consuming.

The actual root problem here that I originally wanted to fix, which I think causes all this weirdness, is that .json is used both as a flag to 'Accept: application/json', and as a mechanism to actually send a JSON payload where it ultimately becomes 'Content-Type: application/json'. However the two aren't mutually inclusive and this is where the issues come in. This seems like a design issue. These potentially should be separated into two different flags, or perhaps the convenience functionality should be removed, or people should be unable to add what fits the requirements for both a form and json request simultaneously.

This problem is compounded by the various combinations you can have of options.form, options.json, options.body, and options.headers.

Potential Solution and Step in the Right Direction

Trying to fix what I deem as the root issue above, seemed to be a tangled mess that wasn't going to be very backwards compatible, so in the pull request to follow, I instead opted to achieve:

1. If .form and .json functions worry about content-type headers, they should worry about them in all use-cases.

2. Because request worries about the content-type headers, looking at the options flags .form and .json isn't enough to determine if the request should be a form or JSON request. Since headers can be manually injected into the options parameter.

3. Unify the form processing logic, don't have the .json function do what work that the .form function should have already performed, since it runs first.

4. Ultimate goal is to have (.json = object) == (.json = true AND body = object) be equivalent, and on the flipside form requests with a body be equivalent.

5. By making the functionality consistent and more readable (hopefully), the consistency will help users will not fall into the trap of trying various combinations and seeing differing results while debugging, and contributors maintaining request will easily grasp the intentions of the software.

Example to Reproduce

var requestOptions = [
  // Test 1 (happy case)
  // 'content-type': 'application/x-www-form-urlencoded'
  // request.body: 'some=url&encoded=data'
  {
    url: 'http://127.0.0.1:1337',
    headers: {'content-type': 'application/x-www-form-urlencoded; charset=UTF-8'},
    form: {some: 'url', encoded: 'data'},
    json: true
  },
  // Test 2 (happy case)
  // 'content-type': 'application/x-www-form-urlencoded; charset=UTF-8'
  // request.body: 'some=url&encoded=data'
  {
    url: 'http://127.0.0.1:1337',
    headers: {'content-type': 'application/x-www-form-urlencoded; charset=UTF-8'},
    body: 'some=url&encoded=data',
    json: true
  },
  // Test 3 (happy case)
  // 'content-type': 'application/json'
  // request.body: '{"foo":"bar"}'
  {
    url: 'http://127.0.0.1:1337',
    json: {foo: 'bar'}
  },
  // Test 4 (happy case)
  // 'content-type': 'application/json'
  // request.body: '{"foo":"bar"}'
  {
    url: 'http://127.0.0.1:1337',
    body: {foo: 'bar'},
    json: true
  },
  // Test 5 (issue case)
  // 'content-type': 'application/x-www-form-urlencoded; charset=UTF-8'
  // request.body: '{"foo":"bar"}'
  {
    url: 'http://127.0.0.1:1337',
    headers: {'content-type': 'application/x-www-form-urlencoded; charset=UTF-8'},
    body: 'some=url&encoded=data',
    json: {foo: 'bar'}
  },
  // Test 6 (issue case)
  // TypeError: str.replace is not a function
  {
    url: 'http://127.0.0.1:1337',
    headers: {'content-type': 'application/x-www-form-urlencoded; charset=UTF-8'},
    body: {foo: 'bar'},
    json: true
  }
];

requestOptions.forEach(function(options) {
  try {
    request.post(options);
  } catch(e) {
    console.log(e);
  }
});

Related Issues

#1759 - This is a related issue that was never completely addressed, the change was quickly reverted (it was a pretty blunt change).

Your Environment

software	version
request	v2.58.0 and v2.83.0
node	v6.9.2
npm	3.10.9
Operating System	Mac OS X 10.11.6

[eilinora]

➕ this highly, when starting to use this library i found this inconsistency HIGHLY confusing. And almost switched to node-fetch

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.