From 471849f4b203d6e9ac741eb9d4b9c14b045ba838 Mon Sep 17 00:00:00 2001 From: Juanjo Diaz Date: Sat, 7 Jan 2023 20:37:54 +0100 Subject: [PATCH] feat: improve error messaging for invalid inputs --- dist/cdn/plainjs/Parser.js | 15 +++-- dist/cdn/plainjs/StreamParser.js | 34 ++++++++--- packages/cli/src/json2csv.ts | 2 +- packages/cli/src/utils/parseNdjson.ts | 14 +++-- packages/cli/test/CLI.js | 60 ++++++++++++++++--- packages/node/test/AsyncParser.js | 26 +++++++- packages/node/test/AsyncParserInMemory.js | 26 +++++++- packages/node/test/Transform.js | 26 +++++++- packages/plainjs/src/Parser.ts | 18 +++--- packages/plainjs/src/StreamParser.ts | 36 ++++++++--- packages/plainjs/test/Parser.js | 38 +++++++++++- packages/plainjs/test/StreamParser.js | 26 +++++++- .../fixtures/json/notAnObject.json | 1 - .../fixtures/json/notObjectArray.json | 1 + .../fixtures/json/notObjectSingleItem.json | 1 + packages/whatwg/test/AsyncParser.js | 26 +++++++- packages/whatwg/test/AsyncParserInMemory.js | 26 +++++++- packages/whatwg/test/TransformStream.js | 26 +++++++- 18 files changed, 337 insertions(+), 65 deletions(-) delete mode 100644 packages/test-helpers/fixtures/json/notAnObject.json create mode 100644 packages/test-helpers/fixtures/json/notObjectArray.json create mode 100644 packages/test-helpers/fixtures/json/notObjectSingleItem.json diff --git a/dist/cdn/plainjs/Parser.js b/dist/cdn/plainjs/Parser.js index 890173b..4ba00e6 100644 --- a/dist/cdn/plainjs/Parser.js +++ b/dist/cdn/plainjs/Parser.js @@ -37,10 +37,17 @@ var JSON2CSVParser = class extends JSON2CSVBase { */ preprocessData(data) { const processedData = Array.isArray(data) ? data : [data]; - if (!this.opts.fields && (processedData.length === 0 || typeof processedData[0] !== "object")) { - throw new Error( - 'Data should not be empty or the "fields" option should be included' - ); + if (!this.opts.fields) { + if (data === void 0 || data === null || processedData.length === 0) { + throw new Error( + 'Data should not be empty or the "fields" option should be included' + ); + } + if (typeof processedData[0] !== "object") { + throw new Error( + 'Data items should be objects or the "fields" option should be included' + ); + } } if (this.opts.transforms.length === 0) return processedData; diff --git a/dist/cdn/plainjs/StreamParser.js b/dist/cdn/plainjs/StreamParser.js index 018b31c..26fa2d0 100644 --- a/dist/cdn/plainjs/StreamParser.js +++ b/dist/cdn/plainjs/StreamParser.js @@ -19,7 +19,12 @@ var __spreadValues = (a, b) => { var __spreadProps = (a, b) => __defProps(a, __getOwnPropDescs(b)); // packages/plainjs/src/StreamParser.ts -import { Tokenizer, TokenParser, TokenType } from "https://cdn.jsdelivr.net/npm/@streamparser/json@0.0.12/dist/mjs/index.mjs"; +import { + Tokenizer, + TokenParser, + TokenType, + TokenizerError +} from "https://cdn.jsdelivr.net/npm/@streamparser/json@0.0.12/dist/mjs/index.mjs"; import JSON2CSVBase from "./BaseParser.js"; var JSON2CSVStreamParser = class extends JSON2CSVBase { constructor(opts, asyncOpts) { @@ -75,7 +80,7 @@ var JSON2CSVStreamParser = class extends JSON2CSVBase { return tokenizer; } getBinaryModeTokenizer(asyncOpts) { - const tokenizer = new Tokenizer(); + const tokenizer = new Tokenizer(asyncOpts); tokenizer.onToken = ({ token, value }) => { if (token === TokenType.LEFT_BRACKET) { this.tokenParser = new TokenParser({ @@ -85,13 +90,19 @@ var JSON2CSVStreamParser = class extends JSON2CSVBase { } else if (token === TokenType.LEFT_BRACE) { this.tokenParser = new TokenParser({ paths: ["$"], keepStack: false }); } else { - this.onError(new Error("Data should be a JSON object or array")); + this.onError( + new Error( + 'Data items should be objects or the "fields" option should be included' + ) + ); return; } this.configureCallbacks(tokenizer, this.tokenParser); this.tokenParser.write({ token, value }); }; - tokenizer.onError = () => this.onError(new Error("Data should be a JSON object or array")); + tokenizer.onError = (err) => this.onError( + err instanceof TokenizerError ? new Error("Data should be a valid JSON object or array") : err + ); tokenizer.onEnd = () => { this.pushHeaderIfNotWritten(); this.onEnd(); @@ -140,10 +151,17 @@ var JSON2CSVStreamParser = class extends JSON2CSVBase { pushLine(data) { const processedData = this.preprocessRow(data); if (!this._hasWritten) { - this.opts.fields = this.preprocessFieldsInfo( - this.opts.fields || Object.keys(processedData[0]), - this.opts.defaultValue - ); + if (!this.opts.fields) { + if (typeof processedData[0] !== "object") { + throw new Error( + 'Data items should be objects or the "fields" option should be included' + ); + } + this.opts.fields = this.preprocessFieldsInfo( + Object.keys(processedData[0]), + this.opts.defaultValue + ); + } this.pushHeader(); } processedData.forEach((row) => { diff --git a/packages/cli/src/json2csv.ts b/packages/cli/src/json2csv.ts index 7eed2a8..37230dc 100755 --- a/packages/cli/src/json2csv.ts +++ b/packages/cli/src/json2csv.ts @@ -332,7 +332,7 @@ async function processStream( processedError = new Error(`Invalid config file. (${err.message})`); } // eslint-disable-next-line no-console - console.error('test', processedError); + console.error(processedError); process.exit(1); } })(programOpts); diff --git a/packages/cli/src/utils/parseNdjson.ts b/packages/cli/src/utils/parseNdjson.ts index d16983f..74557a1 100644 --- a/packages/cli/src/utils/parseNdjson.ts +++ b/packages/cli/src/utils/parseNdjson.ts @@ -1,7 +1,11 @@ export default function parseNdJson(input: string, eol: string): Array { - return input - .split(eol) - .map((line) => line.trim()) - .filter((line) => line !== '') - .map((line) => JSON.parse(line)); + try { + return input + .split(eol) + .map((line) => line.trim()) + .filter((line) => line !== '') + .map((line) => JSON.parse(line)); + } catch (err: any) { + throw new Error("Invalid ND-JSON couldn't be parsed"); + } } diff --git a/packages/cli/test/CLI.js b/packages/cli/test/CLI.js index d8f9f17..8f7007c 100644 --- a/packages/cli/test/CLI.js +++ b/packages/cli/test/CLI.js @@ -41,19 +41,21 @@ export default function (jsonFixtures, csvFixtures) { }); testRunner.add( - 'should error on invalid ndjson input path without streaming', + 'should error if ndjson input data is empty and fields are not set', async (t) => { - const opts = - '--fields carModel,price,color,manual --ndjson --no-streaming'; + const opts = '--ndjson'; try { await execAsync( - `${cli} -i "${getFixturePath('/json2/ndjsonInvalid.json')}" ${opts}` + `${cli} -i "${getFixturePath('/json/empty.json')}" ${opts}` ); - t.fail('Exception expected.'); + t.fail('Exception expected'); } catch (err) { - t.ok(err.message.includes('Invalid input file.')); + t.equal( + err.stderr.split('\n')[2].substring(7), + 'Data should not be empty or the "fields" option should be included' + ); } } ); @@ -89,6 +91,27 @@ export default function (jsonFixtures, csvFixtures) { t.equal(csv, csvFixtures.ndjson); }); + testRunner.add( + 'should error on invalid ndjson input path without streaming', + async (t) => { + const opts = + '--fields carModel,price,color,manual --ndjson --no-streaming'; + + try { + await execAsync( + `${cli} -i "${getFixturePath('/json/ndjsonInvalid.json')}" ${opts}` + ); + + t.fail('Exception expected.'); + } catch (err) { + t.equal( + err.stderr.split('\n')[2].substring(7), + "Invalid ND-JSON couldn't be parsed" + ); + } + } + ); + testRunner.add('should error on invalid input file path', async (t) => { try { await execAsync(`${cli} -i "${getFixturePath('/json2/default.json')}"`); @@ -116,15 +139,36 @@ export default function (jsonFixtures, csvFixtures) { } ); + testRunner.add( + 'should error if input data is single item and not an object', + async (t) => { + try { + await execAsync( + `${cli} -i "${getFixturePath('/json/notObjectSingleItem.json')}"` + ); + + t.fail('Exception expected'); + } catch (err) { + t.equal( + err.stderr.split('\n')[2].substring(7), + 'Data items should be objects or the "fields" option should be included' + ); + } + } + ); + testRunner.add('should error if input data is not an object', async (t) => { try { await execAsync( - `${cli} -i "${getFixturePath('/json2/notAnObject.json')}"` + `${cli} -i "${getFixturePath('/json/notObjectArray.json')}"` ); t.fail('Exception expected.'); } catch (err) { - t.ok(err.message.includes('Invalid input file.')); + t.equal( + err.stderr.split('\n')[2].substring(7), + 'Data items should be objects or the "fields" option should be included' + ); } }); diff --git a/packages/node/test/AsyncParser.js b/packages/node/test/AsyncParser.js index 560dea6..61e4615 100644 --- a/packages/node/test/AsyncParser.js +++ b/packages/node/test/AsyncParser.js @@ -152,14 +152,34 @@ export default function (jsonFixtures, csvFixtures) { } ); + testRunner.add( + 'should error if input data is single item and not an object', + async (t) => { + try { + const parser = new Parser(); + await parseInput(parser, `"${jsonFixtures.notObjectSingleItem()}"`); + + t.fail('Exception expected'); + } catch (err) { + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); + } + } + ); + testRunner.add('should error if input data is not an object', async (t) => { try { const parser = new Parser(); - await parseInput(parser, `"${jsonFixtures.notAnObject()}"`); + await parseInput(parser, jsonFixtures.notObjectArray()); t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); } }); @@ -191,7 +211,7 @@ export default function (jsonFixtures, csvFixtures) { t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal(err.message, 'Data should be a valid JSON object or array'); } } ); diff --git a/packages/node/test/AsyncParserInMemory.js b/packages/node/test/AsyncParserInMemory.js index 7984bdd..e1d8d1a 100644 --- a/packages/node/test/AsyncParserInMemory.js +++ b/packages/node/test/AsyncParserInMemory.js @@ -152,14 +152,34 @@ export default function (jsonFixtures, csvFixtures) { } ); + testRunner.add( + 'should error if input data is single item and not an object', + async (t) => { + try { + const parser = new Parser(); + await parseInput(parser, `"${jsonFixtures.notObjectSingleItem()}"`); + + t.fail('Exception expected'); + } catch (err) { + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); + } + } + ); + testRunner.add('should error if input data is not an object', async (t) => { try { const parser = new Parser(); - await parseInput(parser, `"${jsonFixtures.notAnObject()}"`); + await parseInput(parser, jsonFixtures.notObjectArray()); t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); } }); @@ -191,7 +211,7 @@ export default function (jsonFixtures, csvFixtures) { t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal(err.message, 'Data should be a valid JSON object or array'); } } ); diff --git a/packages/node/test/Transform.js b/packages/node/test/Transform.js index 8f6a08d..9185349 100644 --- a/packages/node/test/Transform.js +++ b/packages/node/test/Transform.js @@ -147,14 +147,34 @@ export default function (jsonFixtures, csvFixtures) { } ); + testRunner.add( + 'should error if input data is single item and not an object', + async (t) => { + try { + const parser = new Parser(); + await parseInput(parser, jsonFixtures.notObjectSingleItem()); + + t.fail('Exception expected'); + } catch (err) { + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); + } + } + ); + testRunner.add('should error if input data is not an object', async (t) => { try { const parser = new Parser(); - await parseInput(parser, jsonFixtures.notAnObject()); + await parseInput(parser, jsonFixtures.notObjectArray()); t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); } }); @@ -186,7 +206,7 @@ export default function (jsonFixtures, csvFixtures) { t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal(err.message, 'Data should be a valid JSON object or array'); } } ); diff --git a/packages/plainjs/src/Parser.ts b/packages/plainjs/src/Parser.ts index 9fca0d3..259be0b 100644 --- a/packages/plainjs/src/Parser.ts +++ b/packages/plainjs/src/Parser.ts @@ -55,13 +55,17 @@ export default class JSON2CSVParser< preprocessData(data: Array | TRaw): Array { const processedData = Array.isArray(data) ? data : [data]; - if ( - !this.opts.fields && - (processedData.length === 0 || typeof processedData[0] !== 'object') - ) { - throw new Error( - 'Data should not be empty or the "fields" option should be included' - ); + if (!this.opts.fields) { + if (data === undefined || data === null || processedData.length === 0) { + throw new Error( + 'Data should not be empty or the "fields" option should be included' + ); + } + if (typeof processedData[0] !== 'object') { + throw new Error( + 'Data items should be objects or the "fields" option should be included' + ); + } } if (this.opts.transforms.length === 0) diff --git a/packages/plainjs/src/StreamParser.ts b/packages/plainjs/src/StreamParser.ts index 9136ce9..258db15 100644 --- a/packages/plainjs/src/StreamParser.ts +++ b/packages/plainjs/src/StreamParser.ts @@ -1,4 +1,9 @@ -import { Tokenizer, TokenParser, TokenType } from '@streamparser/json'; +import { + Tokenizer, + TokenParser, + TokenType, + TokenizerError, +} from '@streamparser/json'; import JSON2CSVBase, { Json2CSVBaseOptions, NormalizedJson2CSVBaseOptions, @@ -99,7 +104,11 @@ export default class JSON2CSVStreamParser< } else if (token === TokenType.LEFT_BRACE) { this.tokenParser = new TokenParser({ paths: ['$'], keepStack: false }); } else { - this.onError(new Error('Data should be a JSON object or array')); + this.onError( + new Error( + 'Data items should be objects or the "fields" option should be included' + ) + ); return; } @@ -107,8 +116,12 @@ export default class JSON2CSVStreamParser< this.tokenParser.write({ token, value }); }; - tokenizer.onError = () => - this.onError(new Error('Data should be a JSON object or array')); + tokenizer.onError = (err) => + this.onError( + err instanceof TokenizerError + ? new Error('Data should be a valid JSON object or array') + : err + ); tokenizer.onEnd = () => { this.pushHeaderIfNotWritten(); this.onEnd(); @@ -165,10 +178,17 @@ export default class JSON2CSVStreamParser< const processedData = this.preprocessRow(data); if (!this._hasWritten) { - this.opts.fields = this.preprocessFieldsInfo( - this.opts.fields || Object.keys(processedData[0]), - this.opts.defaultValue - ); + if (!this.opts.fields) { + if (typeof processedData[0] !== 'object') { + throw new Error( + 'Data items should be objects or the "fields" option should be included' + ); + } + this.opts.fields = this.preprocessFieldsInfo( + Object.keys(processedData[0]), + this.opts.defaultValue + ); + } this.pushHeader(); } diff --git a/packages/plainjs/test/Parser.js b/packages/plainjs/test/Parser.js index b3f1acb..f013a96 100644 --- a/packages/plainjs/test/Parser.js +++ b/packages/plainjs/test/Parser.js @@ -54,16 +54,50 @@ export default function (jsonFixtures, csvFixtures) { t.deepEqual(opts, {}); }); + testRunner.add( + 'should error if input data is empty and fields are not set', + async (t) => { + try { + const parser = new Parser(); + await parseInput(parser, jsonFixtures.empty() || undefined); + + t.fail('Exception expected'); + } catch (err) { + t.equal( + err.message, + 'Data should not be empty or the "fields" option should be included' + ); + } + } + ); + + testRunner.add( + 'should error if input data is single item and not an object', + async (t) => { + try { + const parser = new Parser(); + await parseInput(parser, jsonFixtures.notObjectSingleItem()); + + t.fail('Exception expected'); + } catch (err) { + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); + } + } + ); + testRunner.add('should error if input data is not an object', async (t) => { try { const parser = new Parser(); - await parseInput(parser, jsonFixtures.notAnObject()); + await parseInput(parser, jsonFixtures.notObjectArray()); t.fail('Exception expected'); } catch (err) { t.equal( err.message, - 'Data should not be empty or the "fields" option should be included' + 'Data items should be objects or the "fields" option should be included' ); } }); diff --git a/packages/plainjs/test/StreamParser.js b/packages/plainjs/test/StreamParser.js index 16e35c1..272d25f 100644 --- a/packages/plainjs/test/StreamParser.js +++ b/packages/plainjs/test/StreamParser.js @@ -118,14 +118,34 @@ export default function (jsonFixtures, csvFixtures) { } ); + testRunner.add( + 'should error if input data is single item and not an object', + async (t) => { + try { + const parser = new Parser(); + await parseInput(parser, jsonFixtures.notObjectSingleItem()); + + t.fail('Exception expected'); + } catch (err) { + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); + } + } + ); + testRunner.add('should error if input data is not an object', async (t) => { try { const parser = new Parser(); - await parseInput(parser, jsonFixtures.notAnObject()); + await parseInput(parser, jsonFixtures.notObjectArray()); t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); } }); @@ -157,7 +177,7 @@ export default function (jsonFixtures, csvFixtures) { t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal(err.message, 'Data should be a valid JSON object or array'); } } ); diff --git a/packages/test-helpers/fixtures/json/notAnObject.json b/packages/test-helpers/fixtures/json/notAnObject.json deleted file mode 100644 index 6bfd79c..0000000 --- a/packages/test-helpers/fixtures/json/notAnObject.json +++ /dev/null @@ -1 +0,0 @@ -"not an object" \ No newline at end of file diff --git a/packages/test-helpers/fixtures/json/notObjectArray.json b/packages/test-helpers/fixtures/json/notObjectArray.json new file mode 100644 index 0000000..17ac3ef --- /dev/null +++ b/packages/test-helpers/fixtures/json/notObjectArray.json @@ -0,0 +1 @@ +["not an object", "neither an object", "still not an object"] \ No newline at end of file diff --git a/packages/test-helpers/fixtures/json/notObjectSingleItem.json b/packages/test-helpers/fixtures/json/notObjectSingleItem.json new file mode 100644 index 0000000..f32a580 --- /dev/null +++ b/packages/test-helpers/fixtures/json/notObjectSingleItem.json @@ -0,0 +1 @@ +true \ No newline at end of file diff --git a/packages/whatwg/test/AsyncParser.js b/packages/whatwg/test/AsyncParser.js index 459c915..e70e465 100644 --- a/packages/whatwg/test/AsyncParser.js +++ b/packages/whatwg/test/AsyncParser.js @@ -142,14 +142,34 @@ export default function (jsonFixtures, csvFixtures) { } ); + testRunner.add( + 'should error if input data is single item and not an object', + async (t) => { + try { + const parser = new Parser(); + await parseInput(parser, `"${jsonFixtures.notObjectSingleItem()}"`); + + t.fail('Exception expected'); + } catch (err) { + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); + } + } + ); + testRunner.add('should error if input data is not an object', async (t) => { try { const parser = new Parser(); - await parseInput(parser, jsonFixtures.notAnObject()); + await parseInput(parser, jsonFixtures.notObjectArray()); t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); } }); @@ -181,7 +201,7 @@ export default function (jsonFixtures, csvFixtures) { t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal(err.message, 'Data should be a valid JSON object or array'); } } ); diff --git a/packages/whatwg/test/AsyncParserInMemory.js b/packages/whatwg/test/AsyncParserInMemory.js index 80a37cf..dfb97fa 100644 --- a/packages/whatwg/test/AsyncParserInMemory.js +++ b/packages/whatwg/test/AsyncParserInMemory.js @@ -136,14 +136,34 @@ export default function (jsonFixtures, csvFixtures) { } ); + testRunner.add( + 'should error if input data is single item and not an object', + async (t) => { + try { + const parser = new Parser(); + await parseInput(parser, `"${jsonFixtures.notObjectSingleItem()}"`); + + t.fail('Exception expected'); + } catch (err) { + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); + } + } + ); + testRunner.add('should error if input data is not an object', async (t) => { try { const parser = new Parser(); - await parseInput(parser, `"${jsonFixtures.notAnObject()}"`); + await parseInput(parser, jsonFixtures.notObjectArray()); t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); } }); @@ -175,7 +195,7 @@ export default function (jsonFixtures, csvFixtures) { t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal(err.message, 'Data should be a valid JSON object or array'); } } ); diff --git a/packages/whatwg/test/TransformStream.js b/packages/whatwg/test/TransformStream.js index 4196c6e..77b5b71 100644 --- a/packages/whatwg/test/TransformStream.js +++ b/packages/whatwg/test/TransformStream.js @@ -125,14 +125,34 @@ export default function (jsonFixtures, csvFixtures) { } ); + testRunner.add( + 'should error if input data is single item and not an object', + async (t) => { + try { + const parser = new Parser(); + await parseInput(parser, jsonFixtures.notObjectSingleItem()); + + t.fail('Exception expected'); + } catch (err) { + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); + } + } + ); + testRunner.add('should error if input data is not an object', async (t) => { try { const parser = new Parser(); - await parseInput(parser, jsonFixtures.notAnObject()); + await parseInput(parser, jsonFixtures.notObjectArray()); t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal( + err.message, + 'Data items should be objects or the "fields" option should be included' + ); } }); @@ -164,7 +184,7 @@ export default function (jsonFixtures, csvFixtures) { t.fail('Exception expected'); } catch (err) { - t.equal(err.message, 'Data should be a JSON object or array'); + t.equal(err.message, 'Data should be a valid JSON object or array'); } } );