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

fromJson should handle types with disabled default-constructor #12

Open
Zalastax opened this issue Jan 28, 2015 · 13 comments
Open

fromJson should handle types with disabled default-constructor #12

Zalastax opened this issue Jan 28, 2015 · 13 comments

Comments

@Zalastax
Copy link
Collaborator

Using https://github.com/Zalastax/zalastaxlibd/blob/master/source/notnull.d to wrap types that shouldn't be null is not supported unless the wrapped type implements _fromJSON.
fromJSON!(NotNull!(int[])) should be possible to call.
I have no idea how this should be implemented in a generic way, but lets discuss some ideas and ask the community if we get stuck.

@Zalastax
Copy link
Collaborator Author

If we check the constructor of the type T and see that it takes one argument we could do fromJSON on the type of the argument and then call the constructor on the returned value.
We might need to work out a strategy for how fromJSON should handle constructors.

@BlackEdder
Copy link
Owner

If I understand you correctly the problem is when the default/empty constructor is disabled?

It would be nice to support constructors, but indeed not clear how. Can you get the names of the variables passed to a constructor?
Point( double x, double y ) => names are x and y
Then we could do some mapping of those names to member variable names, but I do not really see how to do this in general...

Maybe we can map order of variables defines in the constructor to in the class? But not sure if that information is available.

struct Point
{
  double _x;
  double _y;
  this( double x, double y ) { _x=x; _y=y; } // Order of x,y line up with order of _x and _y
}

@Zalastax
Copy link
Collaborator Author

Yes the problem is when the default constructor is disabled, since we don't try to call other constructors. We can indeed get all overloads of the constructor using __traits(getOverloads,"__ctor").

A first step to solve this would be to use annotations to choose a function/constructor to use, just like Jackson.
The second step would be to define an order that constructors tries to get called if there are no JsonCreator(or what we call it) annotation. Here we have some decisions to make:

  • What strategy should we use for mapping the name of the json field to a constructor parameter?
    • Order of fields in the class?
    • Name of the constructor parameter should be the same as the field name? Perhaps find the name+type that matches best.
  • Which constructor should we try first?
    • Always try the one with the most arguments first?
    • First the default, then those with the most arguments first?
    • Always try the one with the least arguments first?
    • How do we handle alias this? For NotNull!T we want to do the same deserialization as if fromJSON!T and then pass that to the constructor of NotNull!T. Preferably without having to modify the implementation of NotNull.

We can't do this perfectly without inspecting the whole program, but I want to put in some effort to get the default way really good. I want to make it feel magical.
Can we figure this out together or should I seek help in the forum?

@BlackEdder
Copy link
Owner

The options that I think are correct in most cases (and therefore the most magical) are:

  • Map variables to arguments by name+type that matches the best. We can't map on exact name, because the name of the variable is normally slightly different (i.e. with _ in front) than the name of the argument (see my Point example above).
  • We can select constructor by first looking for constructor with the same number of arguments and of the same type as the member variables, if that fails use constructor with most arguments.
  • alias this is difficult, especially since multiple alias this is now supported. I think if no default constructor is present we could just check for constructors that accept T (that should work in the NotNull case).

Feel free to ask on the forum what they think. Might also result in some more interest in painlessjson :). Only risk is that it will result in loads of bike shedding. Just choosing a solution and sticking with it might be best.

@BlackEdder
Copy link
Owner

As an aside. I only just discovered tupleof, which you can use to cycle over all fields (excluding hidden fields) in order. Guess it ignores @properties

@BlackEdder
Copy link
Owner

Just noticed another jsonizer has just been added to code.dlang.org. They do work with non default constructors (and have done some work on subclassing):
http://code.dlang.org/packages/jsonizer

@Zalastax
Copy link
Collaborator Author

Zalastax commented Feb 2, 2015

I think jsonizer looks quite good. Is it best to keep working on two separate libraries or should we try to join efforts?
I am leaning towards not joining the libraries to make it a bit more competitive. I wouldn't mind keeping working on this to learn D better.

@BlackEdder
Copy link
Owner

I guess the forum post made someone realize they had written the same thing and decide to publish it.

It would be nice if we were able to merge, but I don't really want to change our API (since people might already be using it) and I think there is room for keeping both around. The licenses should even allow cross pollination.

@rcorre
Copy link

rcorre commented Mar 13, 2015

Hey, I'm the author of jsonizer, and I just found my way over here (and I actually saw the forum post shortly after I pushed). I just wanted to let you know that I'd be happy to work with you on merging, "keeping it competitive", or anything in between.

One one hand, jsonizer supports some features that are missing in painlessjson, yet a quick glance at your code suggests your implementation may be a bit cleaner internally. I was learning about D's compile-time reflection as I built jsonizer, so it definitely needs a lot of cleanup.

On the other hand, it might be interesting to see what we come up with if we keep working separately, and at some point try to combine the best of both implementations.

I just want to avoid a situation where someone is torn between two libraries each providing part of what they need.

@Zalastax
Copy link
Collaborator Author

Great!
We have already taken some ideas from your project.
It would be great if you want to contribute directly to painlessjson, but as it is now they are quite similar so it's not a problem to port the good ideas either. Personally I think it's fantastic having someone else reviewing the code before it gets released.

@BlackEdder
Copy link
Owner

Hi

On Fri, 13 Mar, 2015 at 5:22 PM, Ryan Roden-Corrent
notifications@github.com wrote:

Hey, I'm the author of jsonizer, and I just found my way over here
(and I actually saw the forum post shortly after I pushed). I just
wanted to let you know that I'd be happy to work with you on merging,
"keeping it competitive", or anything in between.

What are the actual features that differentiate the libraries
currently? If I remember correctly jsonizer has a way of chosing a
constructor using an UAD? In contrast painlessjson tries to choose the
best constructor automatically. What are the other differences?

One one hand, jsonizer supports some features that are missing in
painlessjson, yet a quick glance at your code suggests your
implementation may be a bit cleaner internally. I was learning about
D's compile-time reflection as I built jsonizer, so it definitely
needs a lot of cleanup.

painlessjson was also my first foray into compile time reflection.
Luckily @Zalastax showed up and cleaned it up quite a bit :)

I guess one thing we could try is to develop a general
(de)serialization library and then build jsonizer/painlessjson on top
of that.

Edwin

@rcorre
Copy link

rcorre commented Mar 14, 2015

painlessjson tries to choose the best constructor automatically. What are the other differences?

I didn't realize painlessjson did that. If that's true, then it does a better job than jsonizer, which picks arbitrarily if you have more than one valid match.

In that case, the only feaure jsonizer supports that painless doesn't is factory construction, which it might be what you're looking for in #8.
However, I stumbled upon a post referring to object.factory as a 'misfeature', so I'm not sure that I'm using the greatest approach here. It also requires the user to mixin JsonizeMe in every subclass, which I haven't found a workaround for (unless I give up on deserializing private fields).

A few other apparent differences based on the docs:

  • painless does not serialize private fields. jsonizer does, but requires mixin JsonizeMe in your type.
  • painless serializes by default and requires an explicit request not to serialize something, jsonizer requires you to explicitly label everything you want serialized (applies to constructors too).

I guess one thing we could try is to develop a general
(de)serialization library and then build jsonizer/painlessjson on top
of that.

I'm interested in this -- it definitely feels like most of the work I did related to inspecting types as opposed to actually doing json stuff. cerealed is another project that may be of interest here, and there is probably at least one similar library out there for xml.

Two crazy goals I had for jsonizer are serializing delegates and compile-time deserialization (akin to ctini). I'm not sure that anyone actually wants/needs those features, I just want to know if it can be done :)

@Zalastax
Copy link
Collaborator Author

I didn't realize painlessjson did that. If that's true, then it does a better job than jsonizer, which picks arbitrarily if you have more than one valid match.

I had great help from your code when implementing that feature. It looks quite similiar to your code.

I guess one thing we could try is to develop a general (de)serialization library and then build jsonizer/painlessjson on top of that.

This is a good idea in the long run, but should not be currently prioritized unless you think that's the most fun thing to work on. JSON offers a good starting point and I think the focus should be on improving the serialization parts and not on adding more supported formats.

A few other apparent differences based on the docs:

  • painless does not serialize private fields. jsonizer does, but requires mixin JsonizeMe in your type.
  • painless serializes by default and requires an explicit request not to serialize something, jsonizer requires you to explicitly label everything you want serialized (applies to constructors too).

We will need a solution like mixins to be able to access private fields. At least I am not aware of any possibility to circumvent private access like you can in C++. Perhaps using unsafe pointers and platform specific code which I wouldn't like at all.
Include all or include nothing at default could be easily configurable. Either through annotations or some settings object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants