dominictarr / JSONStream

rawStream.pipe(JSONStream.parse()).pipe(streamOfObjects)
Other
1.91k stars 165 forks source link

Solitaire open square bracket does not generate error. #121

Open SirWumpus opened 7 years ago

SirWumpus commented 7 years ago

Opening the outer most JSON array and failing to terminate it should generate a parsing error and non-zero exit code. Instead its assuming the opening square bracket indicates an empty array.

$ echo '[ ' | JSONStream 'true' ; echo $?
[]
0

Similarly open square, open curly does not generate an error:

$ echo '[ {' | JSONStream 'true' ; echo $?
[]
0

While starting a key with an open double-quote, will throw an error:

echo '[ { "' | ./index.js 'true' ; echo $?
stream.js:74
      throw er; // Unhandled stream error in pipe.
      ^

Error: Invalid JSON (Unexpected "\n" at position 5 in state STOP)
    at Parser.proto.charError (/home/achowe/git/cdr/node_modules/jsonparse/jsonparse.js:89:16)
    at Parser.proto.write (/home/achowe/git/cdr/node_modules/jsonparse/jsonparse.js:198:23)
    at Stream.<anonymous> (/home/achowe/git/cdr/node_modules/JSONStream/index.js:23:12)
    at Stream.stream.write (/home/achowe/git/cdr/node_modules/through/index.js:26:11)
    at Socket.ondata (_stream_readable.js:555:20)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:176:18)
    at Socket.Readable.push (_stream_readable.js:134:10)
    at Pipe.onread (net.js:548:20)
1

The knock on effect of this is for Node CLI tools, that use JSONStream, used in a shell pipe line will incorrectly report success for an erroneous parse state ('[' and '[ {') .

Real life example:

Using JSONStream in an amqppub CLI that consumes JSON input and publishes to a AMQP exchange fails to return a non-zero exit code, because no error is raised by JSONStream for being in an incomplete parse state. When used in a shell pipeline, the exit code of the end of the pipe signals whether the pipeline terminated normally or not.

So if a tool to decode CDR files to JSON and publish them has an error on the first CDR file, first CDR record, after it has sent the opening square bracket '[' to amqppub, breaks the pipeline with the stream in an invalid parse state, yet amqppub exits with zero (0), because JSONStream didn't raise a parse error.

$ cdrdump data/cdr-truncated ; echo $?
[
throw "data/cdr-truncated": {file_size_data_mismatch,200,225}
1
$ cdrdump truncated-cdr-file | amqppub -j ; echo $?
throw "truncated-cdr-file": {file_size_data_mismatch,200,225}
0
dominictarr commented 7 years ago

you are correct. what should happen is when the input ends, it should check if the parser is in a valid end state, and if not, emit an error.

Since it's been like this a long time, there maybe cases where this is what you want (for example, streaming from a live connection, not a file, and the connection drops) So I'd publish this as a breaking change.

If you make a PR that adds a option (enabled by default) for an error in this case, then I would gladly merge it!

doowb commented 7 years ago

I think #117 might fix this. Would you try that code to see if it works?

That PR could be updated to enable the feature behind an option. Also, I think that failing test is just because it's supposed to error but the error isn't being caught and tested in the test.

dominictarr commented 7 years ago

that PR does need a small update to give correct stream behaviour. thanks @doowb