Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

value() throws unhandled exception for partially specified json object #2393

Closed
1 of 3 tasks
bredej opened this issue Sep 10, 2020 · 13 comments
Closed
1 of 3 tasks

value() throws unhandled exception for partially specified json object #2393

bredej opened this issue Sep 10, 2020 · 13 comments
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@bredej
Copy link

bredej commented Sep 10, 2020

json::value(key, default) function throws an unhandled exception if a json object doesn't include every key value pair.
I would expect missing value(s) be taken from provided default.

struct baz {
    int bas1{ 0 };
    int bas2{ 0 };
};

json:

{
    "baz": {
        "bas1": 1
                         <--  bas2 missing
    }
}

Please describe the steps to reproduce the issue.

Link to example in Compiler Explorer

Can you provide a small but working code example?

#include <cassert>
#include <nlohmann/json.hpp>

using json = nlohmann::json;

struct baz {
    int bas1{ 0 };
    int bas2{ 0 };
};

void to_json(json& j, const baz& b) {
    j = json{ {"bas1", b.bas1}, {"bas2", b.bas2 } };
}

void from_json(const json& j, baz& b) {
    j.at("bas1").get_to(b.bas1);
    j.at("bas2").get_to(b.bas2);
}

int main() {
    json j = R"({
        "baz": {
            "bas1": 1,
            "not_bas2": 2
        }
    })"_json;

    // The following line throws an unhandled exception:
    // [json.exception.out_of_range.403] key 'bas2' not found
    baz b = j.value("baz", baz{ 10, 20 });

    assert(b.bas1 == 1);    // expected value from json
    assert(b.bas2 == 20);   // expected value from default
}

Which compiler and operating system are you using?

  • Compiler: Microsoft Visual C++ 2017
  • Operating system: Windows 10

Which version of the library did you use?

  • latest release version 3.9.1
  • other release - please state the version: 3.9.0
  • the develop branch
@dota17
Copy link
Contributor

dota17 commented Sep 21, 2020

I don't think this is a bug.
what I see from j.value("baz", baz{ 10, 20 }); is, in logically, when the key baz is in object j, the provided default value baz{10, 20} will not be used any more. This is how the function json::value(key, default) is designed.

@bredej
Copy link
Author

bredej commented Sep 21, 2020

If you know the inner workings that might be logical. But from a user’s perspective it means you can't use
json::value(key, default) for composed objects, unless you can guarantee the json file is complete.

My goal is to find an elegant way to update a configuration object (struct) with values from a configuration file.

  • The configuration object is initialized, not necessarily default constructed.
  • Values present in the configuration file should update corresponding values in the configuration object and nothing else.

For instance a new version of the program should be able to add new settings to the configuration object, while still be able to read old configuration files.

The only solution I know of that works without side-effects is to use json::find(key) for every key-value.
This becomes very repetitive code and not elegant.

@gregmarr
Copy link
Contributor

The error here is actually happening in from_json, at j.at("bas2") fails. If you want from_json to support not finding the particular value in the JSON, you'll need something like this:

void from_json(const json& j, baz& b) {
    b.bas1 = j.value("bas1", b.bas1);
    b.bas2 = j.value("bas2", b.bas2);
}

but that doesn't help with the value() case because you don't get the default value inside your from_json. The default value is only used if the baz key isn't found, not when trying to convert the JSON into a baz object. If you really want that, you might be able to do something like this if you know for sure that baz exists:

baz b = { 10, 20 };
j.at("baz").get_to(b);

@bredej
Copy link
Author

bredej commented Sep 22, 2020

I tried using json::value(key, default) inside from_json(), but it didn't go well.
You have to manually test if every instance of every custom type exist.
And if the custom type is not at the top level it becomes even worse.

@gregmarr
Copy link
Contributor

You would indeed need support for this type of processing in the from_json() of every custom type that you're using.

@cristian1980
Copy link

cristian1980 commented Sep 22, 2020

I have pretty much the same issue, I added a new config option to my settings class, and now when loading the old json files I am getting an exception because the new config option is not found in the old json.
It would be nice if in this case we could ignore missing config option in the old json and just use the default.

#include <cassert>
#include <json.hpp>

using json = nlohmann::json;

class baz
{
public:
    baz() : bas1(10), bas2(20) {}
    int bas1;
    int bas2;
    NLOHMANN_DEFINE_TYPE_INTRUSIVE(baz, bas1, bas2)
};


int main() {
    json j = R"({
            "bas1": 1,
    })"_json;

    // The following line throws an unhandled exception:
    // [json.exception.out_of_range.403] key 'bas2' not found
    baz b;
    b = j;

    assert(b.bas1 == 1);    // expected value from json
    assert(b.bas2 == 20);   // expected value from default
}

@gregmarr
Copy link
Contributor

@cristian1980 You can, as I described above, you just need to write from_json yourself and use b.bas2 = j.value("bas2", b.bas2) instead of j.at("bas2").get_to(b.bas2);.
.

class baz
{
public:
    baz() : bas1(10), bas2(20) {}
    int bas1;
    int bas2;
    friend void from_json(const json& j, baz& b) {
        b.bas1 = j.value("bas1", b.bas1);
        b.bas2 = j.value("bas2", b.bas2);
    }
};

https://gcc.godbolt.org/z/vMjq14

If you really want to keep the macros, you just need a different definition of NLOHMANN_JSON_FROM

#define MY_NLOHMANN_JSON_FROM(v1) nlohmann_json_t.v1 = nlohmann_json_j.value(#v1, nlohmann_json_t.v1);

#define MY_NLOHMANN_DEFINE_TYPE_INTRUSIVE(Type, ...)  \
    friend void to_json(nlohmann::json& nlohmann_json_j, const Type& nlohmann_json_t) { NLOHMANN_JSON_EXPAND(NLOHMANN_JSON_PASTE(NLOHMANN_JSON_TO, __VA_ARGS__)) } \
    friend void from_json(const nlohmann::json& nlohmann_json_j, Type& nlohmann_json_t) { NLOHMANN_JSON_EXPAND(NLOHMANN_JSON_PASTE(MY_NLOHMANN_JSON_FROM, __VA_ARGS__)) }

class baz
{
public:
    baz() : bas1(10), bas2(20) {}
    int bas1;
    int bas2;
    MY_NLOHMANN_DEFINE_TYPE_INTRUSIVE(baz, bas1, bas2)
};

or if you really want to keep the get_to(... for efficiency

#define MY_NLOHMANN_JSON_FROM(v1) if (nlohmann_json_j.find(#v1) != nlohmann_json_j.end()) { nlohmann_json_j.at(#v1).get_to(nlohmann_json_t.v1); }

@cristian1980
Copy link

@gregmarr Thank you, this is exactly what I needed. Maybe this should be the default mode.
The macros are really convenient, since its really easy to add new parameters.

@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation and removed kind: bug labels Sep 23, 2020
@bredej
Copy link
Author

bredej commented Sep 23, 2020

Not so fast. What if you have more than one instance of baz?
Like this https://gcc.godbolt.org/z/GKcnYr

@gregmarr
Copy link
Contributor

@bredej
Copy link
Author

bredej commented Sep 24, 2020

I consider this a valid solution.
Thank you all!

Some final thoughts:
Even though this is a valid solution it would be nice if we could avoid the trickery in from_json. Also the intrusive part could be a showstopper for some.

An idea:
Would it be possible to make the value parameter in from_json(json, value) be initialized with the corresponding part of the default value when the parsing is initiated with json::value(key, default)? Wouldn't that mean we could get rid of the json::find() and write our from_json the usual way with json::at() ?

@gregmarr
Copy link
Contributor

The current code for value() is this:

        // if key is found, return value and given default value otherwise
        const auto it = find(key);
        if (it != end())
        {
            return it->template get<ValueType>();
        }

        return default_value;

It would need to become something like this:

        // if key is found, return value and given default value otherwise
        const auto it = find(key);
        if (it != end())
        {
            auto value = default_value.
            it->template get_to<ValueType>(value);
            return value;
        }

        return default_value;

I don't think the potential extra expense of the default_value copy in the normal case would be worth what would be for most users an unexpected behavior of the library.

@bredej
Copy link
Author

bredej commented Sep 24, 2020

I guess I'm the odd one out so put a policy class to customize this behavior on my wish list :-)

@bredej bredej closed this as completed Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

5 participants