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

map<json::value_t, string> exhibits unexpected behavior #1387

Closed
RPGillespie6 opened this issue Dec 7, 2018 · 7 comments
Closed

map<json::value_t, string> exhibits unexpected behavior #1387

RPGillespie6 opened this issue Dec 7, 2018 · 7 comments
Labels
solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)

Comments

@RPGillespie6
Copy link

  • What is the issue you have?
    map<json::value_t, string> exhibits unexpected behavior

  • Please describe the steps to reproduce the issue. Can you provide a small but working code example?

#include <iostream>
#include <json.hpp>

using namespace std;
using nlohmann::json;

static map<json::value_t, string> nlohmann_to_openapi {
    {json::value_t::null,            "null"},
    {json::value_t::boolean,         "boolean"},
    {json::value_t::string,          "string"},
    {json::value_t::number_integer,  "integer"},
    {json::value_t::number_unsigned, "integer"},
    {json::value_t::number_float,    "number"},
    {json::value_t::object,          "object"},
    {json::value_t::array,           "array"}
};

int main()
{
    cout << nlohmann_to_openapi.at(json::value_t::number_float) << endl;
}
  • What is the expected behavior?
    "number"

  • And what is the actual behavior instead?
    "integer"

  • Which compiler and operating system are you using? Is it a supported compiler?
    gcc (Ubuntu 7.3.0-16ubuntu3) 7.3.0
    Ubuntu 18.04

  • Did you use a released version of the library or the version from the develop branch?
    Tried various official released as well as the latest on master

  • If you experience a compilation error: can you compile and run the unit tests?
    n/a

@RPGillespie6 RPGillespie6 changed the title map<json::value_t, string> exhibits unexpected behavior map<json::value_t, string> exhibits unexpected behavior Dec 7, 2018
@pratikpc
Copy link
Contributor

pratikpc commented Dec 9, 2018

The issue incidentally does seem to go away when using unordered_map
The difference between unordered_map and map seems to be that one uses the < operator by default while the other doesn't

I have a feeling the issue is due to this portion of the code

inline bool operator<(const value_t lhs, const value_t rhs) noexcept
      {
         static constexpr std::array<std::uint8_t, 8> order = { {
                 0 /* null */, 3 /* object */, 4 /* array */, 5 /* string */,
                 1 /* boolean */, 2 /* integer */, 2 /* unsigned */, 2 /* float */
             }
         };

         const auto l_index = static_cast<std::size_t>(lhs);
         const auto r_index = static_cast<std::size_t>(rhs);
         return l_index < order.size() and r_index < order.size() and order[l_index] < order[r_index];
      }

Correcting the values here makes the error/issue go away.
I'll try to create a PR to correct the issue.

EDIT
PR Created at #1389

Nope I think my pull request will not work.
For some reason, json library uses the same priority values for integer, unsigned and float.
As such, there is no way to safely operate via map over it.

This is because float, integer and unsigned have the same priority value 2.
As such whenever, an object with the same priority value is inserted, rather than being considered, it is ignored.
This can be observed when you use nlohmann_to_openapi.size()
Even though there are 8 elements, it would show 6.
This is because 2 of the values with the same priority were deleted

For example, if we were to change the order/location of the place where float was written, integer starts issuing the value float

static unordered_map<json::value_t, string> nlohmann_to_openapi{
    {json::value_t::null,            "null"},
    {json::value_t::boolean,         "boolean"},
    {json::value_t::string,          "string"},
    {json::value_t::number_float,    "number"},
    {json::value_t::number_integer,  "integer"},
    {json::value_t::number_unsigned, "integer"},
    {json::value_t::object,          "object"},
    {json::value_t::array,           "array"}
};

int main()
{
   cout << nlohmann_to_openapi.at(json::value_t::number_integer) << endl;
   cout << nlohmann_to_openapi.size();
}

My fix involved keeping the priority order same as that which was present above while declaration.
However, it broke quite a few tests.
As a temporary stopgap, rather than using map you could use unordered_map

@RPGillespie6
Copy link
Author

Thanks for the tip. I ended up writing a different workaround:

string nlohmann_to_openapi(const json & j)
{
    if (j.type() == json::value_t::number_integer || j.type() == json::value_t::number_unsigned)
        return "integer";
    return j.type_name();
}

@nlohmann
Copy link
Owner

Is there anything left to do from my side? bool operator<(const value_t lhs, const value_t rhs) is motivated by Python and does not aim at being used by std::map. Sorry for the inconvenience.

@nlohmann nlohmann added the solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) label Dec 12, 2018
@RPGillespie6
Copy link
Author

That's okay. Maybe want to update the documentation somewhere to mention that type() is not a simple enum and can't be treated as such

@nlohmann
Copy link
Owner

@nlohmann
Copy link
Owner

And there are the is_XXX functions where you can also differentiate between the number types.

@RPGillespie6
Copy link
Author

Yeah, I ended up using type_name with an if clause to determine the number type. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)
Projects
None yet
Development

No branches or pull requests

3 participants