Open avandekleut opened 1 year ago
Hello avandekleut,
I can confirm that isJSON
returns false
for the test case you provided. I didn't know that that was also considered valid "JSON" to be honest :-)
From what I can see, the validator already uses JSON.parse
internally, but then also checks, if the output of the JSON.parse operation is of type object
– and this is where the "0" examples then fails, because that is of type number
(regardless of whether you originally cast it as string or a number).
Additionally it also fails, the validator tries to coerce the output to a boolean via the double exclamation marks, but because 0
is falsy, it will of course also return false
.
See here: https://github.com/validatorjs/validator.js/blob/43803c08d23b9cf316f2e804445b643b52920121/src/lib/isJSON.js#L18
Only question here is, if that was maybe done intentionally, to only allow strings in "expected JSON" formatting? (i.e. anything in curly brackets?)
I guess we should also double-check with the RFC on this as well: https://www.rfc-editor.org/rfc/rfc8259
The RFC states
A JSON value MUST be an object, array, number, or string, or one of the following three literal names: false null true
By this logic, we should be able to delegate validation entirely to JSON.parse
. What do you think?
That would cause more to be accepted than people expect now, so maybe that's for the next major release? And in the meantime we can add a new option to allow non-object values? Like the allow_primitives
option
To adhere to semantic versioning, I assume the following behaviour would have to be preserved:
Arrays are validated
Objects are validated
if allow_primitives
is set to true
:
null
, true
, and false
are validatedThe following behaviour is not implemented, but could be implemented without breaking the default behaviour:
allow_primitives
is set to true
:
string
s and number
s are validated.Since the default is allow_primitives: false
, we can add the new behaviour when allow_primitives
is set to true
.
Here is my implementation of that change, with comments:
import assertString from './util/assertString';
import merge from './util/merge';
const default_json_options = {
allow_primitives: false,
};
export default function isJSON(str, options) {
assertString(str);
/* this doesn't need to be in the try block */
options = merge(options, default_json_options);
try {
/* JSON.parse should succeed if str is a valid stringified JSON object */
const obj = JSON.parse(str);
if (allow_primitives) {
/* by default, primitives are allowed in json */
return true;
}
/* original behaviour is to only validate arrays and objects by default */
/* replaced !!obj with obj !== null which is more readable */
return (obj !== null && typeof obj === 'object');
} catch (e) { /* ignore */ }
return false;
}
Describe the bug
The function
isJSON
rejects valid json.Examples
You can also use
fast-check
to generate examples for testing:Additional context Just wondering why we don't do the following?