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

Why not also look inside the type for (static) to_json and from_json funtions? #511

Closed
JoveToo opened this issue Mar 17, 2017 · 23 comments
Closed
Labels
kind: enhancement/improvement state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@JoveToo
Copy link

JoveToo commented Mar 17, 2017

My knowledge of template magic is limited, so I'm probably missing something :).

Static member functions can access private and protected members.

class Type {
  ...
public:
    static void to_json(json &j, const Type &p) {...};
    static void from_json(json &j, Type &p) {...};
}

The main reason I see to declare these functions outside the scope of Type is to allow to separate serialization completely, which isn't possible because you always need the friends declaration, have getters/setters for all your internal variables or worse, have everything public.

I would even look inside Type for a serializer for std::shared_ptr<Type>.

In case a practical example helps, I am trying to serialize a template<Type> that contains map of std::shared_ptr<Type>.

// T::ptr_type is a typedef for std::shared_ptr<T>
template<typename T>
class Resolvable
{
public:
	typedef 	T	ref_type;
	Resolvable() {};
	Resolvable(ref_type r) : mReference(r) {};

	ref_type &Reference() {return mReference; };

protected:
	ref_type mReference;
};

template<typename T>
class Resolver {
public:
	Resolver()
	{
		// force T to be of Resolvable
		(void)static_cast<Resolvable<typename T::ref_type>*>((T*)0);
	}

	typename T::ptr_type &resolve(const typename T::ref_type &r)
	{
		return mEntries.at(r);
	}

	typename T::ref_type add(const typename T::ptr_type t)
	{
		typename T::ref_type r = static_cast<Resolvable<typename T::ref_type>*>(t.get())->Reference();
		if (mEntries.find(r) != mEntries.end())
			throw std::runtime_error("Duplicate entry " + std::to_string(r) + ".");

		mEntries[r] = t;

		return r;
	}

	void remove(const typename T::ref_type r)
	{
		mEntries.erase(r);
	}

	static void to_json(json &j, const Resolver<T> &resolver)
	{
		for(const std::pair<typename T::ref_type, typename T::ptr_type> &entry: resolver.mEntries)
		{
			j.push_back(*(entry.second));
		}
	};

	static void from_json(json &j, Resolver<T> &resolver)
	{
		for (json &e : j)
		{
			typename T::ptr_type ptr = std::make_shared<T>();
			T::from_json(e, *ptr);
			resolver.add(ptr);
		}
	};

private:
	std::map<typename T::ref_type, typename T::ptr_type>	mEntries;
};

The problem lies in the Resolver::from_json function: I can't find a way to express T::from_json(e, *ptr); in another way. If I do (*ptr) = e;, *ptr = e.get<T>() or ::from_json(e, *ptr);, it can't find the serialization function (and I run out of alternatives).

I have it all working by using static member functions and declaring external helpers that forward to the static ones. It works, it probably all gets optimized away, but it requires a lot of useless code.

If the serializer would check for those functions in the type, none of it would be necessary.

I realize there are probably other good solutions for my problem here (feel free to suggest them) but the static member function seems so simple a solution (for my problem at least) that I had to ask why this is not supported.

@nlohmann
Copy link
Owner

I think this is the way dropbox/json11 is doing it. @theodelrieu, any thoughts on this?

@theodelrieu
Copy link
Contributor

If I understand correctly, you could use friend here:

template <class T> class Resolver {

template <typename U>
friend void to_json(json&, Resolver<U> const&);
};

template <typename U>
void to_json(json& j, Resolver<U> const& r) {
// contains the body of your static to_json
}

I believe friend is the best way of doing it in your case.

@JoveToo
Copy link
Author

JoveToo commented Mar 17, 2017

It does fix the problem with resolving the ::from_json function. Any ideas why?

In the mean time, I can remove a lot of code by doing:

template <typename T, typename SFINAE = typename std::enable_if_v<
		std::is_function<decltype(T::from_json)>::value and
		not std::is_member_pointer<decltype(T::from_json)>::value and
		std::is_function<decltype(T::to_json)>::value and
		not std::is_member_pointer<decltype(T::to_json)>::value
		>>
inline void to_json(json& j, const T& opt) {
	T::to_json(j, opt);
}
template <typename T, typename SFINAE = typename std::enable_if_v<
		std::is_function<decltype(T::from_json)>::value and
		not std::is_member_pointer<decltype(T::from_json)>::value and
		std::is_function<decltype(T::to_json)>::value and
		not std::is_member_pointer<decltype(T::to_json)>::value
		>>
inline void from_json(const json& j, T & opt) {
	T::from_json(j, opt);
}

So the template functions will resolve for any object with static to_json and from_json functions.
Haven't found how to check the function type itself yet.

@JoveToo
Copy link
Author

JoveToo commented Mar 17, 2017

PS: the problem I had with the ::from_json function in the Resolver is because the from_json function had a non-const json argument.

@JoveToo
Copy link
Author

JoveToo commented Mar 18, 2017

I switched to:

//
//	taken from http://stackoverflow.com/questions/87372/check-if-a-class-has-a-member-function-of-a-given-signature
//

template<typename U, typename T = U>
struct has_to_json
{
    template<typename S, void (*)(json& j, const T& opt)> struct SFINAE {};
    template<typename V> static char Test(SFINAE<V, &V::to_json>*);
    template<typename V> static int Test(...);
    static const bool value = sizeof(Test<U>(0)) == sizeof(char);
};
template<typename U, typename T = U>
struct has_from_json
{
    template<typename S, void (*)(const json& j, T& opt)> struct SFINAE {};
    template<typename V> static char Test(SFINAE<V, &V::from_json>*);
    template<typename V> static int Test(...);
    static const bool value = sizeof(Test<U>(0)) == sizeof(char);
};

//
// These template functions are automatically instantiated to
//   serialize any object T with static to_json and from_json
//   member functions.
//
template <typename T, typename SFINAE = std::enable_if_t<
		has_to_json<T>::value
		>>
inline void to_json(json& j, const T& opt) {
	T::to_json(j, opt);
};

template <typename T, typename SFINAE = std::enable_if_t<
		has_from_json<T>::value
		>>
inline void from_json(const json& j, T& opt) {
	T::from_json(j, opt);
};

Seems to work well enough.

@JoveToo
Copy link
Author

JoveToo commented Mar 18, 2017

@theodelrieu : I strongly dislike friend functions. I avoid using them whenever possible, they break encapsulation. In this case, it isn't necessary, so why let json.hpp require using friend functions?

@theodelrieu
Copy link
Contributor

I disagree, friend can enforce encapsulation, but like every feature, it is often mis/overused.
(How would you implement operator<< for Resolver? Either a friend, or you add an accessor)

This library doesn't require friend functions, it's up to the user to implement the from_json free function.

In your case, you don't have a way to access Resolvers private data, your static method could also be a friend, its only purpose is to access mEntries (one cool bonus with the friend, is that it's non-intrusive).

Since Resolver only holds one attribute, this might be a hint that you need an accessor.

I prefer not to merge this into the lib, and wait for more feedback before supporting a new way to serialize.

Plus, there's some issues that need to be resolved:

  • If there's both the static method, and the free function, which one is chosen?
  • the prototypes could be changed to match adl_serializer special case (for non-default-constructible types)

@JoveToo
Copy link
Author

JoveToo commented Mar 20, 2017

The templates take care of serializing Resolver<>.

For the Resolvable<> classes, I provide an explicit (external) function that uses the exported API of a Resolvable (Reference() and resolve()).

// class Type : Resolvable<>;
inline void to_json(json& j, const Type::ptr_type & opt) {
	j = opt->Reference();
}
inline void from_json(const json& j, Type::ptr_type & opt) {
	opt = Type::resolve(j);
}

I have also generic templates to serialize pointers to my classes:

template <typename T>
inline void to_json(json& j, const std::shared_ptr<T> & opt) {
	T::to_json(j, *opt);
};
template <typename T>
inline void from_json(const json& j, std::shared_ptr<T> & opt) {
	T::from_json(j, *opt);
};

I have not yet found a way to override the generic std::shared_ptr<T> template in a meaningful way for classes that obey Resolvable except by providing explicit non-template (external or friend) to_json/from_json functions. It may be possible, my template knowledge (however much this issue has expanded it) is not up to the task :).

If there are both static methods and the free functions (as here in the std::shared_ptr<Resolvable<>> case), overload resolution will select the explicit function over the template forwarders.

I consider that an advantage: the template will act as a fallback and catch any to_json/from_json that isn't explicitly implemented and check for the static functions inside the class. Should you change your mind and add this, no existing code will break.

I do not understand what you mean with your second issue. I looked at that but as far as I understand, those also require friend statements. Perhaps it is possible to put my templates in an adl_serializer but I am unsure of overload resolution in that case and (from my point of view), I fail to see what I would win (feature-wise)?

As for the discussion about friends and not-friends, serializing a class is an action specific to that class. It requires intimate knowledge of that class and its data, both public and private and how to represent it, that alone is enough reason for me to want those functions inside the class. I am willing to agree to disagree though, this is mostly preference. As the templates show, the library doesn't even have to impose that particular preference on its users.

@theodelrieu
Copy link
Contributor

To override the behaviour of std::shared_ptr, you can take a look at the Readme, there is an example with boost::optional.

As for my second issue, the only reason the required prototype for free-functions takes a non-const reference to type T, is to make ADL work.
When specializing adl_serializer, the following prototype is allowed: T from_json(json const&).

This allows non-default constructible types to be converted.

We could improve the existing fallback mechanism, which is quite primitive right now, to support static methods (forwarding references/templates omitted):

  • static T T::from_json(json const&)
  • static json T::to_json(T const&)

Although this would be possible, I believe we should wait 3.0.0 first, since there are more important issues, and I'd like to have other people's input about this too.
We could also support non-static methods.

Anyway, thanks for your feedback!

@JoveToo
Copy link
Author

JoveToo commented Mar 21, 2017

To override the behaviour of std::shared_ptr, you can take a look at the Readme, there is an example with boost::optional.

I have already overridden the std::shared_ptr behavior. I just haven't found a way to specialize the template for Resolvable classes, I have to specify explicit functions for that. I would love to have a second template that handles them but I haven't found how to do it. I am inclined to believe it impossible.

@theodelrieu
Copy link
Contributor

Supposing you have a trait is_resolvable<>, you could do something like this:

template <typename Resolvable>
struct adl_serializer<Resolvable, std::enable_if_t<is_resolvable<Resolvable>::value>
{
  /* ... */
};

To go further, I would need a small reproducible example that I can tweak. Do you have that?

@JoveToo
Copy link
Author

JoveToo commented Mar 21, 2017

I can make one.

The problem with that approach is that (as far as I can tell), the compiler will take my generic pointer template over a template where he needs to do type conversion.

Or do the ad_serializer<> things override the global templates?

@JoveToo
Copy link
Author

JoveToo commented Mar 22, 2017

Here is the example.
testtemplate.cpp.txt

PS: the Resolver class now can add unknown entries itself. Need to find better ways of doing that though.

@theodelrieu
Copy link
Contributor

Thanks, I will take a look

@theodelrieu
Copy link
Contributor

Hi, first of all, I quite don't get the goal of your Resolvable/Resolver classes (the code is quite complicated) but here are my thoughts.

You're right, specializing the adl_serializer overrides the default behaviour:

namespace nlohmann {
template <typename T> struct adl_serializer<std::shared_ptr<T>> {
  static void to_json(json& j, const std::shared_ptr<T>& opt) {
    T::to_json(j, *opt);
  }

  static void from_json(const json& j, std::shared_ptr<T>& opt) {
    T::from_json(j, *opt);
  }
};
}

Secondly, I think most of your static methods can be removed, by providing an accessor, I didn't see real private data in your example (that might be because it is an example though)

Anyway, I'll keep your suggestion in mind, even if I won't implement it in the near future, unless other users ask for that feature.

@JoveToo
Copy link
Author

JoveToo commented Mar 24, 2017

The classes are part of a database structure. When server side processing is done, the data is serialized and sent to the client. Some records are static though and those classes are implemented as a Resolvable, the client has a cached copy of them. Since Resolvable classes only serialize their reference number, a lot less data needs to be transferred.

And yes, I removed all private data from my classes :).

Can you explain what the advantage is of using the adl_serializer over the global function templates I defined?

@theodelrieu
Copy link
Contributor

Sure, there's several advantages.

First, the library cannot find your free method void to_json(json&, std::shared_ptr<T> const&);.
That's because of Argument-Dependent Lookup, this method must be in the type's namespace, which is std in this case, and thus, it's illegal to put stuff into it.
(it is not a good idea to hijacks other libraries' namespaces too, even though it's not illegal).

In your example, it only works because you call that method explicitely, but doing something like this would fail:

// that will trigger a static_assert
auto j = json(yourObject);

Secondly, if we add an overload in the library, that handles std::shared_ptr<T>, it will cause an ambiguous call with your free function, whereas your adl_serializer specialization will always be preferred.

@JoveToo
Copy link
Author

JoveToo commented Mar 24, 2017

I don't think we are on the same page. The example as given compiles and works.

It serializes std::shared_ptr's to both normal classes and Resolvable<> classes, both correctly for their type, both without calling the functions specifically (see main), except inside the Resolver because there I want the non-default behavior (the serialization of the resolver serializes and restores the lookup table, can't use the references for that). Any other reference to T::to_json and T::from_json points to the static functions.

All templates use type T, not std::shared_ptr, so they should be found in T's namespace, not std?

They use these template functions (which match most classes in my project, only A in example):

template <typename T>
inline void to_json(json& j, const std::shared_ptr<T> & opt) {
	T::to_json(j, *opt);
};
template <typename T>
inline void from_json(const json& j, std::shared_ptr<T> & opt) {
	T::from_json(j, *opt);
};

To make the Resolvable classes work, I need to declare non-template free to_json/from_json functions for resolvable classes (type R in the example). These:

// i replaced R::ptr_type compared to the example
inline void to_json(json& j, const std::shared_ptr<R> & opt) {
	j = opt->Reference();
}
inline void from_json(const json& j, std::shared_ptr<R> & opt) {
	opt = R::resolve(j);
}

As I understand it, the compiler will find my non-template functions for the resolvable types (R in example) and just not look for templates.

I cannot find a way to turn these explicit free functions into a free template that will work correctly for Resolvable classes but not interfere with normal classes. As far as I understand it, the normal template (first in this post) keeps applying so the compiler cannot decide which one to use.

As for your second point, this code does not fail:

	A::ptr_type a;
	a = std::make_shared<A>("Class A");

	json normal;
	normal = a;

This should be equivalent to: auto j = json(yourObject); ?

So how exactly does adl_serializer influence overloading? And how would that interfere with the free functions I declared?

PS: we are straying from the initial post and this is getting quite long, so feel free to tell me to figure it out myself :)
PS: You're right that the code needs a cleanup :), especially the add functions are headaches. They check for each added object whether a copy is already present (automatically freeing the added object and returning a reference to the entry from the lookup table) or if it needs to be added. The complexity arises mostly because the added object may or may not have a reference value already.

@theodelrieu
Copy link
Contributor

I might have misread the code, sorry.

I don't have a lot of time right now, so I will simplify the explanation:

The basic_json constructor calls adl_serializer<T>::to_json, which by default looks for the free-function user-defined to_json(json&, T const&).

Thus, if you specialize adl_serializer<T>, and redefine adl_serializer<T>::to_json, the free functions will not be looked up.
We might support other types in the library (like std::shared_ptr for example), by adding a free function in the nlohmann::detail namespace.

And if a user specialized adl_serializer<std::shared_ptr<T>>, it will not conflict with the new overload we added.

I hope I'm clear enough, you can send me a message if you want more detailled explanation.

@xaxxon
Copy link

xaxxon commented Apr 3, 2017

Not sure if this is what this thread is for or not (I guess not, since it says static), but is there any way to use a virtual to_json member function in order to have the right type be automatically selected with a base type pointer/reference?

Obviously I could implement to_json to call a virtual method, but that doesn't seem a very fun solution.

@JoveToo
Copy link
Author

JoveToo commented Apr 3, 2017

This both not what the thread is about nor is it (if I understand it correctly) what this particular json library is about: argument dependent lookup, which I believe means that the correct function is determined at compile-time by the argument type you (de)serialize and not by (possibly runtime) polymorphism.

I solved my problem by implementing to_json/from_json with template functions that automatically forward to the static functions if they are present in the object. Defining those template functions was fun for me but YMMV :)

@nlohmann
Copy link
Owner

nlohmann commented Jul 8, 2017

@theodelrieu Any ideas on this or can this be closed?

@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
kind: enhancement/improvement 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

4 participants