Incompatibility with Express 5 / Error when parsing array URL query parameters (like open_revs)

[pterrien]

Hi !

Similarly to what I did on the pouchdb-express-router repo (link to issue), here is an issue about the integration of express-pouchdb with Express 5. Sorry for the lazy copy-paste from there ;)
Although Express 5 is still alpha, you might want to consider it, as this is problem will definitely come, sooner or later.

Here are my investigations, and 2 proposed workarounds / solutions. Please note I made my tests with express-pouchdb-router, and not with express_pouchdb.

Description of the problem

Starting from Express 5, req.query (corresponding to the URL query string) is now read-only (a getter, technically): see this link for more info.

Regarding express-pouchdb, the impact is that, basically, the following Express middleware in packages/node_modules/express-pouchdb/lib/index.js (line 165) now becomes not so useful:

  app.use(function (req, res, next) {
    var prop;

    // Normalize query string parameters for direct passing
    // into PouchDB queries.
    for (prop in req.query) {
      if (Object.prototype.hasOwnProperty.call(req.query, prop)) {
        try {
          req.query[prop] = JSON.parse(req.query[prop]); // <- THIS IS NOW USELESS!
        } catch (e) {}
      }
    }

    // Provide the request access to the current PouchDB object.
    if (!currentPouchDB) {
      var msg = "express-pouchdb needs a PouchDB object to route a request!";
      throw new Error(msg);
    }
    req.PouchDB = currentPouchDB;

    next();
  });

=> req.query won't get modified anymore, whatever you do here...

The issue is linked to the fact that pouchdb-express-router passes the URL query parameters directly to PouchDB functions (e.g. db.get(...)), and those won't get transformed anymore.
It should still work in most of the requests, when params are strings, but it will fail when an array is passed, like on document, with the open_revs param.

Solutions / Workarounds

Solution 1: tweak the Express query parser

Starting from Express 5, the only way to modify req.query is from the query parser, called before any of the routers (this link for more info).
Instead of using the standard query parser, we can define our own, still based on qs, just to add an additional JSON parse step:

  const qs = require('qs')
  app.set('query parser', function (str) {
    const query = qs.parse(str) // call the standard parser
    Object.keys(query).map(key => {
      try {
        query[key] = JSON.parse(query[key])
      } catch (e) {} // silently ignore
    })
    return query
  })

Important note: this is linked to Express app, not router, but it could be implemented in express-pouchdb as it controls a full Express app, not just a router
I don't find this approach neither efficient nor elegant, but it works, and could be an acceptable workaround for some people I guess.

Pros:

• nearly nothing to modify in this library, except adding the code above before using the routers.

Cons:

• we mainly break the segregation of concerns, by modifying the app behavior, because of this issue:
  • the query parser should be used to parse HTTP queries, not to implement some kind of application-related logic
  • what is related to the router should stay at the router level

Solution 2: do no use req.query anymore, put PouchDB stuff in res.locals instead

The Express-way of storing user-defined data is to use res.locals.
So it's quite simple: we could modify the middleware, to store JSON parsed values in res.locals.query instead of req.query (or res.locals.pouchdb.query, or anything you'd prefer).
And then modify all the route functions, to get data from res.locals.query instead of req.query.

The middleware could be as simple as:

  app.use(function (req, res, next) {
    if (!res.locals.query) {
      res.locals.query = {}
    }
    var prop;

    // Normalize query string parameters for direct passing
    // into PouchDB queries.
    for (prop in req.query) {
      if (Object.prototype.hasOwnProperty.call(req.query, prop)) {
        try {
          res.locals.query[prop] = JSON.parse(req.query[prop]);
        } catch (e) {
          res.locals.query[prop] = req.query[prop]
        }
      }
    }

    // Provide the request access to the current PouchDB object.
    if (!currentPouchDB) {
      var msg = "express-pouchdb needs a PouchDB object to route a request!";
      throw new Error(msg);
    }
    req.PouchDB = currentPouchDB;

    next();
  });

Note: As the same time, you could also copy currentPouchDB to res.locals.PouchDBinstead of req.PouchDB...

Pros:

• cope with Express best practices
• keeping the application logic in a middleware would make easier to extend in the future
• we could use res.locals to store additional stuff, like current PouchDB object, user context or so

Cons:

• some adjustments on source code, but they should be quite limited after all

Conclusion

I hope you will find this issue interesting, and maybe it will bring some help to people struggling with Express 5 the way I did.

As you may have anticipated, I would certainly go with solution 2, but I'll let you decide which one you prefer, of course.
I may not be able to provide a full tested PR, as I don't use this library, but I will do my best to help if needed.

Many thanks for your work on PouchDB, I find this solution really interesting I must say!

Hope it will help,
Keep up the good work guys!

Cheers,

Pascal.