fix body-parser middleware stream issue

[ghost]

• Adds an check to see the body object is empty

#102

[ghost]

Hopefully someone will get back to me and let me know of a better fix. 👍

[h2non]

Hey! This looks good, but I'm wondering in which scenarios body will be a different data type of string or Buffer.

Looks like the fix at this level is just trying to hide another issue somewhere else. A body which is different from those data types should never reach that part of the forward middleware logic.

I'm curious why the body is an empty object.
Since the fix looks like a self-protectionist change, I'm open to merge it, but just wanna understand the scenario better.

[ghost]

Thank you for following up on this! It's most appreciated, as we would need to at least have this fix in order to deliver a solution we're presently working on.

I'm not sure it would ever be an instanceof Buffer either, but from what I've read, only strings, Buffers, or a null may be pushed onto a Readable stream:

readable.push(chunk[, encoding])
chunk | | Chunk of data to push into the read queue
encoding Encoding of String chunks. Must be a valid Buffer encoding, such as 'utf8' or 'ascii'
Returns true if additional chunks of data may continued to be pushed; false otherwise.
When chunk is a Buffer or string, the chunk of data will be added to the internal queue for users of the stream to consume. Passing chunk as null signals the end of the stream (EOF), after which no more data can be written.
https://nodejs.org/api/stream.html#stream_readable_push_chunk_encoding

In my particular scenario, since I am using some additional body-parser middleware, req.body is set to be an empty JS object:

The bodyParser object exposes various factories to create middlewares. All middlewares will populate the req.bodyproperty with the parsed body, or an empty object ({}) if there was no body to parse (or an error was returned).
https://github.com/expressjs/body-parser

We're using this middleware because it's needed by passport-saml, which we're using for SAML authentication to protect our API.

The commit makes sense to me in that yes, it does protect Rocky from blowing up in this scenario, but I'm still wondering if there's maybe something that should be added.

[h2non]

That makes sense. I don't have time to dig into details and come up with a stronger solution, so I'm going to merge this and release a new version.

[ghost]

Thanks for doing this. A great help for our project! I'll go ahead and close #102

[h2non]

rocky@0.4.13 is now available in npm registry.

[ghost]

Confirmed:

$ yarn upgrade rocky
yarn upgrade v0.20.3
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
success Saved lockfile.
success Saved 1 new dependency.
└─ rocky@0.4.13
✨  Done in 14.77s.

Thanks, @h2non!