Do not throw exception when default_value's type does not match the actual type

[cezheng]

Thanks for this great library. I have been quite happy about almost every aspect of the library, except one point about the value method on object.

Consider this sample code.

int main() {
  nlohmann::json j = { {"b", nullptr}};
  std::string result = j.value("b", "1"); // std::domain_error: type must be string, but is null
  return 0;
}

When I'm using value to specify a default value of a key, I pretty much don't care about whether it exists or not, whether it is a valid value or not. It's pretty much like, if it failed to fetch a value of a specific type, then return the default_value I specified.

So I'm thinking about changing the following method:
https://github.com/nlohmann/json/blob/develop/src/json.hpp#L3653

to something like this:

    template <class ValueType, typename
              std::enable_if<
                  std::is_convertible<basic_json_t, ValueType>::value
                  , int>::type = 0>
    ValueType value(const typename object_t::key_type& key, ValueType default_value) const
    {
        // at only works for objects
        if (is_object())
        {
            // if key is found, return value and given default value otherwise
            const auto it = find(key);
            if (it != end())
            {
                try
                {
                    return *it;
                }
                catch (...)
                {
                    // if some kind of exception occurred, return default value
                    return default_value;
                }
            }
            else
            {
                return default_value;
            }
        }
        else
        {
            throw std::domain_error("cannot use value() with " + type_name());
        }
    }

Which I think will make the value method a lot easier to use. What do you think? @nlohmann

[nlohmann]

Thanks for reporting. I'll have a look.

[ghost]

This has been causing me issues too, this addition would be a great help.

[nlohmann]

@cezheng

I understand your use case, but it is different to the described behavior of the value function:

Returns either a copy of an object's element at the specified key key or a given default value if no element with key key exists.

(Emphasis added). In your example, there exists an entry with key b of value null, and you are requesting value to cast it to a std::string which is not possible - hence the exception from the conversion function.

I am not sure whether your proposed "catch all and return default_value" behavior would be helpful, because it may be confusing.

Consider for instance an object that stores numbers and strings. If you ask value to give you the value for a given key or 42 if an error occurs, you never know whether you got 42, because (1) it was the stored value, (2) there was no value stored at the given key, or (3) a string was stored at the given key.

[cezheng]

@nlohmann That's true if the user cares about error handling. For many real cases, this is just inconvenient. If we really care about whether there's an error, there're plenty of other methods for us to find that out, and providing a default value means we really don't want any exceptions.

[gregmarr]

I'm not sure I agree with that last statement. Providing a different value that what's stored because the caller asked for the wrong type doesn't seem right.

[ghost]

What about only if the value is null? To me it makes sense for .value to return the default value if the value is null. For my usage, there is an API that returns JSON, but a certain string is set to null instead of not being present if it is not relevant, and I want to use the default value for this.

[cezheng]

what about having a nothrow version of value so that users can choose which to use?

[prsyahmi]

We could have a nothrow version
For example in java
json.getString(key) <- throw version
json.optString(key, fallback value) <- nothrow version

[markand]

This bugs me as well, I would like to see much more permissives functions to avoid code like this:

auto it = object.find("property");
if (it != object.end() && it->is_string())
    myvalue = *it;

I would like to see much more permissives implicit conversions functions that also allow casting to default values if not.

json j; // null
int i = j; // i = 0

This will help us deserialize untrusted input much easier! Like this:

auto port = object["network"].value("port", 6667);

What do you think?

[gregmarr]

In that case, untrusted input, would you want that to do a string to number conversion if port were a string?
I think that might be better as something that explicitly calls out that it will do a conversion.

object ["network"].try_parse("port", 6667);

It could be as, cast, value_as, ...

[nlohmann]

With this post, I try to summarize the discussion. Please correct me if I misunderstood.

Using the example of @markand, the current value function distinguishes these cases when called with

auto port = object["network"].value("port", 6667);

1. object["network"]["port"] exists and holds an integer value, say 8080. As a result, variable port equals 8080.
2. object["network"] has no value with key "port". Hence, the default value 6667 is returned, so port equals 6667.
3. object["network"]["port"] exists and holds a non-integer value, for instance a string "port is not set". Right now, a std::domain_error exception would be thrown, because the value type of object["network"]["port"] does not match the default value.
4. object["network"] is not an object, and a std::domain_error exception is thrown.

The current discussion shows that case 3 makes people unhappy. Right now, it seems we have three possibilities:

1. Leave the value function as it is.
2. Follow the proposal of @cezheng and make a non-throwing version of value.
3. Follow the proposal of @markand and try to convert the existing value to the type of the default value.
4. Instead of changing the value function, implement a second version with the behavior of possibility 2 or 3. [edit]

Apparently, “solution” 1 leaves people unhappy… However, it also has the most unsurprising behavior.

For solution 2, I already expressed my concerns that it may be confusing if the default value is returned even if the requested value object["network"]["port"] exists, but just happens not to match the type of the default value.

Solution 3 seems to be an implicit conversion from the stored value to the type of the default value. Here, I see an issue: Such a conversion only makes sense for few types. While a conversion from number types is straightforward, it may already be confusing to assume an implicit conversion from booleans or null to int. But even if some convention is found, what to do with strings, arrays, or objects? For these types, I fail to see reasonable semantics. This in turn may introduce more confusion by falling back to the default value in such cases.

I added Solution 4 after the comment of @gregmarr. [edit]

Did I miss anything?

[gregmarr]

Did I miss anything?

4 Leave the value function as it is and add another function that in case #3 tries to convert to the type of the default value.

It's roughly equivalent to the differences between Int.Parse and Int.TryParse in C#.

[nlohmann]

@gregmarr What do you mean with "tries to convert to the type of the default value"? Apart from "add another function", isn't this what I described with possibility 3 (note: I edited the original comment, because the sentence stopped in the middle).

[gregmarr]

It's very similar, I just made it explicit by using a different function instead of implicit with the existing function.

[markand]

What about an overload of value() that takes the expected value type as additional argument, thus:

1. If the property is missing, return the default value
2. If the property is there but not as expected type, return the default value.

Alternatively, we can just add a function that takes a json object as default value and compare the types directly from the json default value argument.

auto v = object.value("port", 123); // 123 is a json object.

// v = 123 if port is not present or not of type int.

[nlohmann]

@markand I like this approach. I'll have a look.

[nlohmann]

I implemented the proposal of @markand with a slight change: instead of having the user provide a type, the type is derived from the default value. There are two cases:

1. A value is found for the given key which matches the type of the default value; return the value at the key.
2. Return the default value otherwise.

An exception is only thrown if the function is called on a JSON value which is not an object.

There are still some issues with respect to number types: currently, -100 and 100 would be treated as different types. This may yield situations in which the return value is returned even if there is a stored value which just happens to be of the wrong number type...

The function (ignore the name for now) looks like this:

template <class ValueType, typename
          std::enable_if<
              std::is_convertible<basic_json_t, ValueType>::value
              , int>::type = 0>
ValueType value_with_type(const typename object_t::key_type& key,
                          ValueType default_value) const
{
    // derive JSON value type from given default value
    const value_t return_value_type = basic_json(default_value).type();

    // at only works for objects
    if (is_object())
    {
        // if key is found and stored value has the same type as the
        // default value, return value and given default value otherwise
        const auto it = find(key);
        if (it != end() and it->type() == return_value_type)
        {
            return *it;
        }
        else
        {
            return default_value;
        }
    }
    else
    {
        throw std::domain_error("cannot use value() with " + type_name());
    }
}

Here are some test cases:

json j =
{
    {"port", 1234}, {"proxy", true}
};

int port1 = j.value_with_type("port", 8080);
int port2 = j.value_with_type("the_port", 8080);
int port3 = j.value_with_type("proxy", 8080);

bool proxy1 = j.value_with_type("proxy", false);
bool proxy2 = j.value_with_type("the_proxy", false);
bool proxy3 = j.value_with_type("port", false);

CHECK(port1 == 1234);
CHECK(port2 == 8080);
CHECK(port3 == 8080);

CHECK(proxy1 == true);
CHECK(proxy2 == false);
CHECK(proxy3 == false);

What do you think?

• Does this solve the issue for you? @cezheng, @jackb-p, @gregmarr, @prsyahmi, @markand ?
• Should this be a separate function? If so, what should be the name?
• Should this replace the current value function?

[ghost]

This solves the issue for me. To me it doesn't matter whether it's a new function (and I don't really know whether it should be 😕)

[markand]

It is very nice.

@jackb-p a behaviour change in a public function is considered as breaking change and thus should require a major version upgrade (if following semantic versioning).

For me there is no problem if it is a new function, it's much more explicit.

[nlohmann]

I wouldn't mind putting a changed function into the 3.0.0 release. As #244 would also introduce changes to the public API, a major release should be not too far away.

[nlohmann]

Any further opinion on this?

1. Change value() to the described semantics (return the default value if the type at key does not match the type of the default value)?
2. Leave value() as is and implement the described semantics in a new function for which we would need a name?
3. Anything else? ;)

I currently prefer the first option, because the change affects a corner case that no-one really thought about in the first place.

[cezheng]

Do you mean replacing value with value_with_type's implementation? I wouldn't mind either way since all I want is a nothrow version of value though.

[clwill]

While I appreciate the semantic consistency you're stressing, I think the meaning of value() is/should be: I'll look up what you asked for (be it an individual item, float, array, whatever), and if I don't find it, I'll return the default value.

For example, I would argue that value("/this/that/7", 19) to reference an array that should also return 19 if element 7 doesn't exist, if that isn't found, or if this isn't found.

To that end, I think the only time value() should throw is in the case of a malformed pointer. In all other cases it should return a found value, or the default value.

I don't know if this is hard to implement, but my thinking is at the higher level about what the point of value() is. Maybe I'm reading in too much?

[nlohmann]

@clwill Let's not thing about how hard implementation is - it's more important to come up with semantics that do surprise the caller.

[clwill]

Then in that case, I think throwing errors should be reserved for coding issues, not data issues. That is, missing/wrong data (as in the original form of this issue -- the data type of the found object) shouldn't throw, it should return the default.

As the original poster put it:

When I'm using value to specify a default value of a key, I pretty much don't care about whether it exists or not, whether it is a valid value or not. It's pretty much like, if it failed to fetch a value of a specific type, then return the default_value I specified.

I think that's pretty much where I am too.

[gregmarr]

@clwill If the data is there but the wrong type, how do you tell whether it's a coding issue or a data issue? If the data is known to be correct, then this is a coding issue, because the caller is using the wrong type to look it up.

[clwill]

I'm confused, if the data is known to be correct, why are you using value() and not [] or at?

[gregmarr]

It can be known to be correct and still have optional entries.

[tsabirgaliev]

in reply to @nlohmann comment: #278 (comment)

Please, don't forget about a sub-case of 3: object["network"]["port"] exists and holds a string value, for instance a string "1234".

[nlohmann]

I'm sorry we discussed thus issue for so long and still did not find a solution. I just read this thread again and it seems to me that we spent a lot of time discussing the case what should happen if the type of the default value does not match the stored value if it is present. The thing is: the stored value is a JSON value, so why shouldn't the default value be a JSON value itself. The resulting function would have the same semantics as Python's get function:

get(key[, default])
Return the value for key if key is in the dictionary, else default. If default is not given, it defaults to None, so that this method never raises a KeyError.

The name get is already taken, so here would be an implementation with name get_value:

basic_json get_value(const typename object_t::key_type& key,
                     basic_json default_value = nullptr) const
{
    // get_value only works for objects
    if (is_object())
    {
        // if key is found return stored value, or default value otherwise
        const auto it = find(key);
        if (it != end())
        {
            return *it;
        }

        return default_value;
    }
    else
    {
        JSON_THROW(type_error::create(306, "cannot use get_value() with " + type_name()));
    }
}

Any opinions on this?

[theodelrieu]

I'm not sure if a default parameter is great here.

Also, do we really want to return a basic_json?
Why not using user-defined conversions?

// omitting boilerplate: U must be convertible to T
template <typename T = basic_json, typename U = void>
T get_value(const typename object_t::key_type& key, U&& default_value) const { /* ... */ }

This looks a bit like optional::value_or, and we could also add a get_or method to provide that feature for all JSON types, not just object.

EDIT: Instead of adding a get_value method, we could add a get_or overload for object types.

[nlohmann]

Thanks for the quick reply!

• I though a default parameter would be nice, because it matches the Python way. Maybe I got too used to that...
• If we don't return basic_json we may again get into troubles if the values stored value, the return value, and the default value do not match. Or am I misreading your template magic?
• I'm fine with any name. I just wanted to make a proposal for those you care less about the types without changing the original value function.

[theodelrieu]

To be honest I did not read the whole thread with scrutiny (I scrolled quite fast)

Here are some calling code I had in mind while writing this reply:

j.get_value("key", nullptr); // T is defaulted to basic_json, will return a basic_json
j.get_value<vector<string>>("key", {"default", "value"});

Since U must be convertible to T, that only leaves the question of the stored value.
I would say that when you provide a default value, you expect the method to not throw anything in case of failure, but to return the default.

So the logical conclusion (for me, at least) would be to try catch around the conversion call, and to return the default in the catch block.

However, this will simply not work for users using -fnoexceptions, this method would be a big lie to them...
I don't know if this is a real problem though, since good support for those users would require a lot of code to be rewritten using constructs like optional or expected.

[nlohmann]

Now I see what you are up to with the template parameters ;)

The second example is exactly the issue I wanted to avoid in the first place. If j["key"] is not convertible to std::vector<string>, you have an error. Try/catch is indeed not helpful if you have exceptions switched off. Therefore, I liked the Python way - and at the same time, have get_value return the same type as operator[] or at.

Another question: do you think whether it is (a) possible and (b) a good idea to implement get_value with name get?

And: I still like the idea of returning null if the value was not found - be it with a default parameter or a second function with only one parameter.

[theodelrieu]

Hmm, I gave it some thought, and I think you're right about the second example, we should throw if there is a type mismatch.

I do still believe we could use the prototype I wrote earlier, instead of restricting it to basic_json though, if the conversion fails, it's the responsability of the from/to_json implementer to throw. (We should begin a draft on those functions preconditions by the way).

About implementing get_value with get, it is possible but I don't think this would be a good idea, get operates on a json value, and giving a key as an argument feels quite weird, I would personally prefer adding another method.

However, a get_or method with a single parameter (the default value) still looks valuable to me.

[nlohmann]

As long as the version that returns basic_json does not throw, I am happy. I wonder what the others from this thread think.

[clwill]

I have changed to limp along with a "test first" strategy. Not "what's the value, apply default in case" approach but "is there value? No value? Fine, apply default. Is value? Use it." I'm building code on an embedded device and am doing my damnedest to avoid having exceptions thrown for anything other than real hard core exceptions -- such as hardware failures. This code is simply for device parameter settings, and it seems to me that throwing exceptions when a default value is missing is awfully dramatic. So, if there is any option I can get where throwing exceptions is a truly last resort kind of thing (e.g. a completely malformed request), then I'm good.

…

On Jul 9, 2017, at 12:00 PM, Niels Lohmann ***@***.***> wrote: As long as the version that returns basic_json does not throw, I am happy. I wonder what the others from this thread think. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#278 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABROJVcqK1C2I31TZa0V7qFmn7JvQigNks5sMSNbgaJpZM4JF_GV>.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[thekie]

Hello,
I just read through the post, but I don't think there is a solution for this problem here, right?

I totally agree with @nlohmann in one point:
I don't think that the default value should be returned, when the given JSON value for a key, doesn't matches the requested value. But I do think that the null value is a special case. nullis used mostly for not given, which is the same as leaving out the key completely. So I think, that the solution of returning the default value in case of nullmakes a lot of sense.

Sorry for heating up this thread, and even more if there is already a solution, but I am currently facing exactly this dilemma.

Thanks for the great library & cheers!
Daniel

[deepbluev7]

I currently have the issue, that I have to parse a lot of json from various sources. Sometimes there is a mistake in one of the sending clients and they send an integer as a string or null instead of an integer, etc. I would prefer it if there were some functionality to give me a value, if the json value is of the expected type and exists and in any error case returns me a default value. Currently there seems to be no functionality like that and I need to write my own function, that combines value(key, default) and a try catch for type errors. I would like to have a function, that can help me avoid the try catch.

[nlohmann]

What would be the signature of that function?

[deepbluev7]

The signature would basically be the same as value, taking the key and the default value. It would only differ in the behaviour, since it would treat type errors as the key not existing. I have no idea, what one would name this function though. Basically I don't want to catch the errors, when someone sends me the wrong type for an optional field, but just discard it and use the default value.

json j = R"({ "timestamp": "1234567890", "width": null })"_json;
uint64_t ts = j.value1("timestamp", 0); // just default to 0 in the error case
uint64_t width = j.value1("width", 0); // I don't care, that someone put null in the json insted of not adding the key at all

[Wambou]

It could be possible to rely on overload with a extra type as parameter.

struct NoTypeError {};

template <typename T>
T value(std::string, T default, NoTypeError);