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

problem with new struct parsing syntax #688

Closed
ykochenkov opened this issue Aug 10, 2017 · 12 comments
Closed

problem with new struct parsing syntax #688

ykochenkov opened this issue Aug 10, 2017 · 12 comments
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@ykochenkov
Copy link

ykochenkov commented Aug 10, 2017

Hello.
We wrote a big product with the help of version 2.0.x. With this version we could write serializers like that:

EngineState::EngineState(const json& config) :
	userConfig(config.at("userConfig")),
	publicConfig(config.at("publicConfig"))
{}

EngineState::EngineState::operator json() const
{
	return{
		{ "userConfig", userConfig },
		{ "publicConfig", publicConfig },
	};
}

With the modern version of library this code is not compiled and to_json and from_json functions required.
Is there a way to fix that problem without rewriting the whole project?

@nlohmann
Copy link
Owner

Oh, this seems to be something we did not think of. Maybe @theodelrieu has an idea how to fix this. Maybe, we should search for such constructors and operators in addition to from_json and to_json functions.

@theodelrieu
Copy link
Contributor

You could use a custom serializer, I am writing one, and will post it as soon as I finish it.

@theodelrieu
Copy link
Contributor

All right it works!

First of all, you need the most specific feature of this conversion mechanism (the last layer in the onion analogy, without the crying part, at least I hope).

The easiest way to solve your problem, is to not use the default nlohmann::json, but your own.
Indeed since 2.1.0, there is an additional template parameter JSONSerializer.

I'll paste the code and explain the whole thing below, it will be easier to understand (there is meta-programming involved though):

#include <type_traits>
#include <json.hpp>

template <typename, typename> struct custom_serializer;

using json = nlohmann::basic_json<std::map,
                                  std::vector,
                                  std::string,
                                  bool,
                                  std::int64_t,
                                  std::uint64_t,
                                  double,
                                  std::allocator,
                                  custom_serializer>;

template <typename T, typename BasicJsonType>
struct has_operator_json
{
private:
  template <
      typename U,
      typename = std::enable_if_t<
          // int can be casted to json, and crashes, so only take
          // structs/classes into account
          !std::is_pod<U>::value &&
          sizeof(decltype(std::declval<U>().operator BasicJsonType())) != 0>>
  static int detect(U&&);
  static void detect(...);

public:
  static auto constexpr value =
      std::is_integral<decltype(detect(std::declval<T>()))>::value;
};

// check that T can be converted to json (with operator json())
template <typename T, typename>
struct custom_serializer
{
  // chosen when operator json() is defined in T
  template <
      typename BasicJsonType,
      typename U = T,
      std::enable_if_t<has_operator_json<U, BasicJsonType>::value, int> = 0>
  static void to_json(BasicJsonType& j, U const& val)
  {
    // cannot static_cast, it would cause an infinite loop...
    j = val.operator BasicJsonType();
  }

  // Use 2.1.1 ADL mechanism for all other types
  template <
      typename BasicJsonType,
      typename U = T,
      std::enable_if_t<not has_operator_json<U, BasicJsonType>::value, int> = 0>
  static void to_json(BasicJsonType& j, U const& val)
  {
    using nlohmann::to_json;

    to_json(j, val);
  }

  // U can be constructed with json
  // Cannot check for is_constructible, since calling basic_json::get will stack overflow
  template <
      typename BasicJsonType,
      typename U = T,
      std::enable_if_t<has_operator_json<U, BasicJsonType>::value, int> = 0>
  static U from_json(BasicJsonType const& j)
  {
    return U(j);
  }

  // Use 2.1.1 ADL mechanism for all other types (that are default-constructible)
  template <
      typename BasicJsonType,
      typename U = T,
      std::enable_if_t<not has_operator_json<U, BasicJsonType>::value, int> = 0>
  static U from_json(BasicJsonType const& j)
  {
    using nlohmann::from_json;

    U ret;
    from_json(j, ret);
    return ret;
  }
};

struct Test {
  Test() = default;
  Test(json const& j)
    : a(j.at("int").get<int>()), b(j.at("string").get<std::string>())
  {
  }

  operator json() const {
    json j;
    j["int"] = this->a;
    j["string"] = this->b;
    return j;
  }

  int a;
  std::string b;
};

int main(int argc, char const *argv[])
{
  json j = 2;
  std::cout << j << std::endl;

  Test t;
  t.a = 42;
  t.b = "forty-two";
  json j2 = t;
  std::cout << j2 << std::endl;

  std::cout << j.get<int>() << std::endl;

  auto t2 = j2.get<Test>();
  std::cout << t2.a << ", " << t2.b << std::endl;
}

As you can see we alias json to nlohmann::basic_json</* all types */, custom_serializer>, this is the change you'll need to make in your code base.

My advice is to have your own Json.hpp that contains the serializer definition and the alias,
and including this one rather than json/json.hpp. Everything should work (I didn't test thoroughly though, as you can guess from the small main).

There is a section in the Readme about this, but I'll explain more details here.

When writing your own serializer, you must:

  • Handle types that required you to write it in the first place (here, types with operator json)
  • Handle all other types

If you don't fulfill the last requirement, you will have a lot of trouble the moment you want to convert an int to json...

It is also very easy to infinitely recurse when writing this. For instance, I needed to call val.operator BasicJsonType() explicitely, instead of static_cast.
Indeed, that cast called basic_json constructor which called back custom_serializer::to_json...

I chose to write typename BasicJsonType everywhere instead of json, this is useful if you use a different alias with the same serializer. If you don't need this, you can replace BasicJsonType with json.

One last thing, this piece of code needs C++14, but can be easily converted to C++11.

std::enable_if_t</* ... */> -> typename std::enable_if</* ... */>::type

Don't hesitate to ask for more details!

@nlohmann
Copy link
Owner

@theodelrieu Wouldn't it be a good idea to add this to the library so the user can choose between either way?

@theodelrieu
Copy link
Contributor

The problem is that not one user uses the same way of converting, some have static methods in their objects, some uses operator json and constructors, etc etc. I'd rather keep adl_serializer as is, it could have undesirable effects:

Imagine someone has an object with from_json method, that they use for a specific purpose.
They could/could not want this method to be called automatically by the library.

I feel this quote of the Zen of Python is appropriate here:

In the face of ambiguity, refuse the temptation to guess.

But we could provide serializers as examples or extensions, like this one, and helping users modify those to their needs.
A cookie-cutter serializer that relies on member functions presence would be easy to add inside a serializers folder in the repo!

@nlohmann
Copy link
Owner

Imagine someone has an object with from_json method, that they use for a specific purpose. They could/could not want this method to be called automatically by the library.

This rather sounds like an argument against using the to_json/from_json functions here.

But I also understand you when you want to reduce ambiguity. Having one defined way for (de)serialization is preferable to a zoo of possibilities. I was just puzzled that we introduced compatibility-breaking behavior as we do not "find" the constructor/conversion operators.

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Aug 10, 2017
@theodelrieu
Copy link
Contributor

I poorly explained my point sorry, I was talking about some class with a from_json member function (which was poorly named, should have named it fromJson), which could be called unexpectedly, from the user point of view.

As for the mechanism introduced in 2.1.0, there was a very low risk of clash/unexpected behaviour, since the prototype of from/to_json is quite strange (returning void, taking 'json&', etc...)

I don't think we did break any behavior, or did I miss something?

@nlohmann
Copy link
Owner

nlohmann commented Aug 14, 2017

With breaking I mean that the code in #688 (comment) worked with version 2.x.x, but not with version 3.x.x of the library, and I think this is worth noting, because Foo::operator json() and Foo(const json&) are reasonable way of implementing conversions for user-defined types.

@theodelrieu
Copy link
Contributor

Oh I didn't even catch that! Actually it did break in 2.1.0.

Yep, I had not thought about that when writing tests...

I still don't know if supporting constructors is a good idea, the operator json is quite explicit about the user's intent, but I'm not entirely sure if that's the case for constructors.

Anyway this would be easy to integrate, it would require some tweaks to the huge enable_if checks on basic_json constructor and get, adl_serializer doesn't need to change.

@nlohmann
Copy link
Owner

For the constructors: In case of a non-explicit constructor which only works with a (const) reference to json, I think it is fair to assume that semantics like "construct an object from this JSON value" could be assumed.

I would really like to see this supported. Not just to avoid breaking code, but also to allow for another (in my eyes, more intuitive) way to support (de)serialization.

@theodelrieu
Copy link
Contributor

I don't know if it is possible to detect that a constructor is explicit, but this is not really an issue.

Fair enough, this might be very useful in the end.

@stale
Copy link

stale bot commented Oct 25, 2017

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 stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Oct 25, 2017
@stale stale bot closed this as completed Nov 1, 2017
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 state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

No branches or pull requests

3 participants