pouchdb-server crashes due to request containing invalid rev
Issue
When POSTing to _bulk_get, an invalid rev causes PouchDB Server to crash.
Info
- Environment: Node.js v8.11.1
- Platform: No browser involved. Both client and server running in Node.js v8.11.1.
- Adapter: LevelDB
- Server: PouchDB Server
Reproduce
All of the code needed to reproduce this issue can be found in this repo. I did not use glitch.me in order to demonstrate the server crash.
-
Start PouchDB Server.
pouchdb-server --port 3030 --dir ./dbsIn the sample repo, this can be done with
npm run start-server. -
Run the code found in this file
This code sends a POST request to the
_bulk_getendpoint. The code demonstrates that whenrevis set to a valid value, the request succeeds. But, whenrevis set to an invalid value, it causes the PouchDB Server to crash. -
Observe that the second POST request causes the PouchDB Server to crash with the following error and stack trace:
Error: Unable to resolve latest revision for id person_1, rev 1-ecdf613faa5b4ba68c610c4d54dc0bca at Object.latest (/Users/kwpeters/dev/pouchdb_invalid_rev/node_modules/pouchdb-merge/lib/index.js:462:9) at /Users/kwpeters/dev/pouchdb_invalid_rev/node_modules/pouchdb-adapter-leveldb-core/lib/index.js:445:42 at /Users/kwpeters/dev/pouchdb_invalid_rev/node_modules/sublevel-pouchdb/lib/index.js:245:9 at /Users/kwpeters/dev/pouchdb_invalid_rev/node_modules/sublevel-pouchdb/lib/index.js:76:13Granted, the request is invalid, but I would expect the server to return an appropriate error rather than crash.
This is a duplicate posting of an issue submitted in the pouchdb repo. It is probably more appropriate here.
Note that if you request a "bad" revision (i.e. a string that does not match the rev pattern - "sdfasdfasdf") you receive a "no revision found" error. This only happens if you request a non-existent revision that matches the correct pattern.
More information: this does not happen for any _bulk_get request for a non-existent revision , it only happens if you specify /_bulk_get?latest=true and request a non-existent revision.
After much investigation and analysis, I have been unable to find a relaiable fix for this in the express-poucdbcodebase. Due to the nested nature of all the async TaskQueues used in the adapters, the exception thrown by PouchDB does not bubble up to a single point that could be caught.
It would have seemed that we could have caught the exception by putting a try {...} catch on the
call to bulkGet (code fragement below), but the exception takes down the server before this can be caught.
req.db.bulkGet(opts, function (err, response) {
if (err) {
return utils.sendError(res, err);
}
...
utils.sendJSON(res, 200, { results: httpResults });
});
Example code performing all a variety of queries (and a summary of the results and differences) were added to @kwpeters example in this fork of his original repro case.
I added express-pouchdb unit tests in a fork of the pouchdb-server repo (which I can submit an MR for) illustrating this as well.
So, all this being said, my proposed fix for this is in PouchDB itself, which means this is really a fix for
PouchDB Issue 7756. (can this be re-opened?). The code works fine for http Pouch requests, but fails for local (Node.js -> LeveDB) databases. This is because the implementation in the Level adapter is a bit different. The code in this commit fixes this issue, and has unit tests covering the scenerio (which fail before the fix to the adapter), and succeeds after the fix. All other unit tests pass with this fix as well.
Note in the unit test expectations, the results from the http and local adapters is different for
bad revisions and also for these non-existing revisions. These responses should be the same I imagine, but that is a much larger issue to fix, and would have backwards compatibility issues (people with code expecting the bad behavior).
Does this seems like the right fix? Thoughts? Suggestions? Did I miss anything?