Feature Request: JSON Parser

[alexandruiulian10]

Provides a proposal for a safe JSON parser feature.

[github-actions]

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server and connecting to it...
INFO: Invocation ID: 62a09ee5-9071-436d-8e1f-6338083f0669
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
WARNING: Target pattern parsing failed.
ERROR: Skipping '//:license-check': no such target '//:license-check': target 'license-check' not declared in package '' defined by /home/runner/work/score/score/BUILD
ERROR: no such target '//:license-check': target 'license-check' not declared in package '' defined by /home/runner/work/score/score/BUILD
INFO: Elapsed time: 4.964s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

[masc2023]

so what is the impact? Access Control needed?, please set also tag security to yes

[alexandruiulian10]

For access control and manipulation prevention (e.g. dm-verity) the hosting process and system configuration need to take care of it.

[masc2023]

then may add this as assumption of use in the feature request, otherwise it get lost, by the way, this is more a component_request as part of feature:baselib

[aschemmel-tech]

Checked: is now a component request, but AoU is still missing. Additionally we should have a seperate AoU that states that the integrity of the input JSON data is with the user (i.e. if it comes from a file it must be protected)

[alexandruiulian10]

I added this in the latest commit.

[aschemmel-tech]

Sorry about not making this clear: The AoU should be done as a requirement using template https://eclipse-score.github.io/process_description/main/process_areas/requirements_engineering/guidance/requirements_templates.html#gd_temp__req__aou_req

[alexandruiulian10]

I added the two AoU requirements in the latest commit.

[github-actions]

The created documentation from the pull request is available at: docu-html

[lennartbecker-d]

To sum things up:
Which public API is meant in the different requirements?

[lennartbecker-d]

What is the intention here? Which public API is meant here. Do you mean the one implemented in baselibs or the one provided by nlohmann?
Just considering how this might be supported with evidence? Is an SME review necessary here?

[aschemmel-tech]

Requirement is "Non-functional" so a "SME review" is appropriate. After discussion with @LittleHuba we concluded we wanted this kind of requirement (which is applicable for every user API) rather defined in a coding guideline. But this is a future PR so let's keep this here for the moment.

[lennartbecker-d]

This will be proven to be fulfilled by SME review or on codebasis? Which API is meant here again? The requirement appears to be very general and vague.

[aschemmel-tech]

same comment as above

[sankurm]

Thanks a lot for the great progress in this area.

I classify my comments with the following priority/importance (must/should/could/may):

1. Must: parsed_json reqs
2. Must: Error handling reqs
3. Could: JSON schema reqs
4. May: Compile-time parsing

[sankurm]

The requirements around the 'parsing functionality' of JSON have been captured here. The outcome of the parsing shall be a parsed representation of the JSON. Let me refer to this as the parsed_json. I did not find any requirements that relate to the parsed_json. I would like to note some high-level aspects related to the parsed_json.

The parsed_json shall

1. be a regular type
2. not expose the implementation library types (as this can restrict future changes/evolution)

Some of the above requirements may relate to other components & modules too. Such high-level requirements should be considered to be elevated as guidance (e.g. thou shalt not expose implementation-level data-types across the component boundaries).

[alexandruiulian10]

I added a requirement for this in the latest commit.

[sankurm]

Our implementations should have a greater guarantee. Error handling should not be left to a Note in my opinion.

I would like to suggest the following requirements: (again, if found generic, we could promote to overall Eclipse S-CORE level)

1. Error encountered (due to incorrect input, system error, or otherwise) should be reported back to the user
2. Error reporting via non-throwing mechanisms is preferrable (like optional<T>, std::expected<T>, score::cpp::optional, score::Result<T>)
3. In case of error, it should be difficult for the users to carry out operations on the parsed representation of the JSON as if it was successful. The API shall allow foe easy error handling in case of an error. This is to make correct implementations easier than not.

Sample code:

//Let's say the application wants to apply config read from a JSON
void apply(score::json);

//Possible API offered by this component
using score::json = ...defined by this component...;
score::Result<score::json> deserialize(const std::string_view json_str) { ... }

//Such an API cannot be used incorrectly
auto ill_formatted_json = "xyz"sv;
auto my_config = deserialize(ill_formatted_json);

//The following must not even compile as it is trying to use the return as json
apply(my_config);

//Error handling must be possible
score::json default_config = { {"threshold-min", 80}, {"threshold_max", 95} };  //should be possible to construct directly
apply(my_config.value_or(default_config));

[alexandruiulian10]

This is a good point, but I think we should clarify if it should be added here or somewhere else. It seems to me more like a general S-CORE guideline than something specific to the JSON library.

[aschemmel-tech]

Also here, I discussed with @LittleHuba and said we would not have this in every component's requirements but add to a Coding Guideline. No actions in this PR. #1533 created

[sankurm]

A possible requirement could be to ensure correctness by default by allowing compile-time constructs & checks.

I would put it as below:

1. to be able to parse a JSON at compilation-time. This can help have compile-time tests (with facilities like static_assert). This will block compilation if the implementation does develop an error or incorrectness. This will ensure that a component with errors cannot exist even in the absence of a pipeline (yes, the tests should be extensive). Plus, generally, it will shift-left catching errors.

I understand that the nlohmann library does not offer compile-time parsing. So, this can be kept as a good to have requirement. Or be dropped, if we decide to not support.

[aschemmel-tech]

We should only have requirements which can be fulfilled by our implementation (OSS component we select). Can be added to the component requirements later if really required. So we should add this to the component request in https://eclipse-score.github.io/score/pr-822/modules/baselibs/json/docs/index.html#future-extensions

[sankurm]

Do we want JSON schema validation as a requirement?

Something like this:

1. It should be possible to validate a json against a json-schema.

This could be lower in priority as compared to the core functionality depending on the need across all partners.

[aschemmel-tech]

We discussed this already in the past and then the decision was to not have schema validation. Can be added to the component requirements later if really required. Also here: add this to the component request in https://eclipse-score.github.io/score/pr-822/modules/baselibs/json/docs/index.html#future-extensions with the addition that this might even be an additional component (request).

[arsibo]

Requirements are fine from logging side
https://github.com/orgs/eclipse-score/discussions/1414#discussioncomment-13787441

[aschemmel-tech]

Two comments originally from masc2023 still have to be resolved

[aschemmel-tech]

see also updated inline comments

[aschemmel-tech]

I would say this is a Functional requirement, as could be tested.

[alexandruiulian10]

Changed to :reqtype: Functional in the latest commit.

[PhilipPartsch]

I believe this requirement is not security relevant.

[alexandruiulian10]

Changed to :security: NO in the latest commit.