C++ rapidjson: GenericValue::IsNull is returning false in any case

Question

I still shocked after detecting a mysterious issue on our project.

We realized that calling HasMember("string") was performing an extra seek. So, for performance reasons, we change it.

The main idea is:

Instead of calling HasMember and afterwards precaching the reference like:

rapidjson::Document d;
d.Parse<0>(json);

if(d.HasMember("foo"))
{
    const rapidjson::Value& fooValue = d["foo"];

    // do something with fooValue
}

Changed to:

rapidjson::Document d;
d.Parse<0>(json);

const rapidjson::Value& fooValue = d["foo"];
if( !fooValue.IsNull() )
{
    // do something with fooValue
}

This was pretty good, we save to perform two seeks instead of only one. However, here it comes the issue.

If you start looking how rapidjson implements nullvalue (returned by default when seek fails) you will see the following code:

//! Get the value associated with the object's name.
GenericValue & operator[](const Ch* name) {
    // Check
    if (Member * member = FindMember(name)) {
        return member->value;
    } else {
        // Nothing
        static GenericValue NullValue;
        return NullValue;
    }
}

// Finder
const GenericValue & operator[] (const Ch* name) const { 
    // Return
    return const_cast<GenericValue &> (* this)[name]; 
}

So, if didn't find the member we return a local static variable. This may sound good enough at first glance but since this is returning by reference can lead easily to hidden bugs.

Imagine that someone change the reference of the static NullValue. This will cause that all further calls to IsNull (after looking for it) will fail because the NullValue changed to another type or even to random memory.

So, what do you thing? Do you think this is a good null pattern example?

I am confused, I like the idea of returning a default null value but since is not returned as const this is dangerous. And, even, if we do return it in all cases as const, devs can still use const_cast (but I wouldn't expect that, if they do, will be under them responsibility).

I want to hear other cases and examples like this one. And if someone can give a real solution under rapidjson code would be basically awesome and amazing.

Answer 1

The pitfall of this design has been raised up by the community long time ago. Since operator[] also needs a non-const version, it is not possible to maintain the integrity of the static variable.

So this API has been changed in newer versions of RapidJSON. The operator[] simply assert for non-exist key. If it is unsure that a key is exist, it is preferably using

MemberIterator FindMember(const Ch* name);
ConstMemberIterator FindMember(const Ch* name) const;

And comparing the value with MemberEnd() to check whether the key exists. This is also documented here.

Besides, please note that RapidJSON has been moved to GitHub. Many issues has been resolved. Please use the newest version if possible. Thank you.

P.S. I am the author of RapidJSON.