apache / couchdb-nano

Nano: The official Apache CouchDB library for Node.js
https://www.npmjs.com/package/nano
Apache License 2.0
651 stars 165 forks source link

Higher-level stream implementations can create errors in `db.attachments.insert(...)` #274

Closed sploders101 closed 3 years ago

sploders101 commented 3 years ago

Expected Behavior

When passing a stream as an attachment to db.attachments.insert(...), nano should stream the contents to the database regardless of the stream's implementation details.

Current Behavior

Nano uses a "poor man's deep clone" to create a copy of the request data (added in #256) before scrubbing the username and password for logging. When using a custom stream, this can create an issue where you are serializing a stream, which contains lots of circular references, and cannot be done.

Possible Solution

Implement a better scrubbing strategy. Ex:

// scrub and log
const scrubbedReq = {};
Object.entries(([key, value]) => {
  if(key !== "auth") scrubbedReq[key] = value;
});
log(scrubbedReq);

You don't need to clone the entire object. Deep cloning uses a lot of computing power for something that probably won't even end up happening. If you do a shallow clone, you can then omit the entire auth property (which might be safer if there is other authentication data in it). Data won't be modified during logging, so pass-by-reference (the issue deep cloning solves) shouldn't be an issue here.

Steps to Reproduce (for bugs)

  1. Create a readable stream (ex: const myStream = fs.createReadStream(...))
  2. Create a circular reference (ex: myStream.test = myStream) (many stream implementations have these for a variety of reasons).
  3. Pass the req object into db.attachments.insert(...) as the data
  4. A circular reference error is emitted from JSON.stringify(...)

Context

I am trying to stream an upload via ExpressJS into CouchDB and need to either patch nano or use a PassThrough stream to omit circular references being passed to CouchDB. JSON.stringify() in most cases should not be used on objects that contain classes, because a great number of class implementations contain circular references.

Your Environment

glynnbird commented 3 years ago

Thanks @sploders101. You reasoning is very sensible: i've never liked JSON.parse(JSON.stringify(x)) and you're right than in this case it's cloning too much data.

glynnbird commented 3 years ago

Fixed in 9.0.4