Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not throw on duplicate keys #57

Closed
jordanbtucker opened this issue Jul 28, 2014 · 6 comments
Closed

Do not throw on duplicate keys #57

jordanbtucker opened this issue Jul 28, 2014 · 6 comments

Comments

@jordanbtucker
Copy link
Member

According to the note at the bottom of ES5 §15.12.2 regarding JSON.parse:

In the case where there are duplicate name Strings within an object, lexically preceding values for the same key shall be overwritten.

JSON5 throws on duplicate keys because that's what Crockford's implementation does. However, the JSON standard, Node.js, and browsers do not throw on duplicate keys.

> JSON.parse('{"a": true, "a": false}')
{ a: false }

> JSON5.parse('{"a": true, "a": false}')
SyntaxError: Duplicate key "a"

I propose that JSON5 overwrite duplicate keys instead of throwing.

I believe that JSON5 should be completely backward compatible with JSON. Anything that JSON can parse, JSON5 should parse exactly the same. There are other examples where JSON5 is not backward compatible, which I will begin documenting. Perhaps the Wiki is a good place for that?

@aseemk
Copy link
Member

aseemk commented Jul 29, 2014

Agreed, thanks!

If JSON5 is not backwards compatible with JSON anywhere, that's definitely worth an issue/bug (like this) instead of the wiki I think.

@amb26
Copy link
Contributor

amb26 commented Dec 9, 2015

I know that this issue has been long-closed, but I did want to express my preference for the other decision - repeating a key in a structure, as far as I can see, always represents an error or oversight of some kind, and it's a mistake I've made several times myself! I think it would be great if we returned to the original behaviour. This thread contains some interesting discussion: http://stackoverflow.com/questions/21832701/does-json-syntax-allow-duplicate-keys-in-an-object

an interesting point being that whilst ECMA-404 doesn't say anything about duplicate keys, RFC 7159 says that they "should" not occur. It would be possible in time, I guess, to have a JSON5 standard that adopted one or other of these choices without saying that we had "deviated" from the JSON behaviour which seems to mostly result from accidents in parser implementation.

I know this decision is unlikely ever to be revisited but I thought I'd put my opinion on record :)

@jordanbtucker
Copy link
Member Author

@amb26 I'm in agreement with you that having duplicate keys in a JSON document is bad form. However, the major ECMAScript implementations—V8, SpiderMonkey, and Chakra—all allow duplicate keys. And they should, because according to the official spec, once the parser validates that a JSON document conforms to the JSON syntax, it parses the document as ES5 (almost). Note that RFC 7159 only describes the structure of JSON documents but not the API and implementation requirements.

All this considered, one of the core values of JSON5 is being backward compatible with JSON—in document structure, API, and implementation.

I do see some value in logging a warning when duplicate keys are encountered, especially since JSON5 is meant for handwritten data documents where duplicate keys are more likely to occur.

I appreciate your opinion and research on this matter and welcome any further comments. And don't be too quick to think that these issues won't be revisited. Just look at some of the earliest issues for this repository and note how many times the community went back and forth on some pretty important features :)

@zamicol
Copy link

zamicol commented Jul 25, 2022

Although I'm not advocating for it, disallowing duplicates is a one word change to the JSON RFC from:

The names within an object SHOULD be unique.

To

The names within an object MUST be unique.

Now what I would advocate for is this: there's a strong argument to be made for JSON5 to disallow duplicates while guaranteeing fully backward compatible with JSON.

JSON5 should require that implementations default behavior set as "MUST", and for implementations that support it, an optional flag for "SHOULD". ( Crockford's implementation would not have an optional "SHOULD" flag since the implementation doesn't support "SHOULD" and only supports "MUST" behavior). This allows strict backward comparability while permitting all the advantages of deduplication.

Other related thoughts:

Crockford's implementation is a great example of desired behavior. Although the standard allows it, the implementation from the author precludes it.

Disallowing duplcates would conform to the very small I-JSON RFC. The author of the I_JSON RFC, Tim Bray, is also the author of the JSON RFC 8259

These are all authority figures, and I would consider it reasonable for JSON5, which makes minor "improvements" to JSON, to do this.

There's also security problems and interoperability problems with duplicates:
https://bishopfox.com/blog/json-interoperability-vulnerabilities.

@jordanbtucker
Copy link
Member Author

@zamicol Thank you for your suggestions. You make some interesting points. Would you be willing to echo these ideas as a new issue on the JSON5 spec repo?

Issues on this repo are now primarily for changes to this library rather than the spec.

@zamicol
Copy link

zamicol commented Jul 26, 2022

Thanks @jordanbtucker. I submitted the suggestion: json5/json5-spec#38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants