Query string parsing is potentially misleading

[alxndrsn]

From the README:

Mock 'http' objects for testing Express...

Query string parsing was added in #30, but the results are potentially misleading.

node-mocks-http uses node's built-in querystring to parse query strings at

node-mocks-http/lib/mockRequest.js

Line 89 in dd1fb05

mockRequest.query = querystring.parse(mockRequest.url.split('?')[1]);

ExpressJS's default query string parsing ('extended' mode) does not use querystring. See query parser at https://expressjs.com/en/api.html#app.set:

The extended query parser is based on qs.

This may lead to misleading results for tests written for an ExpressJS application with default query parser configuration which use node-mocks-http's implicit query string parsing (as opposed to the query option in createRequest()).

Example

Test code

const { assert } = require('chai');
const express = require('express');
const nodeMocksHttp = require('node-mocks-http');

describe('query string parsing', () => {
  [
    'a=1',
    'a=1&b=2',
    'a=1&a=2',
    'a[]=1',
    'a[b]=1',
  ].forEach((str, idx) => {
    it(`should not produce different result for example #${idx}`, () => {
      // given
      const expressQueryStringParser = express().get('query parser fn');

      // when
      const parsedByExpress = expressQueryStringParser(str);
      const parsedByNodeMocksHttp = nodeMocksHttp.createRequest({ url:'?'+str }).query;

      // then
      assert.deepEqual(parsedByNodeMocksHttp, parsedByExpress);
    }); 
  }); 
});

Output

  query string parsing
    ✔ should not produce different result for example #0
    ✔ should not produce different result for example #1
    ✔ should not produce different result for example #2
    1) should not produce different result for example #3
    2) should not produce different result for example #4


  3 passing (15ms)
  2 failing

  1) query string parsing
       should not produce different result for example #3:

      AssertionError: expected { 'a[]': '1', …(1) } to deeply equal { a: [ '1' ] }
      + expected - actual

       {
      -  "a[]": "1"
      +  "a": [
      +    "1"
      +  ]
       }
      
      at Context.<anonymous> (test.js:22:11)
      at process.processImmediate (node:internal/timers:478:21)

  2) query string parsing
       should not produce different result for example #4:

      AssertionError: expected { 'a[b]': '1', …(1) } to deeply equal { a: { b: '1' } }
      + expected - actual

       {
      -  "a[b]": "1"
      +  "a": {
      +    "b": "1"
      +  }
       }
      
      at Context.<anonymous> (test.js:22:11)
      at process.processImmediate (node:internal/timers:478:21)

[eugef]

Hi @alxndrsn thanks for reporting this issue.

Indeed, Express.js by default uses qs instead of querystring.
I agree that for better compatibility node-mocks-http should also use qs.

Since this project relies a lot on contributors, we'd really appreciate it if you could come up with a fix. Thanks a bunch!

[alxndrsn]

I agree that for better compatibility node-mocks-http should also use qs.

That's not what I'm suggesting - I think doing any parsing of query strings is going to give misleading results for some users in some situations.

Here are the parsers used by the frameworks listed in https://github.com/eugef/node-mocks-http/blob/master/README.md:

library	parser	docs
expressjs ("extended"/default)	qs	https://expressjs.com/en/api.html#app.set
expressjs ("simple")	querystring	https://expressjs.com/en/api.html#app.set
NextJS	URLSearchParams	https://nextjs.org/docs/app/api-reference/functions/use-search-params
Koa (default)	querystring	https://github.com/koajs/koa/blob/master/lib/request.js#L13

Safe approaches might be to:

1. deprecate query string parsing in this library and log a warning
2. remove query string parsing from this library
3. require explicit configuration of a particular parser
4. allow some way to derive which parser to use from a user-supplied application instance

Option 2 would seem the simplest & safest, with obvious inconvenience for anyone using this feature.

[github-actions]

Stale issue message

[eugef]

To keep library backwards compatible we will continue using built-in querystring but add a configuration option to allow using other parsing libs

[github-actions]

Stale issue message