jorgen / json_struct

json_struct is a single header only C++ library for parsing JSON directly to C++ structs and vice versa
Other
422 stars 57 forks source link

Reading JSON which contains certain extraneous values breaks parsing #24

Closed noodlecollie closed 2 years ago

noodlecollie commented 2 years ago

I discovered this issue when I was trying to parse a small subset of data from a large JSON message. If I created a set of nested structs with members corresponding to only the data I needed, the parse would fail with an ExpectedObjectStart error at an arbitrary location. However, if I added in all of the values present in the message, it would parse correctly.

I spent some time investigating this and have managed to come up with a minimal reproducible example. In the example the parsing doesn't explicitly fail, but rather the parser seems to get out of sync and places incorrect values into structs. I believe that the parser getting out of sync was the cause of my original issue.

The fundamental issue seems to be surrounding nested objects. Take the following JSON:

{
    "object1":
    {
        "value": 1,
        "nested_object":
        {
            "nested_1":
            {
                    "some_value": "foo"
            },
            "nested_2":
            {
                    "some_value": "bar"
            }
        }
    },
    "object2":
    {
            "value": 2
    }
}

If this message is parsed using structs, where every key-value pair is catered for, then parsing succeeds as expected. However, if nested_object is omitted from the struct definition, this triggers the parsing issue. Note that the issue does not occur unless the nested_object has at least two other nested JSON objects within it.

The attached C++ project encapsulates the minimal reproducible example. In the case where the nested object fails to parse correctly. the struct representing object2 contains a value of 0 when it should contain a value of 2.

json_struct MRE.zip

noodlecollie commented 2 years ago

After further investigation, I reckon something is up with the implementation of skipArrayOrObject(). I'll continue looking.

noodlecollie commented 2 years ago

I've got an implementation locally that seems to fix the issue, I'll make a PR for it.

jorgen commented 2 years ago

Hi, thank you for your time and effort. I really appreciate it!

jorgen commented 2 years ago

I took the liberty of pushing c2b4906cd72a98bf7953a0be6732fdb91cf71e9b under your name. Its the testcase you attached, reworked in to a unit test.

jorgen commented 2 years ago

Ok, I was reviewing the code once more, and I compare it with what is done when creating the skiplists for the map object. I have pushed a1874db424a24881b77b5970a5465e67bea299f3, its removes the recursion and just loops until done. I don't think its a performance issue, but I like the implementation better. I hope you don't mind that I have overwritten your fix.

noodlecollie commented 2 years ago

That's fine by me! I'll make sure it works on my end as well.