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 Json.toJSONValue & Json.fromJSONValue #1904

Merged
merged 1 commit into from Feb 24, 2018

Conversation

Projects
None yet
4 participants
@WebFreak001
Contributor

WebFreak001 commented Aug 28, 2017

fix #1465

But for some reason I can't get the serializer/deserializer to work. If I replace the @safe with trusted in the workaround code here serializing works. I made the JSONValue serialization exactly the same as Json serialization, just with the call to toJSONValue/fromJSONValue but it still seems to fail with seemingly unrelated issues (some strange inout issue in the deserializer). Any idea how to make the (de)serializer work?

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Sep 15, 2017

Hm, good question. I'd have to manually narrow down the cause for the error, I can't see anything in the diff that would explain it.

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Sep 15, 2017

Okay, found it. The main reason was that only the JsonStringSerializer was adjusted to support JSONValue, but the JsonSerializer was tested. The other issue was that the list of supported value types wasn't updated with JSONValue. This is the diff of my changes to make this work:

diff --git a/data/vibe/data/json.d b/data/vibe/data/json.d
index d97d263..beaea77 100644
--- a/data/vibe/data/json.d
+++ b/data/vibe/data/json.d
@@ -1707,7 +1707,7 @@ unittest { // issue #1660 - deserialize AA whose key type is string-based enum
 struct JsonSerializer {
 	template isJsonBasicType(T) { enum isJsonBasicType = std.traits.isNumeric!T || isBoolean!T || is(T == string) || is(T == typeof(null)) || isJsonSerializable!T; }
 
-	template isSupportedValueType(T) { enum isSupportedValueType = isJsonBasicType!T || is(T == Json); }
+	template isSupportedValueType(T) { enum isSupportedValueType = isJsonBasicType!T || is(T == Json) || is (T == JSONValue); }
 
 	private {
 		Json m_current;
@@ -1735,7 +1735,9 @@ struct JsonSerializer {
 	void writeValue(Traits, T)(in T value)
 		if (!is(T == Json))
 	{
-		static if (isJsonSerializable!T) {
+		static if (is(T == JSONValue)) {
+			m_current = Json.fromJSONValue(value);
+		} else static if (isJsonSerializable!T) {
 			static if (!__traits(compiles, () @safe { return value.toJson(); } ()))
 				pragma(msg, "Non-@safe toJson/fromJson methods are deprecated - annotate "~T.stringof~".toJson() with @safe.");
 			m_current = () @trusted { return value.toJson(); } ();
@@ -1780,6 +1782,7 @@ struct JsonSerializer {
 	T readValue(Traits, T)()
 	@safe {
 		static if (is(T == Json)) return m_current;
+		else static if (is(T == JSONValue)) return m_current.toJSONValue;
 		else static if (isJsonSerializable!T) {
 			static if (!__traits(compiles, () @safe { return T.fromJson(m_current); } ()))
 				pragma(msg, "Non-@safe toJson/fromJson methods are deprecated - annotate "~T.stringof~".fromJson() with @safe.");
@@ -1817,7 +1820,7 @@ struct JsonStringSerializer(R, bool pretty = false)
 
 	template isJsonBasicType(T) { enum isJsonBasicType = std.traits.isNumeric!T || isBoolean!T || is(T == string) || is(T == typeof(null)) || isJsonSerializable!T; }
 
-	template isSupportedValueType(T) { enum isSupportedValueType = isJsonBasicType!T || is(T == Json); }
+	template isSupportedValueType(T) { enum isSupportedValueType = isJsonBasicType!T || is(T == Json) || is(T == JSONValue); }
 
 	this(R range)
 	{
@@ -1866,7 +1869,7 @@ struct JsonStringSerializer(R, bool pretty = false)
 				m_range.put('"');
 			}
 			else static if (is(T == Json)) m_range.writeJsonString(value);
-			else static if (is(T == JSONValue)) m_range.writeJsonString(Json.fromJSONValue(value));
+			else static if (is(T == JSONValue)) m_range.writeJsonString(() @trusted { return Json.fromJSONValue(value); } ());
 			else static if (isJsonSerializable!T) {
 				static if (!__traits(compiles, () @safe { return value.toJson(); } ()))
 					pragma(msg, "Non-@safe toJson/fromJson methods are deprecated - annotate "~T.stringof~".toJson() with @safe.");
@@ -2536,8 +2539,8 @@ private auto trustedRange(R)(R range)
 	assert(b.foos[0].foos.length == 0);
 }
 
-@system unittest { // Json <-> std.json.JSONValue
-	auto a = parseJsonString(`{
+@safe unittest { // Json <-> std.json.JSONValue
+	auto astr = `{
 		"null": null,
 		"string": "Hello",
 		"integer": 123456,
@@ -2547,8 +2550,17 @@ private auto trustedRange(R)(R range)
 		"array": [1, 2, "string"],
 		"true": true,
 		"false": false
-	}`);
+	}`;
+	auto a = parseJsonString(astr);
+
+	// test Json <-> JSONValue conversion
 	assert(Json.fromJSONValue(a.toJSONValue) == a);
+
+	// test Json <-> JSONValue serialization
 	auto v = a.toJSONValue;
 	assert(deserializeJson!JSONValue(serializeToJson(v)) == v);
+
+	// test JSON strint <-> JSONValue serialization
+	assert(deserializeJson!JSONValue(astr) == v);
+	assert(parseJsonString(serializeToJsonString(v)) == a);
 }
@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Sep 15, 2017

Oh, by the way, I could of course simply push this change to your PR brach if you don't mind.

@WebFreak001

This comment has been minimized.

Contributor

WebFreak001 commented Oct 31, 2017

ok adjusted the code now, assumed you would just push to it

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Nov 5, 2017

Okay, thanks, I'll just push next time. I'm just careful with that and usually only do it when I'm sure that it's going to be the last commit before merging. Otherwise it's just too easy to produce a disaster when the PR author is not proficient with git and gets caught by surprise. Would be good to give this a rebase once master is finally green again (still not sure why binding to suddenly "::1" fails, but I'll disable the tests for now).

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Nov 5, 2017

Okay, master should be green again now. Can you do a quick rebase to see if this passes, too?

@WebFreak001

This comment has been minimized.

Contributor

WebFreak001 commented Nov 6, 2017

um rebase with what?

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Nov 6, 2017

Onto master, so that it incorporates the fixes.

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Nov 6, 2017

Something like git fetch && git rebase origin/master && git push --force-with-lease should do.

@WebFreak001

This comment has been minimized.

Contributor

WebFreak001 commented Nov 6, 2017

I just squashed it

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Nov 6, 2017

Thanks! Setting to auto-merge.

@s-ludwig s-ludwig added the auto-merge label Nov 6, 2017

@WebFreak001

This comment has been minimized.

Contributor

WebFreak001 commented Nov 6, 2017

looks like there is some version issue in the build thingy

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Nov 30, 2017

+echo 'Mismatch between versions.'

Mismatch between versions.

@WebFreak001 maybe simply rebasing your PR helps?

Add Json.toJSONValue & Json.fromJSONValue
fix #1465

Apply change from s-ludwig

@dlang-bot dlang-bot removed the auto-merge label Feb 23, 2018

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Feb 23, 2018

(rebaseD)

@wilzbach wilzbach added the auto-merge label Feb 23, 2018

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Feb 24, 2018

After the merge (since it's so increadibly involved to get a PR merged these days, due to CI friction), I'd change this a little bit and convert fromJSONValue to a constructor and toJSONValue to opCast, so that it's possible to later also add support for stdx.data.json.

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Feb 24, 2018

BTW not sure whether you have seen it, but I have two PRs in the pipeline to improve things:
dlang/installer#273 and dlang/installer#301
With 273 and GitHub/S3's generally good availability we could also revert to download the latest dub release by default.

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Feb 24, 2018

#301 sounds good! I'll adjust the test script accordingly when that is available. Using the bundled DUB by default in general sounds like a good idea, so I wouldn't mind having to explicitly switch to a later version.

@dlang-bot dlang-bot merged commit 9c9fe5a into vibe-d:master Feb 24, 2018

3 checks passed

codecov/patch 96.774% of diff hit (target 58.841%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@WebFreak001

This comment has been minimized.

Contributor

WebFreak001 commented Feb 24, 2018

@wilzbach aren't some arm builds missing from the dub releases on github?

s-ludwig added a commit that referenced this pull request Feb 24, 2018

Convert (to/from)JSONValue to opCast/constructor. See #1904.
This allows other similar conversions to be implemented later, in particular for `stdx.data.json`. It also enables `json.to!JSONValue` and `jsonvalue.to!JSON` conversions.

s-ludwig added a commit that referenced this pull request Feb 26, 2018

Convert (to/from)JSONValue to opCast/constructor. See #1904.
This allows other similar conversions to be implemented later, in particular for `stdx.data.json`. It also enables `json.to!JSONValue` and `jsonvalue.to!JSON` conversions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment