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

Add std.uuid.UUID serialization to strings for Json. #2088

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@BenjaminSchaaf
Contributor

BenjaminSchaaf commented Feb 25, 2018

Not sure about the proper approach to adding this in, but this passes the tests for me.

I personally am using UUIDs as part of a REST API and supporting this avoids a large amount of parsing I have to do manually right now.

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Mar 5, 2018

I was wondering whether it makes sense to handle UUID in a generic way in vibe.data.serialization. Unfortunately it has no generic traits that make it recognizable as a type that can be represented as a string (like e.g. SysTime has to/fromISOExtString), so it would have to be treated specially, like Nullable!T and the like. Also binary formats are of course better off storing the raw bytes. So that doesn't seem like the most promising approach.

What generally should also really be added is support for custom serialization policies in the REST interface generator, so that such custom type handling is made pluggable. I started make some first steps of a new serialization framework that is more flexible than the current one and that avoids the huge static if/else trains. That could become a candidate for Phobos inclusion, so I'd hold off on the REST integration for a while still.

I lean towards pulling this in for the next release (since the 0.8.3-rc.1 will be tagged today), because it definitely makes sense to have this as the default behavior one way or another. In the meantime, a wrapper type can avoid the manual handling in the REST API:

struct RESTUUID {
    UUID uuid;
    alias uuid this;
    static RESTUUID fromString(string v) { return RESTUUID(parseUUID(v)); }
}

static assert (isStringSerializable!RESTUUID);
@@ -198,6 +199,8 @@ struct Json {
/// ditto
this(string v) @trusted { m_type = Type.string; m_string = v; }
/// ditto
this(UUID v) { this(v.toString()); }

This comment has been minimized.

@s-ludwig

s-ludwig Mar 5, 2018

Member

The support should be restricted to JsonSerializer and JsonStringSerializer, since the current types are all unambiguously representable as JSON, while UUID is really not a JSON value type.

@BenjaminSchaaf

This comment has been minimized.

Contributor

BenjaminSchaaf commented Mar 5, 2018

I've made the requested changes and improved tests a little. Thanks for taking the time to review my PR :)

Adding serialization policy support for the REST interface generator would be great! Even better would be the ability to use your own serializer so that it becomes possible to trivially switch to bson in case json becomes a bottleneck.

@BenjaminSchaaf

This comment has been minimized.

Contributor

BenjaminSchaaf commented Mar 6, 2018

It seems like removing the this(UUID) Json constructor breaks serialization. I'm not sure if there's a better way to handle it, or if the constructor is actually required by the serialization framework.

@wilzbach wilzbach added this to the 0.8.4 milestone Apr 2, 2018

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Apr 2, 2018

Someone recently asked about this feature on the NG: https://forum.dlang.org/post/glzndvkxwxezuzmcxfqk@forum.dlang.org - so maybe this can make it to 0.8.4?

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented May 9, 2018

Okay, I've opened a rebased version of this PR (#2158) with the added modification that the constructor/assignment operator is private. The types handled by Json directly should really be restricted to the ones that can actually be stored, everything else can quickly become confusing.

I'll merge as soon as the tests pass and will then also tag a beta release.

@BenjaminSchaaf

This comment has been minimized.

Contributor

BenjaminSchaaf commented May 9, 2018

Awesome, thank you very much!

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