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

Development/json types #1433

Closed
wants to merge 104 commits into from
Closed

Development/json types #1433

wants to merge 104 commits into from

Conversation

msieben
Copy link
Contributor

@msieben msieben commented Oct 20, 2023

No description provided.

pwielders and others added 30 commits July 7, 2023 22:33
…f possible.

In case the JSONRPC message reeived is malformatted, Thunder should try to return
a JSONRPC error on malformatted JSON message in stead of:
1) Not responding at all.
2) Send an Web Error status even if the ID was sxtracted from the message.

These fixes are related to: METROL-802/METROL-803
Starting or ending of an opaque structure should *only* contain white spaces. Anything
else should be considered something to be parsed..
…earlier.

In the mean time also prevented quoted areas from being accounted in the cope counting and in case we are
deserializing in opaque areas drop the white space for further processing and storage.
…well..

Checking the validity of the JSONRPC messages coming in over the websockets. If incorrect, rturn an error if possible.
Empty (opaque) strings should be quoted if enabled.
See issues:

- METROL-779

- METROL 780

 -METROL-781

- METROL-782

- METROL-795

- METROL-796

- METROL-797
…st cases.

- See issue: METROL-798

- Rename 'test_jsonnumbertype.cpp' to 'test_jsontypes.cpp'
Copy link
Contributor

@pwielders pwielders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to avoid allocation using new. So far it was never needed hence my quesion what are we fixing that requires this ?


// FIXME: Currently fallback to underlying String instead of actual type
switch(depth) {
case 1 : element = new ArrayType<String>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What issue is resolved by these allocations. Allocations definitely on these low levels of functionality, might cause fragmentation of memory. So far no issues have been reported that require this allocation so would like to understand the need of this construction. What is the use-case that gives erroneous output without this allocation construction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed with @MFransen69 the code path is there not to resolve any reported issue although it shows fields in config.json have no corresponding IElement in the contained list. For debugging purposes and to simplify unit testing for now it has been quite helpful. The use of Add(..) prior Deserialize(..) and ignoring unmatched fields is preferred. A proposed (recommended) approach is to remove the code path from a release delivery and save it elsewhere for use with the aforementioned purposes.

@rdkcmf-jenkins
Copy link

Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

  • Protex Server Path: /home/blackduck/github/Thunder/1433/rdkcentral/Thunder

  • Commit: 071c318

Report detail: gist

@rdkcmf-jenkins
Copy link

Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

  • Protex Server Path: /home/blackduck/github/Thunder/1433/rdkcentral/Thunder

  • Commit: 071c318

Report detail: gist

@rdkcmf-jenkins
Copy link

WARNING: A Blackduck scan failure has been waived

A prior failure has been upvoted

  • Upvote reason: not a problem

  • Commit: 071c318

@msieben
Copy link
Contributor Author

msieben commented Jun 4, 2024

Closing the PR but keeping the related branch.

@msieben msieben closed this Jun 4, 2024
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

Successfully merging this pull request may close these issues.

None yet

5 participants