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

feat!: throw error with out of bounds integer values, optionally wrap into DsInt or provide a custom 'integerValue' type cast options #516

Merged
merged 41 commits into from
Nov 14, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
834d9a3
integerValues decode in entity.Int object, throw error with out of bo…
AVaksman Oct 13, 2019
64bd8df
fix: system tests, samples tests
AVaksman Oct 13, 2019
5206576
address minor comments
AVaksman Oct 16, 2019
21ade5e
fix: add equal check, demonstrating auto valueof
crwilcox Oct 18, 2019
86baea7
fix: add equal check, demonstrating auto valueof for second operation
crwilcox Oct 18, 2019
8938cc3
lint-lint
AVaksman Oct 20, 2019
8d93873
Merge branch 'master' into object_numeric_typecast
AVaksman Oct 23, 2019
c9f7946
test: remove unused vars
AVaksman Oct 24, 2019
d24e515
Merge branch 'master' into object_numeric_typecast
AVaksman Oct 28, 2019
aab5bac
add and refactor with wrapNumbers option
AVaksman Oct 30, 2019
ba4a314
lint: lint
AVaksman Oct 30, 2019
536ac14
docs: add docs
AVaksman Oct 31, 2019
063777e
lint: lint
AVaksman Oct 31, 2019
a2eb3e3
revert changes in samples
AVaksman Oct 31, 2019
2c9ee4e
refactor: friendlify the error message
AVaksman Nov 4, 2019
c4d944e
Merge branch 'master' into object_numeric_typecast
AVaksman Nov 4, 2019
de30699
refactor: integerTypeCastFunction should be optional
AVaksman Nov 4, 2019
d9d0d80
docs: camel case
AVaksman Nov 4, 2019
bad51b3
refactor: clarify error message
AVaksman Nov 4, 2019
66574dc
Merge branch 'master' into object_numeric_typecast
bcoe Nov 5, 2019
aa2a4ca
refactor: bump wrapNumbers up a level
AVaksman Nov 5, 2019
c64b6fa
Merge branch 'master' into object_numeric_typecast
bcoe Nov 6, 2019
58d9767
refactor: adjust friendly error message
AVaksman Nov 6, 2019
3d4f8ba
docs: update docs
AVaksman Nov 6, 2019
7c366cc
test: test ignore if is not set
AVaksman Nov 7, 2019
f353bdf
fix: some more refactoring and docs correction
AVaksman Nov 7, 2019
08f6fa3
lint: lint
AVaksman Nov 7, 2019
96b0043
jsdoc: minor correction
AVaksman Nov 7, 2019
98f0491
Merge branch 'master' into object_numeric_typecast
crwilcox Nov 8, 2019
f856765
Merge branch 'master' into object_numeric_typecast
crwilcox Nov 10, 2019
05968bd
refactor: refactor into signle param wrapNumbersOptions: boolean | In…
AVaksman Nov 11, 2019
e59dd2d
refactor: rename wrapNumbersOptions to wrapNumbers
AVaksman Nov 11, 2019
832d31b
fix: if integerTypeCasFunction is provided API should return call it …
AVaksman Nov 11, 2019
aeda4f4
docs: more descriptions about wrapNumbers options
AVaksman Nov 11, 2019
a193365
lint: lint
AVaksman Nov 11, 2019
6b440f5
remove accidently added package
AVaksman Nov 12, 2019
ae1d249
improve error messages and correct typos
AVaksman Nov 12, 2019
ecde411
improve test for auto-call #valueOf when integerTypeCastFunction prov…
AVaksman Nov 12, 2019
db6fb32
test: split unit tests
AVaksman Nov 13, 2019
0d277c1
test: move done after assert
AVaksman Nov 13, 2019
87792e6
Merge branch 'master' into object_numeric_typecast
AVaksman Nov 13, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions samples/concepts.js
Original file line number Diff line number Diff line change
Expand Up @@ -731,8 +731,8 @@ class Query extends TestHelper {
const percentCompletes = [];
const [tasks] = await datastore.runQuery(query);
tasks.forEach(task => {
priorities.push(task.priority);
percentCompletes.push(task.percent_complete);
priorities.push(task.priority.valueOf());
percentCompletes.push(task.percent_complete.valueOf());
});

return {
Expand Down Expand Up @@ -1017,14 +1017,22 @@ class Transaction extends TestHelper {
const accounts = results.map(result => result[0]);
// Restore `datastore` to the mock API.
datastore = datastoreMock;
assert.strictEqual(
assert.equal(
accounts[0].balance,
originalBalance - amountToTransfer
);
assert.strictEqual(
crwilcox marked this conversation as resolved.
Show resolved Hide resolved
accounts[0].balance.valueOf(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a couple examples in:

https://github.com/googleapis/nodejs-datastore/blob/master/samples/concepts.js#L272

That demonstrate:

  1. how to save an entry with an integer value (I believe we already have this).
  2. how to fetch an entry with an integer value.
  3. potentially how we'd JSON stringify an entry we fetched?

I know we're already showing the conversion with accounts[0].balance.valueOf(), but I think a more self contained example might be nice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this can be done without looking odd. This sample exists alongside other language ones that are this simple.

I would be fine making a future work issue for this.

/cc: @BenWhitehead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't currently have any section in the docs discussing how to work with "large" numbers, and I'm not sure when we will. I think it'd be good to at least have a commented sample in the repo that can be discovered and referred to independent from this development PR. Once we have the sample we can add a work item to get it published into the docs.

Copy link
Contributor

@crwilcox crwilcox Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am capturing this in an internal bug b/143094618.

If you feel strongly this is needed, it may belong in a doc. comment with the function being called?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as there is something (either in samples, or in the reference docs alongside the code) so that someone can learn about this, my main concern is satisfied. Since this is a bit of an odd compatibility case that not all of the supported languages necessitate I think it'll take some design/work to incorporate into the docs based on how they're structured right now.

originalBalance - amountToTransfer
);
assert.equal(
accounts[1].balance,
originalBalance + amountToTransfer
);
assert.strictEqual(
accounts[1].balance.valueOf(),
originalBalance + amountToTransfer
);
} catch (err) {
datastore = datastoreMock;
throw err;
Expand Down
104 changes: 92 additions & 12 deletions src/entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import arrify = require('arrify');
import * as extend from 'extend';
import * as is from 'is';
import {Query, QueryProto} from './query';
import {Query, QueryProto, IntegerTypeCastOptions} from './query';
import {PathType} from '.';
import {Entities} from './request';
import * as Protobuf from 'protobufjs';
Expand Down Expand Up @@ -89,20 +89,73 @@ export namespace entity {
*
* @class
* @param {number|string} value The integer value.
* @param {object} typeCastOptions Config for custom `integerValue` cast.
* @param {function} [typeCastOptions.integerTypeCastFunction] A custom user
* provided function to convert `integerValue`.
* @param {sting|string[]} [typeCastOptions.properties] `Entity` property
* names to be converted using `integerTypeCastFunction`.
*
* @example
* const {Datastore} = require('@google-cloud/datastore');
* const datastore = new Datastore();
* const anInt = datastore.int(7);
*/
export class Int {
export class Int extends Number {
value: string;
constructor(value: number | string) {
typeCastFunction?: Function;
typeCastProperties?: string[];
private _entityPropertyName: string | undefined;
constructor(
value: number | string | ValueProto,
typeCastOptions?: IntegerTypeCastOptions
) {
super();
this._entityPropertyName =
typeof value === 'object' ? value.propertyName : undefined;
this.value =
typeof value === 'object'
? value.integerValue.toString()
: value.toString();
/**
* @name Int#value
* @type {string}
*/
this.value = value.toString();
if (typeCastOptions) {
if (typeof typeCastOptions.integerTypeCastFunction !== 'function') {
throw new Error(
`integerTypeCastFunction is not a function or was not provided.`
AVaksman marked this conversation as resolved.
Show resolved Hide resolved
);
}
this.typeCastFunction = typeCastOptions.integerTypeCastFunction;
this.typeCastProperties = typeCastOptions.properties
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fair to just do this.typeCastProperties = arrify(typeCastOptions.properties), even if it's empty 🤷‍♂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it won't work. arrify will always return an array (empty) even if typeCastOptions.properties is undefined
A few lined down the logic is

  • only custom cast those properties whose names have been specified by user. As such an empty array will signal to "not to custom cast anything as it is not provided by user".
  • an undefined typeCastOptions.properties will result in custom casting every integerType entity.property

? arrify(typeCastOptions.properties)
: undefined;
}
}
// tslint:disable-next-line no-any
valueOf(): any {
Copy link
Member

@jkwlui jkwlui Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make this type-safe for typescript users by making this class generic on the return type of valueOf():

class Int<T = number> extends Number ...

We'll also be able to make integerTypeCastFunction type-safe by making IntegerTypeCastOptions generic:

export interface IntegerTypeCastOptions<T> {
  integerTypeCastFunction: (value: string) => T;
  properties?: string | string[];
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind folks.. It's not possible to override the return type of valueOf() to the generic type since Int inherits from Number. Typescript really dislike type-hacking..

Copy link
Member

@jkwlui jkwlui Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also means Typescript users would have to coerce the type returned by valueOf():

// assume integerTypeCastFunction returns a BigInt
const bigInt = myInt.valueOf() as BigInt;

let customCast = this.typeCastFunction ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a bit more readable to ask exactly what we're wondering:

let shouldCustomCast = typeof this.typeCastFunction === 'function';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. shouldCustomCast is a better name
  2. we already throwing an error above if this.typeCastFunction is not a function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first reason I thought to suggest this change was due to the this.typeCastFunction ? true : false, which is somewhat unusual-- that is coercing the type based on if it's not undefined/null. For readability, I would rather see it say exactly what we're wondering: "We should custom cast the integer if the user gave us a function", vs "We should custom cast the integer if the user gave us a non-null argument".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.
My line of thought was, since we already weeded out anything but a function, at this point the value of this.typeCastFunction can only be a function.
But for code readability purposes it makes sense.
Fixed it.

if (
this.typeCastProperties &&
!this.typeCastProperties.includes(this._entityPropertyName!)
) {
customCast = false;
}

if (customCast) {
try {
return this.typeCastFunction!(this.value);
} catch (error) {
error.message = `integerTypeCastFunction threw an error - ${error.message}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up for debate, but newlining here might print better:

error.message = `integerTypeCastFunction returned an error:\n\n  ${error.message}`;

throw error;
}
} else {
const num = Number(this.value);
if (!Number.isSafeInteger(num)) {
throw new Error(`Integer value ${this.value} is out of bounds.`);
}
return num;
}
}
}

Expand Down Expand Up @@ -332,6 +385,11 @@ export namespace entity {
*
* @private
* @param {object} valueProto The protobuf Value message to convert.
* @param {object} typeCastOptions Config for custom `integerValue` cast.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line needs to drop one down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think typeCastOptions is optional, so it should be bracketed. And then typeCastOptions.integerTypeCastFunction is required within that object, so I don't think it should be bracketed. I'm not sure that's true, though!

* @param {function} [typeCastOptions.integerTypeCastFunction] A custom user
* provided function to convert `integerValue`.
* @param {sting|string[]} [typeCastOptions.names] `Entity` property
* names to be converted using `integerTypeCastFunction`.
* @returns {*}
*
* @example
Expand All @@ -350,13 +408,19 @@ export namespace entity {
* });
* // <Buffer 68 65 6c 6c 6f>
*/
export function decodeValueProto(valueProto: ValueProto) {
export function decodeValueProto(
valueProto: ValueProto,
typeCastOptions?: IntegerTypeCastOptions
) {
const valueType = valueProto.valueType!;
const value = valueProto[valueType];

switch (valueType) {
case 'arrayValue': {
return value.values.map(entity.decodeValueProto);
// tslint:disable-next-line no-any
return value.values.map((val: any) =>
entity.decodeValueProto(val, typeCastOptions)
);
}

case 'blobValue': {
Expand All @@ -372,7 +436,7 @@ export namespace entity {
}

case 'integerValue': {
return Number(value);
return new entity.Int(valueProto, typeCastOptions);
}

case 'entityValue': {
Expand Down Expand Up @@ -505,6 +569,11 @@ export namespace entity {
*
* @private
* @param {object} entityProto The protocol entity object to convert.
* @param {object} typeCastOptions Config for custom `integerValue` cast.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop one line, and same concerns about where to bracket to denote optionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* @param {function} [typeCastOptions.integerTypeCastFunction] A custom user
* provided function to convert `integerValue`.
* @param {sting|string[]} [typeCastOptions.names] `Entity` property
* names to be converted using `integerTypeCastFunction`.
* @returns {object}
*
* @example
Expand All @@ -525,15 +594,18 @@ export namespace entity {
* // }
*/
// tslint:disable-next-line no-any
export function entityFromEntityProto(entityProto: EntityProto): any {
export function entityFromEntityProto(
entityProto: EntityProto,
typeCastOptions?: IntegerTypeCastOptions
) {
// tslint:disable-next-line no-any
const entityObject: any = {};
const properties = entityProto.properties || {};

// tslint:disable-next-line forin
for (const property in properties) {
const value = properties[property];
entityObject[property] = entity.decodeValueProto(value);
entityObject[property] = entity.decodeValueProto(value, typeCastOptions);
}

return entityObject;
Expand Down Expand Up @@ -719,7 +791,11 @@ export namespace entity {
* @param {object[]} results The response array.
* @param {object} results.entity An entity object.
* @param {object} results.entity.key The entity's key.
* @returns {object[]}
* @param {object} typeCastOptions Config for custom `integerValue` cast.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a comment about line dropping and brackets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* @param {function} [typeCastOptions.integerTypeCastFunction] A custom user
* provided function to convert `integerValue`.
* @param {sting|string[]} [typeCastOptions.names] `Entity` property
* names to be converted using `integerTypeCastFunction`.* @returns {object[]}
*
* @example
* request_('runQuery', {}, (err, response) => {
Expand All @@ -733,9 +809,12 @@ export namespace entity {
* //
* });
*/
export function formatArray(results: ResponseResult[]) {
export function formatArray(
results: ResponseResult[],
typeCastOptions?: IntegerTypeCastOptions
) {
return results.map(result => {
const ent = entity.entityFromEntityProto(result.entity!);
const ent = entity.entityFromEntityProto(result.entity!, typeCastOptions);
ent[entity.KEY_SYMBOL] = entity.keyFromKeyProto(result.entity!.key!);
return ent;
});
Expand Down Expand Up @@ -1225,6 +1304,7 @@ export interface ValueProto {
values?: ValueProto[];
// tslint:disable-next-line no-any
value?: any;
propertyName?: string;
}

export interface EntityProto {
Expand Down
6 changes: 6 additions & 0 deletions src/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -517,9 +517,15 @@ export interface QueryProto {
*/
export {Query};

export interface IntegerTypeCastOptions {
integerTypeCastFunction: Function;
properties?: string | string[];
}

export interface RunQueryOptions {
consistency?: 'strong' | 'eventual';
gaxOptions?: CallOptions;
integerTypeCastOptions?: IntegerTypeCastOptions;
}

export interface RunQueryCallback {
Expand Down
10 changes: 5 additions & 5 deletions system-test/datastore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@ describe('Datastore', () => {
publishedAt: new Date(),
author: 'Silvano',
isDraft: false,
wordCount: 400,
rating: 5.0,
wordCount: datastore.int(400),
rating: datastore.int(5.0),
AVaksman marked this conversation as resolved.
Show resolved Hide resolved
likes: null,
metadata: {
views: 100,
views: datastore.int(100),
},
};

Expand Down Expand Up @@ -447,7 +447,7 @@ describe('Datastore', () => {
},
});
const [entity] = await datastore.get(key);
assert.strictEqual(entity.year, integerValue);
assert.strictEqual(entity.year.valueOf(), integerValue);
});

it('should save and decode a double', async () => {
Expand Down Expand Up @@ -794,7 +794,7 @@ describe('Datastore', () => {
datastore.get(key),
]);
assert.strictEqual(typeof deletedEntity, 'undefined');
assert.strictEqual(fetchedEntity.rating, 10);
assert.strictEqual(fetchedEntity.rating.valueOf(), 10);
});

it('should use the last modification to a key', async () => {
Expand Down
Loading