Json::Value crash after copy

[cpisz-compeat]

Is Json::Value meant to be copyable?
Am I permitted to store it in a std::vector and then retrieve it later and call its methods?

I am developing and testing my own "path" method in a class built on top of JsonCpp. When using the path build into the JsonCpp library, it would crash with any path more than 1 level deep. So, I figured I'd roll my own. It also crashes on any level more than 1 deep when calling a method on a Json::Value that was stored in an std::vector.

It looks like it occurs on line 1003 of json_value.cpp is being triggered or an exception is thrown, but the type is an array value, so I don't understand what's going on. gtests is gobbling it up. I will try taking this code out of gtest in the meanwhile to see if it's an exception or the assert.

My method:

//--------------------------------------------------------------------------------------------------
// In development
std::vector<JsonLoader::JsonElement> JsonLoader::GetFromPath(const std::string & path)
{
    std::vector<JsonElement> result;
    JsonElement element;
    element.m_key    = "root";
    element.m_value  = m_root;
    element.m_parent = Json::Value(NULL);
    result.push_back(element);

    // Tokenize the path
    // Currently, the only path symbols we support are 
    // '/'

    if (path.empty())
    {
        return result;
    }

    size_t                  currentPosition = 0;
    char                    currentChar = path[currentPosition];
    std::string             currentToken;
    std::deque<std::string> tokens;

    if (currentChar != '$')
    {
        const std::string msg("Json path must begin with  '$' for root.");
        throw Shared::Exception(__FILE__, __LINE__, msg);
    }

    while(currentPosition < path.size() )
    {
        switch (currentChar)
        {
        case '$':
            break;
        case '.':
            tokens.push_back(currentToken);
            currentToken.clear();
            break;
        default:
            currentToken.push_back(currentChar);
            break;
        }

        ++currentPosition;
        currentChar = path[currentPosition];
    }

    if (!currentToken.empty() )
    {
        tokens.push_back(currentToken);
        currentToken.clear();
    }

    std::vector<JsonElement> elementsLevelBeingSearched;
    std::vector<JsonElement> elementsLevelToSearchNext;

    elementsLevelToSearchNext.push_back(element);

    for(size_t currentDepth = 0; currentDepth < tokens.size(); ++currentDepth)
    {
        elementsLevelBeingSearched = elementsLevelToSearchNext;
        elementsLevelToSearchNext.clear();

        for (std::vector<JsonElement>::iterator itElementsBeingSearched = elementsLevelBeingSearched.begin();
             itElementsBeingSearched != elementsLevelBeingSearched.end(); ++itElementsBeingSearched)
        {
            JsonElement & elementBeingSearched = *itElementsBeingSearched;

            // Crash occurs here on 2nd iteration of outter for loop
            // with Path = "$people_with_jobs.jobs"
            if ( elementBeingSearched.m_value.isMember(tokens[currentDepth]) )
            {
                element.m_key    = tokens[currentDepth];
                element.m_value  = elementBeingSearched.m_value.get(tokens[currentDepth], Json::Value());
                element.m_parent = elementBeingSearched.m_value;

                elementsLevelToSearchNext.push_back(element);
            }
        }
    }

    return elementsLevelToSearchNext;
}

---

class JsonLoader : public IJsonLoader
{
public:
    // This struct is really just for easy debugging for now
    struct JsonElement
    {   
        std::string m_key;
        Json::Value m_value;
        Json::Value m_parent;
    };

    /// <summary>
    /// Constructor
    /// </summary>
    JsonLoader(const std::string & json);

    /// <summary>
    /// Deconstructor
    /// </summary>
    virtual ~JsonLoader();

    /// <summary>
    /// Implements <c>IJsonLoader</c>
    /// </summary>
    virtual void Parse(const std::string & json,
                       const std::list<EntityDefinition::SharedPtr> & entityDefinitions);

protected:

    Json::Value  m_root;

    /// <summary>
    /// Based http://goessner.net/articles/JsonPath/
    /// If the path references a non existant value Json::Value(NULL) is returned.
    /// in development
    /// </summary>
    virtual std::vector<JsonElement> GetFromPath(const std::string & path);

};

[cdunn2001]

In theory, yes, it should be copyable. Here is line 1003. This will be hard to debug without more info. A test that we can run to repro the problem would be ideal. A stack-trace would be helpful.

[cpisz-compeat]

In what format would you like said test? gtest? just a pain int main() runner?
Version I was running is 0.10.5, that is the latest, correct? My line 1003 points to the assert.

[cdunn2001]

Best is if you can add a test-case to our current suite.

[cpisz-compeat]

I can't figure out your test suite for the life of me. I've got no python experience or even have it installed, nor am I familiar with the other tools.

Here is a stand alone console application stripped to it's bare bones demonstrating the first problem:
jsoncpptest1.txt

Working on the second which occured when I actually tried to manually implement path.

[cdunn2001]

First, you are using the old Reader. We expect that to work, but the CharReader is more flexible (and much easier for us to maintain).

Your code does not compile for me on OSX with the tip of master:

foo.cpp:97:17: error: call to constructor of 'Json::Value' is ambiguous
    Json::Value root(NULL);
                ^    ~~~~

I fixed that by removing NULL. I suspect you are using an old commit. Any idea which commit?

What compiler are you using for _ASSERT? Most people would simply #include <cassert>.

You are not using path properly. Path-resolution (which I personally never use, but some people like it) only works on dictionary JSON elements, but you have applied it to an array. See the array?

{
...
"people_with_jobs": [{
   "name": "Pete",
   "jobs": [{
       "job": "books"
   }, {
       "job": "cleanup"
   }]
}]
}

The code does not crash; it raises a LogicError exception:

terminating with uncaught exception of type Json::LogicError: in Json::Value::find(key, end, found): requires objectValue or nullValue

And why are you even using path in your test-case? Just skip it. Use the simple operator[](). You can fix your assertions this way:

    assert( !root["people_with_jobs"].isNull() );
    assert( !root["people_with_jobs"][0]["jobs"].isNull() );

I dumped the JSON before and after parsing. It looks the same to me. We need a simple test-case to repro the problem. So far, I have seen no evidence of a bug.

[cdunn2001]

Oh, I confused this with a different Issue. This might be a valid bug. If you can find a real repro, please post it into GitHub as a "gist". You can post the JSON input there too.

[cpisz-compeat]

I am using 0.10.5 as mentioned previously. That is listed as the latest release.

I figured out that path will not give you a collection of nodes at all, but requires you to path to a specific node and that node has to be an individual object and can only be an object. When comparing to a xpath syntax in XML, I am expecting a collection of all nodes that match the path. Therein lies the problem. I'm gonna try my hand at implementing it manually.

The copy problem mentioned earlier might not be a real problem. As I am implementing the path manually, I am not running into it again. It appears to be a bug on my part where I thought I was dealing with an element of an array rather than the array.

I can't use operator [] because, according to the doc, creates if it does not exist. That is not my desire.

Can go ahead and close/disregard this issue for now.

[cdunn2001]

Yes, operator[]() is not what you want, but you get the idea.

Path probably should have been a separate library. There is more than one sensible behavior.