RowBuilder.toJSON should return un-wrapped Floats and Ints

Environment details

OS: MacOS
Node.js version: 9.2.0
npm version: 5.6.0
@google-cloud/spanner version: 0.10.0

Steps to reproduce

Make a query which returns rows containing numeric values
Run rows.map(row => row.toJSON())
The results should look something like that (field names do not matter, this is just an example):

{
  "markedAsPaid": false,
  "note": "Hello",
  "paid": false,
  "paidAt": {
    "value": "0"
   },
   "startAt": {
      "value": "1513013430444"
   },
}

As you can see, the toJSON method correctly formats all types, except for numeric values. It instead returns an object with a value property containing the desired output value. This issue might have links with #27.

Also, every RowBuilder method's documentation page is a 404 😢.

Thanks for your help 😄

PS: This issue template asks for the@google-cloud/spanner version, but GitHub tries to search a google-cloud/spanner user, and dismisses this string as it can not find one. The @ symbol should be escaped or removed 😄

Numbers stored in Spanner can go outside the boundaries of what a JavaScript Number can hold. To support this, we use custom types Spanner.int and Spanner.float.

You can see how values are treated here:

nodejs-spanner/src/codec.js

Lines 44 to 66 in dbf026e

    
           function Float(value) { 
        
             this.value = value; 
        
           } 
        
           Float.prototype.valueOf = function() { 
        
             return parseFloat(this.value); 
        
           }; 
        
           codec.Float = Float; 
        
           function Int(value) { 
        
             this.value = value.toString(); 
        
           } 
        
           Int.prototype.valueOf = function() { 
        
             var number = Number(this.value); 
        
             if (number > Number.MAX_SAFE_INTEGER) { 
        
               throw new Error('Integer ' + this.value + ' is out of bounds.'); 
        
             } 
        
             return number; 
        
           };

Regarding the docs, another team is still working on the kinks.

Thanks for the tip on the issue template.

@stephenplusplus

Thanks for your quick answer @stephenplusplus . But is this the way you want toJSON to work? I mean, it's not what you expect from a toJSON method. I'd rather expect this:

{
  "markedAsPaid": false,
  "note": "Hello",
  "paid": false,
  "paidAt": 0,
  "startAt":  1513013430444,
}

In my team, we had to implement our custom toJSON method to deal with this. Is it intended?

To be more specific, the valueOf methods on Int and Float do not get called when toJSON is called. The toString method is called instead, and returns a JSON serialization, which looks like:

{
  "value": xxxxx
}

Unfortunately, yes. If we only return a string, the JS Number characteristics are lost:

var values = row.toJSON()

values.numUsers
// { value: "100" }

++values.numUsers
// 101

We could rename the method to "row.toObject()" if using the name "toJSON()" is misleading. Although, the value types we return from the method is compliant with this interpretation/definition of how a toJSON() method is to be defined: https://derickbailey.com/2015/10/26/dont-return-a-json-document-from-the-tojson-method/

If we called valueOf() early on a row with an invalid number, this would throw:

var values = row.toJSON()
// throws number is out of bounds error

I think it's more valuable to return the values in their custom type objects, and if you need to stringify them, using your own toJSON() method is perfectly reasonable.

@stephenplusplus

I understand, thanks again for your detailed explanation @stephenplusplus.

In my opinion (and I completely understand if you and your team have a different view on this), the (vast) majority of users will handle numeric values which are not out of bounds, and will have to write a custom toJSON implementation. If you are expecting out of bounds values from your DB, you know you'll need to take care of them down the pipe anyway and write custom handlers, so I think I would encourage users facing this issue to roll out their own implementation.

Basically, with the current implementation, I feel it will be useless to every user, because you can't code the rest of your app to try to read a value property on every property of a JSON object it receives. On the contrary, using the implementation I suggest, only a small percentage would have to write a custom implementation.

Or maybe there could be 2 different helper functions? Or an option passed to toJSON to handle out of bounds values?

In our case, we use this utility function in almost all our micro-services, and we have to use our custom implementation every time, which we feel sad about. I really think there will be other people facing the same problem in the future.

I hope my explanation is clear, and again, I understand if you have a different view on this, at least we can understand the reasoning behind this 👍

@WaldoJeffers

I think @WaldoJeffers has raised a valid concern. Can we discuss what are the options here and if we can somehow make things easier for users.

toJSON taking an option to treat all numbers as JS Numbers seems brilliant. I would suggest we default to that behavior, and the option would disable it and return the custom type.

Why not add a different method for this purpose (which behaves the same as current toJson) and change toJson to treat numbers as JS numbers? That seems clearer to me than adding an option.

As an aside, we should also document clearly how does toJson handle missing field names and duplicate field names.

That seems like a matter of preference; I’m not sure I have a strong enough reason for either way. Take it to a vote? Unless there’s a clear reason a separate method is better.

Can we try to make it easy to use for the majority of users (those who will never see any large integers in responses), but still leave the possibility to deal with large integers (e.g. via the apiResponse)?

@stephenplusplus

@stephenplusplus Ok, I understand why we need a spanner.Float. But I agree with @WaldoJeffers that when converting to JSON we should just emit it as a number.

PR in progress: #84.

Navigation Menu

Repository navigation

RowBuilder.toJSON should return un-wrapped Floats and Ints #80

Environment details

Steps to reproduce

18 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	function Float(value) {
	this.value = value;
	}

	Float.prototype.valueOf = function() {
	return parseFloat(this.value);
	};

	codec.Float = Float;

	function Int(value) {
	this.value = value.toString();
	}

	Int.prototype.valueOf = function() {
	var number = Number(this.value);

	if (number > Number.MAX_SAFE_INTEGER) {
	throw new Error('Integer ' + this.value + ' is out of bounds.');
	}

	return number;
	};

RowBuilder.toJSON should return un-wrapped Floats and Ints #80

Description

Environment details

Steps to reproduce

Activity

stephenplusplus commented on Jan 3, 2018

WaldoJeffers commented on Jan 3, 2018

WaldoJeffers commented on Jan 3, 2018

stephenplusplus commented on Jan 3, 2018

WaldoJeffers commented on Jan 3, 2018

vkedia commented on Jan 4, 2018

stephenplusplus commented on Jan 4, 2018

vkedia commented on Jan 4, 2018

stephenplusplus commented on Jan 4, 2018

alexander-fenster commented on Jan 4, 2018

18 remaining items

vkedia commented on Jan 5, 2018

stephenplusplus commented on Jan 5, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions