ordered_json vs json types comparison

[akontsevich]

nlohman compares jsons as strings so comparing 2 equal nlohmann::ordered_json objects could produce wrong results:

using json = nlohmann::ordered_json; // ordered to preserve the insertion order for print
using ujson = nlohmann::json; // unordered json for testing comparison

    json j1 = { {"version", 1}, {"type", "mytype"} };
    json j2 = { {"type", "mytype"}, {"version", 1} };
    ujson j3 = ujson(j1);
    ujson j4 = ujson(j2);

    // nlohman compares jsons as strings, so...
    BOOST_CHECK_NE(j1, j2);     // these are not equal
    BOOST_CHECK_EQUAL(j3, j4);  // these are equal

So I would change operator== to this:

inline bool operator==(const nlohmann::ordered_json& lhs, const nlohmann::ordered_json& rhs) 
{
    return nlohmann::json(lhs) == nlohmann::json(rhs);
}

and add operator to compare json and nlohmann::ordered_json:

inline bool operator==(const nlohmann::json& lhs, const nlohmann::ordered_json& rhs) 
{
    return lhs == nlohmann::json(rhs);
}

and vice versa:

inline bool operator==(const nlohmann::ordered_json& lhs, const nlohmann::json& rhs) 
{
    return nlohmann::json(lhs) == rhs;
}

What do you think?

Compiler and operating system

gcc 9.4.0, Ubuntu 20.04.4 LTS

Library version

3.10.5

Validation

• The bug also occurs if the latest version from the develop branch is used.
• I can successfully compile and run the unit tests.

[falbrechtskirchinger]

Interesting. I'm working on some significant changes to the comparison code for C++20. I'll take a look at comparing different basic_json types.

There're some issues with your proposed solution. json and ordered_json are not the only instantiations of basic_json. Converting one basic_json to another may (1) be costly and (2) doesn't work in all cases due to another bug (#3425) that needs to be fixed first.

But thanks for pointing this out. I'll try to find a workable solution.

[falbrechtskirchinger]

Taking a second look, your type aliases threw me off.

using json = nlohmann::ordered_json; // ordered to preserve the insertion order for print
using ujson = nlohmann::json; // unordered json for testing comparison

    json j1 = { {"version", 1}, {"type", "mytype"} };
    json j2 = { {"type", "mytype"}, {"version", 1} };

j1 != j2 is correct and in line with the documented behavior. ordered_map stores key-value pairs in a std::vector and comparing two vectors compares element by element.

[akontsevich]

Sorry, I just used json = nlohmann::ordered_json everywhere, however for testing was forced to introduce ujson = nlohmann::json and convert types as equal objects were "not equal". I could be wrong about the way library compares json, however unit test definitely fails with ordered_json and no way to compare different json types which could be very useful.

[falbrechtskirchinger]

Sorry, I just used json = nlohmann::ordered_json everywhere, however for testing was forced to introduce ujson = nlohmann::json and convert types as equal objects were "not equal". I could be wrong about the way library compares json, however unit test definitely fails with ordered_json and no way to compare different json types which could be very useful.

So, I've just tested comparing different basic_json types and luckily there's no un-ambiguous conversion to allow that to compile, which is what I was worried about. And as I mentioned earlier, converting between basic_json types is expensive and therefore shouldn't happen implicitly (and unbeknownst to the developer) as part of a comparison.

In summary: Two ordered_json values compare as intended/documented. Comparing different basic_json types fails to compile, which is desirable.

If you insist on comparing different basic_json types in your code, either convert to a common basic_json type before comparing, or overload operators as needed yourself.

[akontsevich]

Two ordered_json values compare as intended/documented

This is not right!

[falbrechtskirchinger]

Two ordered_json values compare as intended/documented

This is not right!

Don't know what to tell you, it's right there:

https://json.nlohmann.me/api/basic_json/operator_eq/
https://json.nlohmann.me/api/ordered_map/

Sorry, 🤷.

---

Compares two JSON values for equality according to the following rules:

• Two JSON values are equal if (1) they are not discarded, (2) they are from the same type, and (3) their stored values are the same according to their respective operator==.

[akontsevich]

• Two JSON values are equal if (1) they are not discarded, (2) they are from the same type, and (3) their stored values are the same according to their respective operator==.

Then ordered_json's operator== is wrong. User expects it compares json - not an order.

[falbrechtskirchinger]

• Two JSON values are equal if (1) they are not discarded, (2) they are from the same type, and (3) their stored values are the same according to their respective operator==.

Then ordered_json's operator== is wrong. User expects it compares json - not an order.

That would be std::vector<std::pair<const basic_json::string_t, basic_json>>::operator== you're calling wrong. Difficult sell. :-)

basic_json is essentially a wrapper around STL containers (unless different types are chosen for the relevant template parameters). ordered_map is just a std::vector<std::pair<const basic_json::string_t, basic_json>>. ordered_json compares accordingly.

I also disagree that users universally expect an ordered_json to compare like json. Surely, if you're using ordered_json order matters somehow. If you don't want to compare honoring element order, either convert to json before comparing (expensive), or provide your own ordered_map that compares its elements in an order-agnostic manner.

If that's not deemed too niche, we could add an order-agnostic comparison users can opt into as a compromise?

[nlohmann]

If that's not deemed too niche, we could add an order-agnostic comparison users can opt into as a compromise?

I think it’s too niche. Maybe we should just add another note/example to the documentation.

[gregmarr]

provide your own ordered_map that compares its elements in an order-agnostic manner.

This seems like the most straightforward method. Is there anything that nlohmann::ordered_map could do to make that easier? I suspect @akontsevich could create a my_ordered_map class derived from ordered_map that just replaces operator==(const my_ordered_map &rhs) and then do using json=basic_json<my_ordered_map>.

[akontsevich]

provide your own ordered_map that compares its elements in an order-agnostic manner.

This seems like the most straightforward method. Is there anything that nlohmann::ordered_map could do to make that easier? I suspect @akontsevich could create a my_ordered_map class derived from ordered_map that just replaces operator==(const my_ordered_map &rhs) and then do using json=basic_json<my_ordered_map>.

Yes, I can do, but I would prefer this would be a part of the library. Not natural to have this in consumer project.

[falbrechtskirchinger]

provide your own ordered_map that compares its elements in an order-agnostic manner.

This seems like the most straightforward method. Is there anything that nlohmann::ordered_map could do to make that easier? I suspect @akontsevich could create a my_ordered_map class derived from ordered_map that just replaces operator==(const my_ordered_map &rhs) and then do using json=basic_json<my_ordered_map>.

Except how straightforward is it to implement in a way that is actually more efficient than converting to json and comparing those.

The way I imagine it might work is to keep a second vector of indices around. Then implement a custom iterator for sorting the indices. That iterator has to return the values stored in the map, but when swapped, has to swap the indices and not the values. A boolean flag is set after a sort and cleared whenever the map is modified. The actual comparison would then compare values in the order dictated by the indices vector, which would be the same for (content-wise) identical maps.

Something like that (with a few blanks left to fill in). Not difficult, but maybe also not straightforward.

Maybe we need a contrib folder for code that isn't worth shipping as part of the library but nevertheless useful. Other code that might go into that folder would be a better floating-point comparison algo then the abysmal suggestion in the documentation. ;-)