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

Request for I-JSON (RFC 7493) Parsing Mode #127

Open
jayeshbadwaik opened this issue Jul 15, 2020 · 12 comments · May be fixed by #801
Open

Request for I-JSON (RFC 7493) Parsing Mode #127

jayeshbadwaik opened this issue Jul 15, 2020 · 12 comments · May be fixed by #801
Assignees
Labels

Comments

@jayeshbadwaik
Copy link

I was wondering if there is any interest in implementing the I-JSON parsing mode for boost.json. I-JSON is an interoperable version of JSON which aims to remove some ambiguities in the parsing. The changes required for such a compliance are listed in Section 2 of the document.

@vinniefalco
Copy link
Member

It is very likely that we already follow these rules. If so, we should document it. If not, we should consider whether to follow it.

@sdkrystian
Copy link
Member

The only normative requirement we don't comply with is checking for duplicate object key. There is some normative encouragements in there that we don't strictly follow, such as limiting precision.

@vinniefalco
Copy link
Member

We never emit JSON with duplicates, as the DOM (via json::object) does not allow it. When we parse JSON, I believe we always take the first value if there are duplicate keys

@jayeshbadwaik
Copy link
Author

The restriction of allowing only a single object key is during the parsing, not emission.

@vinniefalco
Copy link
Member

When an object is constructed during parsing, we check its elements for duplicates here:
https://github.com/CPPAlliance/json/blob/ec29dfa768a16672534f758005dfe4841a8ea93d/include/boost/json/detail/impl/object_impl.ipp#L79
The duplicates are removed by replacing the duplicate with the last element in the object array, and then shrinking the array size by 1.

@sdkrystian
Copy link
Member

@vinniefalco To conform to I-JSON we would have the check for duplicate keys within basic_parser, not the handler.

@vinniefalco
Copy link
Member

Fuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuck

@vinniefalco
Copy link
Member

basic_parser can never do such a thing, so I don't think we can claim I-JSON support for it. But that's okay, because we can claim I-JSON support in parser which is what 99% of users care about. For the other 1% of users who are implementing their own parser by using basic_parser directly, I think it is reasonable to expect that if they want I-JSON compliance, that can be responsible for it. How do you feel about that @jayeshbadwaik ?

@jayeshbadwaik
Copy link
Author

jayeshbadwaik commented Aug 12, 2020

One way to implement that in your codebase I see is to throw in case of duplicate in here: https://github.com/CPPAlliance/json/blob/ec29dfa768a16672534f758005dfe4841a8ea93d/include/boost/json/detail/impl/object_impl.ipp#L79

And handle that exception upward to report that the provided I-JSON document is ill-formed. This will require adding an option to parse_options to allow the user to parse in "I-JSON" mode and require you to remove noexcept qualifer for object_impl.build(). Since I am not aware of how the rest of the code works, I wonder if that is palatable.

@vinniefalco
Copy link
Member

Actually, when we detect a duplicate we can set a bit somewhere in the object and then in parser we can query new objects to see if there's a duplicate and throw as needed.

@jayeshbadwaik
Copy link
Author

That can work too, yeah.

@vinniefalco
Copy link
Member

We should target this for the middle of year release?

@vinniefalco vinniefalco assigned grisumbras and unassigned vinniefalco Mar 4, 2021
@grisumbras grisumbras linked a pull request Nov 6, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants