Bug in basic_json::operator[] const overload

[dariomt]

The following code

// note the *const*
const json object =
{
        {"one", 1}, {"two", 2}, {"three", 2.9}
};
std::cout << object.size() << '\n';
std::cout << object["doh!"] << '\n'; // element not found
std::cout << object.size() << '\n'; // const object modified!

prints

3
null
4

Allowing a const object to change like this is not a good idea.
I think this is a bug: either the const overload of operator[] throws if the key cannot be found, or there is no such const overload (e.g. std::map does not have it).

[gregmarr]

The problem is that json::operator[] const simply chains to array_t/object_t::operator[] using m_value.array/object, which is a const pointer to a non-const object.
For array, instead of doing this:

    return m_value.array->operator[](idx);

This will fix it:

    const array_t *array = m_value.array;
    return array->operator[](idx);

For object, it's a bit more complicated, since as you said,map doesn't have operator[] const:

    auto iter = m_value.object->find(key);

    if (iter == m_value.object->end())
    {
        throw std::domain_error("cannot use operator[] with non-existent key on const object of type " + type_name());
    }

    return iter->second;

[arnaudbrejeon]

This problem is very misleading. I use a json object for parameter input/output and it ends up with a lot of null objects.
Do you have any idea when it could be fixed?
Thank you.

[nlohmann]

I'm on holiday right now. I'll check this when I come back.

[nlohmann]

Changing a const object is definitely not OK. However, I think it was a mistake to offer an operator[] as const function in the first place, because such a function does not exist for std::map, and I wanted to mimic the STL containers as close as possible.

I see two options here:

1. Implement @gregmarr 's fix.
2. Make the operator[] non-const. In this case, one still can use at. At the same time, one looses the simple syntax to access const objects.

Even with the downside, I would prefer option 2, because it is closer to the other STL containers.

What do you think?

[arnaudbrejeon]

I would prefer 2 as well.

[dariomt]

+1 for option 2
following the STL convention is good

[nlohmann]

I'll see if I can realize option 2 this weekend.

[gnzlbg]

I need unchecked const access, and was using operator[] const for this. Since it has been removed, I'm stuck with an older version of the library because the proposed "workaround", to use at, is not good enough for my use case.

The problem with at is that it performs the check on every access, but for my use case I:

• only want to check in debug builds,
• can coalesce lots of checks when I construct the json object (which I then pass around as const since I do not intend to modify it),
• can coalesce lots of checks before loops.

IIUC I have been left without an alternative/workaround for my use case. Is there one?

In a nutshell what I need is:

unchecked const access with undefined behavior if the element is not there

That is basically what operator[] const is/was there for.

I understand that in the previous state operator[] const was very error prone/buggy.

For me, using the standard NDEBUG macro, or some library specific macro to, in debug builds, either assert/throw/call a user-provided callback/... if the element is not there would have been a better solution (undefined behavior lets you do anything).

I can understand the rationale of "doing what std::map does", but this is an API breaking change, that was not implemented using the [[deprecated]] attribute, and for which no workaround has been provided (correct me if I'm wrong here, I wasn't able to find a function to perform unchecked const access).

[nlohmann]

Hi all, just to let you know, I reintroduced the const operator[], see http://nlohmann.github.io/json/classnlohmann_1_1basic__json_a8e34088252a3ee6b2377f3a1f26dd1ba.html#a8e34088252a3ee6b2377f3a1f26dd1ba. Sorry for all the inconvenience.