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

JSON in C++ #383

Closed
SteVwonder opened this issue Aug 30, 2018 · 13 comments
Closed

JSON in C++ #383

SteVwonder opened this issue Aug 30, 2018 · 13 comments
Assignees

Comments

@SteVwonder
Copy link
Member

Now that more of flux-sched is becoming C++, do we want to keep using the jansson C interface to interact with JSON objects from within C++, or do we want to pull in a C++ JSON library? @trws recommended JSON for modern C++ (which would require a patch to compile under gcc 4.8.5) or RapidJSON.

@dongahn
Copy link
Member

dongahn commented Aug 30, 2018

I guess when we have real use cases. I haven't had one yet. Flux's pack and unpack has been sufficient for me so far.

@dongahn
Copy link
Member

dongahn commented Apr 26, 2019

#455 clearly indicates that we will absolute need this for better code maintainability.

@trws
Copy link
Member

trws commented Apr 26, 2019

One thought I was reminded of earlier this week. I think either of those fit the bill, but it would be really good to pick one that interoperates well with one of the jsonschema implementations for C++ so we don't need to end up with multiple. Might be good to replace the python dep on jsonschema in the long run too if that becomes a performance issue.

@dongahn
Copy link
Member

dongahn commented Apr 26, 2019

Makes sense.

FWIW, for sched for now, python jsonschema isn't on the critical path as I just used it for testing. For actual execution, @grondo and I decided we won't pass an R through jsonschema validation as this is an internal operation and a malformed R will result in an exception raised from the execution system.

But in general, it does make sense to have "embedded jsonschema" support as this seems to become trendy with REST API and etc.

@SteVwonder
Copy link
Member Author

SteVwonder commented Apr 27, 2019

it would be really good to pick one that interoperates well with one of the jsonschema implementations for C++

That's a good point. Here are the C++ json-schema validators that I could find (via the json-schema.org implementations list and from googling) and the C++ json parser that they are compatible with:

jsonschema lib compatible json parsers notes
Valijson many requires C++11, supports json-schema draft-4
RapidJSON RapidJSON supports json-schema draft-4
pboettch's nlohmann's JSON for Modern C++ supports draft-7
KayEss's ? requires C++17

EDIT: nlohmann's JSON now supports gcc 4.8

@dongahn
Copy link
Member

dongahn commented Apr 27, 2019

Probably need draft7 support and C++11.

@SteVwonder
Copy link
Member Author

Probably need draft7 support and C++11.

I definitely agree that we should limit ourselves to C++11 (and not use C++17, etc).

Is there anything, in particular, that we need from draft-5 through draft-7? If not, I think RapidJSON is very appealing as both a C++ library and json-schema validator. It would be just a single new dependency, rather than two, and the JSON parser is extremely fast (faster than jansson IIRC).

That being said, nlohmann's JSON for Modern C++ recently added support for gcc 4.8, so that is back to being a viable option for our JSON parser. I updated the table to reflect that. IIRC, this was the C++ JSON parsing library that @trws recommended. Which opens up support for draft-7 via pboettch's jsonschema library.

@dongahn
Copy link
Member

dongahn commented Apr 29, 2019

The official JGF jsonschema is written for draft4 so I don't have use case for draft 7 yet. If we ever add schema for other formats like rlite we can always just conform to draft4. So if RapidJson looks promising otherwise, this should still be viable.

I don't know if jobsprec jsonschema used any other features from draft 4 - 7.

@trws
Copy link
Member

trws commented Apr 30, 2019 via email

@SteVwonder
Copy link
Member Author

I don't know if jobsprec jsonschema used any other features from draft 4 - 7.

Just found two features of draft-5 that would be nice to leverage: $merge and $patch. They essentially allow you to do "inheritance" with json-schema definitions that have additionalProperties: false. If additionalProperties is true, then you can just use allOf, otherwise, you are stuck duplicating properties in draft-4. More details: https://stackoverflow.com/questions/27410216/json-schema-and-inheritance

v7 also supports comments, which is really helpful considering how complicated these schemas can become.

Given this and the previous comments from @dongahn and @trws, I vote that we give nlohmann's JSON for Modern C++ and pboettch's jsonschema libraries a shot. They support draft-v7, are nice interfaces, and work with gcc 4.8. If we hit a deal breaker, we can always fallback to RapidJSON.

@SteVwonder SteVwonder self-assigned this Jul 5, 2019
@dongahn
Copy link
Member

dongahn commented Jul 5, 2019

Thank you for the research! Bringing this into flux-sched will substantially simplify our code base and make it less prone to errors. In terms of priorities though, we will need to discuss when would be a good transition point and put a plan around it.

@trws
Copy link
Member

trws commented Jul 6, 2019 via email

@dongahn
Copy link
Member

dongahn commented Jul 16, 2020

We decided to stick w/ Jansson.

@dongahn dongahn closed this as completed Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants