support ulong values for Json #1118

IgorStepanov · 2015-05-21T18:23:47Z

Hello, Vibe.D!

I've written about my problem on forum: http://forum.rejectedsoftware.com/groups/rejectedsoftware.vibed/thread/25482/

I use vibe.d on small commercial product, and two days ago I got this trouble.
One REST server (api.shodan.io) sent me a JSON like {..., serial: 17559991181826658461, ...}
I read the official JSON standard: http://json.org/
Unfortunately, it doesn't define bounds for a number value, thus 99999999999999999999999999 is a correct JSON value.
However the most real systems uses 64-bit (as maximum) signed and unsigned integers for describe integral values.
Thus, supporting of ulong is very needed for users.

I've added a new Type value: uint_, which represents ulong values.
I prefer int_ type instead of uint_ when it possible.
If the integral value x <= long.max, I write is to Json as int_ even if it hasn't the sign.

If this way is unacceptable, please suggest another way to parse ulong values.
We may convert it to long or to string.
Get an exception during parsing of correct JSON answer of a REST server is very, because I can't get the REST answer as string (using vibe.web.rest) and parse it manually.
Thanks.

Geod24 · 2015-05-21T20:50:27Z

Using ulong just pushes the limit, it doesn't really fix the problem. Me think supporting BigInt could definitely fix this. @s-ludwig ?

Geod24 · 2015-05-21T20:50:44Z

source/vibe/data/json.d

@@ -108,6 +110,7 @@ struct Json {
 		Null = null_,           /// Compatibility alias - will be deprecated soon
 		Bool = bool_,           /// Compatibility alias - will be deprecated soon
 		Int = int_,             /// Compatibility alias - will be deprecated soon
+		Uint = uint_,           /// Compatibility alias - will be deprecated soon


I doubt we need to add a compatibility alias for a new feature.

etcimon · 2015-05-21T20:52:36Z

I also vote for a BigInt here

s-ludwig · 2015-05-21T21:11:04Z

BigInt would also have the advantage of better forward compatibility with std_data_json. So I'd also be in favor of that solution.

IgorStepanov · 2015-05-21T21:28:21Z

So, you are all prefer BigInt? Ok. Should it replace the long value, or there should be both types?
In the second case, how should work parsing of int values?

s-ludwig · 2015-05-21T21:33:22Z

It should work similar to the current ulong solution. So all integral numbers fitting in long should still be stored as a long and anything bigger as a BigInt. The only issue is that the naive implementation of this (convert string to BigInt and the BigInt to long if possible) will probably have a non-trivial performance impact. But for cases where that matters, we could introduce a version (VibeJsonDisableBigInt) that uses the old string -> long parsing mode.

IgorStepanov · 2015-05-21T21:36:43Z

Ok, I'll do it.

IgorStepanov · 2015-05-22T00:10:02Z

@s-ludwig done

s-ludwig · 2015-05-22T06:16:59Z

source/vibe/data/json.d

@@ -92,28 +117,59 @@ struct Json {
 		}
 	}

+	version (VibeUseJsonBigInt) {
+		long bigIntToLong() inout


Missing private

s-ludwig · 2015-05-22T06:25:20Z

Thanks @IgorStepanov! I'm leaning towards making the version only apply to the parsing stage, but to keep general support for BigInt anyway. The current approach just seems to make it too likely to break foreign code (such as a final switch (json.type)). Thoughts?

Apart from my comments above, seems to look fine. I'll do a more thorough review later.

IgorStepanov · 2015-05-22T11:35:52Z

I'm leaning towards making the version only apply to the parsing stage, but to keep general support for BigInt anyway. The current approach just seems to make it too likely to break foreign code (such as a final switch (json.type)). Thoughts?

Ok, I've changed the code. See the second commit.
However, If we talk only about parsing, we may simply check the number length in skipNumber, and construct BigInt only if it needed.
Note, JSON doesn't support non-trivial integer literals like octal, or hex thus parsing of JSON ints is so easy.
We may simply compute result while parsing a number (num *= 10; num += (s[idx]) = '0');) and check the overflow at each step. And no special version.

marcioapm · 2015-05-22T11:50:58Z

Sorry for being late to the party, but what about supporting cent/ucent, instead? :)
At first sight it seems like a more natural extension than BigInt.

I'm not sure what the state of this is in DMD but I remember it was worked on recently for LDC.

Seems like a bad spot to be in for performance sensitive scenarios - deciding between not supporting values >= 2^63 and < 2^64 or using BigInts which require extra heap storage currently, whereas the cent would fit in the Json value's buffer - converting cent back to ulong is also pretty fast.

etcimon · 2015-05-22T12:36:55Z

https://github.com/D-Programming-Language/dmd/blob/895eb9caf2e76208b52a48231410ce27bf37f996/src/mtype.c#L318

IgorStepanov · 2015-05-22T20:24:11Z

@marcioapm cent isn't supported in dmd and it doesn't solve the general problem: json standard allows an unlimited numbers.
For the floating point numbers a trouble is less essential: all float types allow to represent any real number (-inf, +inf) with some error.
For an integer number json, current parser may reject a correct value which greater then long.max. I is not good.

IgorStepanov · 2015-05-22T20:27:19Z

@s-ludwig I've add an overflow check to skipNumber, and removed version (VibeJsonDisableBigInt):
now I build a BigInt value only if number is greater then long.max.

schuetzm · 2015-05-24T09:36:14Z

source/vibe/data/json.d

@@ -108,12 +128,12 @@ struct Json {
 		Null = null_,           /// Compatibility alias - will be deprecated soon
 		Bool = bool_,           /// Compatibility alias - will be deprecated soon
 		Int = int_,             /// Compatibility alias - will be deprecated soon
+		BigInt = bigint,       /// Compatibility alias - will be deprecated soon


This is unnecessary.

Ok, removed.

Geod24 · 2015-05-25T23:53:21Z

Could you squash your commits into a single one ?

Geod24 · 2015-05-25T23:54:56Z

examples/json/source/app.d

@@ -17,4 +17,12 @@ void main()
 	Json parent = obj;
 	parent.remove("item1");
 	foreach (i; obj) writeln(i);
+
+	version (VibeJsonDisableBigInt) {
+	} else {


This seem to be the only occurence of the version left.

My bad. I'll remove it.

IgorStepanov · 2015-05-26T00:07:46Z

Could you squash your commits into a single one ?

All commits represents different approaches. If the my last approach will be considered best, I squash commits. Otherwice, it will be much easy to rollback to previous version.

Geod24 · 2015-05-26T00:08:10Z

source/vibe/data/json.d

@@ -181,6 +199,8 @@ struct Json {
 	/// ditto
 	long opAssign(long v) { m_type = Type.int_; m_int = v; return v; }
 	/// ditto
+	BigInt opAssign(BigInt v) { m_type = Type.bigint; initBigInt(); m_bigint = v; return v; }


IDK how BigInt are implemented, but I can see the cycle: 'blitting the default value' -> 'assigning data' -> 'Do it again' as potentially problematic: if BigInt use malloced memory, it's more likely that on assignement, either malloc or free (or both) happen.

IDK how BigInt are implemented, but I can see the cycle

Default value may be only static value, because BigInt is a struct, and BigInt.init should be known at compile time.
Thus everytime, a, b and c in the following example has a identical (bitwise) value. Thus I simply write BigInt.init to the m_data.

BigInt a; BigInt b; BigInt c;

What I was refering to was, if someone calls opAssign with a BigInt value multiple time, the finalizers of all but the last 'instance' will not be run.

This case is fixed.
initBigInt() is called only if old m_type in assign is not Type.bigint
However there are another situation:
Json j = BigInt(...);
j = 5;
I may add a special destructor for it, however BigInt doesn't have ~this, I've checked id.
What do you think?

Geod24 · 2015-05-26T00:13:39Z

All commits represents different approaches. If the my last approach will be considered best, I squash commits. Otherwice, it will be much easy to rollback to previous version.

That request is just to make the history easier to understand. I often find myself blameing a file, to see when the code was introduced, and why. Having to go through various modifications for the same P.R. just eats a lot of time (same goes for history).

As per the rollback, with git, you can easily checkout a branch that points to your current commit, and squash them together. git never destroys old objects - unless nothing points to it, and they are gc'ed, which is ~ 2 weeks later, but won't happen with another branch -.

Geod24 · 2015-05-26T03:56:01Z

source/vibe/data/json.d

+	BigInt opAssign(BigInt v)
+	{
+		if (m_type != Type.bigint)
+			initBigInt();


IIUC, in the following code:

Json bi; bi = BigInt("42"); bi = 42L; bi = BigInt("42");

The bug is still present, isn't it ?

Ok, I've fixed it. (see the below description)

s-ludwig · 2015-05-26T05:36:01Z

@IgorStepanov WRT union vs void*[...], the issues I had were memory leaks, but also it broke using std.algorithm.swap (and thus std.sort etc.). void* needs to be used instead of ubyte to tell the GC to look for pointers within the array. If we'd store for example a Json[] within an ubyte[] array, it may simply get collected even if it's still referenced.

s-ludwig · 2015-05-26T05:46:37Z

@Geod24 The destructor issue is actually a good point. In fact, the same issue is in std_data_json/JSONNumber and nobody has noticed it yet (probably because it looks so natural with the union syntax). Maybe it would be worth to at least add a warning when a type with destructor is used within a union and that union is stored in a struct without destructors (same for postblit and possibly opAssign).

Also, I'll have to find the tagged union implementation for Phobos again that I had started some time ago and submit a PR. Algebraic is (not so) nice for many things, but often it's simply desirable to work with a type enum instead of TypeInfos and one shouldn't have to mess with this kind of low level stuff in regular code.

IgorStepanov · 2015-05-26T12:19:08Z

@s-ludwig @Geod24 I've fixed BigInt destruct issue.
Now when we try to call opAssign for BigInt objects, and save non-BigInt to it, finiBigInt is called.
finiBigInt simply creates an automatic BigInt init_ variable, and swaps it with the stored BigInt value.
When finiBigInt is finished, old BigInt value will be destroyed with init_, and m_bigint now contains a BigInt.init value, which can be rewritten with any non-BigInt value without any possible leaks.

s-ludwig · 2015-06-02T06:15:45Z

Looks good to me now. I'll just make some minor, mostly cosmetic, adjustments after merge. Does anyone else still see any issue?

Geod24 · 2015-06-02T23:51:46Z

Well... I look forward to std.data.json :D

s-ludwig · 2015-06-03T06:21:40Z

Good thing is that this change improves compatibility with std.data.json.

Merging now. Thanks Igor for your work!

support ulong values for Json

- Json.Type.bigint -> "bigInt" (to match "BigInt") - Move private methods to the bottom of struct Json - Move type check into finiBigInt and rename to runDestructors

s-ludwig · 2015-06-03T11:11:52Z

Okay, got the changes in. The only interface change has been to rename Type.bigint to Type.bigInt.

IgorStepanov · 2015-06-03T18:35:19Z

Okay, got the changes in. The only interface change has been to rename Type.bigint to Type.bigInt.

Thanks!
BTW, when do you plan to release the next vibe.d version (with this patch)? I mean, that when I'll able to fetch it with dub.
I use vibe.d in a commercial product, and if the next release will not be soon, I'll write temporary code to handle this issue until the next version will be released.

s-ludwig · 2015-06-03T18:50:44Z

I could tag an alpha/beta release right away.

s-ludwig · 2015-06-03T18:53:14Z

Okay 0.7.24-beta.1 is tagged now.

IgorStepanov · 2015-06-03T19:16:44Z

Okay 0.7.24-beta.1 is tagged now.

Thanks a lot! :)

MartinNowak · 2015-07-09T22:25:26Z

???
Javascript doesn't have integer types at all, it uses double-precision (64-bit) float for any Number.
math - What is JavaScript's highest integer value that a Number can go to without losing precision? - Stack Overflow
Try alert(01234567890123456789);.

s-ludwig · 2015-07-10T05:36:45Z

I don't think that this really matters, since this is JSON and not JavaScript. JSON doesn't specify anything about the numerical representation, so the implementation is basically free to choose whatever fits. Since large integers are used in practice, it would be quite arbitrary to not support them.

IgorStepanov force-pushed the ulong-json branch 2 times, most recently from 0f12262 to abfdf9a Compare May 21, 2015 18:57

Geod24 reviewed May 21, 2015
View reviewed changes

IgorStepanov force-pushed the ulong-json branch 3 times, most recently from 759fdcb to a73efec Compare May 21, 2015 23:52

s-ludwig reviewed May 22, 2015
View reviewed changes

schuetzm reviewed May 24, 2015
View reviewed changes

IgorStepanov force-pushed the ulong-json branch 2 times, most recently from 19d39e8 to 6496323 Compare May 25, 2015 21:46

Geod24 reviewed May 25, 2015
View reviewed changes

Geod24 reviewed May 26, 2015
View reviewed changes

IgorStepanov force-pushed the ulong-json branch 3 times, most recently from e9d66b6 to 02de141 Compare May 26, 2015 00:55

Geod24 reviewed May 26, 2015
View reviewed changes

BigInt support for Json

5a3224b

IgorStepanov force-pushed the ulong-json branch from 02de141 to 5a3224b Compare May 26, 2015 12:11

s-ludwig added a commit that referenced this pull request Jun 3, 2015

Merge pull request #1118 from IgorStepanov/ulong-json

309ea82

support ulong values for Json

s-ludwig merged commit 309ea82 into vibe-d:master Jun 3, 2015

s-ludwig added a commit that referenced this pull request Jun 3, 2015

Cosmetic changes. See #1118.

835f545

- Json.Type.bigint -> "bigInt" (to match "BigInt") - Move private methods to the bottom of struct Json - Move type check into finiBigInt and rename to runDestructors

s-ludwig added a commit that referenced this pull request Jun 3, 2015

Ensure that Json doesn't skip BigInt destruction/postblit. See #1118.

a373835

Geod24 mentioned this pull request Jun 8, 2015

fix BigInt initialization #1126

Merged

support ulong values for Json #1118

support ulong values for Json #1118

Conversation

IgorStepanov commented May 21, 2015

Geod24 commented May 21, 2015

Choose a reason for hiding this comment

etcimon commented May 21, 2015

s-ludwig commented May 21, 2015

IgorStepanov commented May 21, 2015

s-ludwig commented May 21, 2015

IgorStepanov commented May 21, 2015

IgorStepanov commented May 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-ludwig commented May 22, 2015

IgorStepanov commented May 22, 2015

marcioapm commented May 22, 2015

etcimon commented May 22, 2015

IgorStepanov commented May 22, 2015

IgorStepanov commented May 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 commented May 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorStepanov commented May 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 commented May 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-ludwig commented May 26, 2015

s-ludwig commented May 26, 2015

IgorStepanov commented May 26, 2015

s-ludwig commented Jun 2, 2015

Geod24 commented Jun 2, 2015

s-ludwig commented Jun 3, 2015

s-ludwig commented Jun 3, 2015

IgorStepanov commented Jun 3, 2015

s-ludwig commented Jun 3, 2015

s-ludwig commented Jun 3, 2015

IgorStepanov commented Jun 3, 2015

MartinNowak commented Jul 9, 2015

s-ludwig commented Jul 10, 2015