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.

Sign up for GitHub

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

Jump to bottom

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

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

Show file tree
Hide file tree
Changes from 35 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
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@
"google-gax": "^1.7.5",
"is": "^3.3.0",
"split-array-stream": "^2.0.0",
"stream-events": "^1.0.5"
"stream-events": "^1.0.5",
"typescript": "~3.7.0"
crwilcox marked this conversation as resolved.
Show resolved Hide resolved
},
"devDependencies": {
"@types/extend": "^3.0.1",
Expand Down
159 changes: 146 additions & 13 deletions src/entity.ts
View file
Open in desktop
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 * as Protobuf from 'protobufjs';
import * as path from 'path';
Expand Down Expand Up @@ -111,16 +111,36 @@ export namespace entity {
*
* @class
* @param {number|string} value The integer value.
* @param {object} [typeCastOptions] Configuration to convert
* values of `integerValue` type to a custom value. Must provide an
* `integerTypeCastFunction` to handle `integerValue` conversion.
* @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 {
type: string;
value: string;
constructor(value: number | string) {
typeCastFunction?: Function;
typeCastProperties?: string[];
private _entityPropertyName: string | undefined;
constructor(
value: number | string | ValueProto,
typeCastOptions?: IntegerTypeCastOptions
) {
super(typeof value === 'object' ? value.integerValue : value);
this._entityPropertyName =
typeof value === 'object' ? value.propertyName : undefined;
this.value =
typeof value === 'object'
? value.integerValue.toString()
: value.toString();
/**
* @name Int#type
* @type {string}
Expand All @@ -130,7 +150,46 @@ export namespace entity {
* @name Int#value
* @type {string}
*/
this.value = value.toString();
if (typeCastOptions) {
this.typeCastFunction = typeCastOptions.integerTypeCastFunction;
if (typeof typeCastOptions.integerTypeCastFunction !== 'function') {
throw new Error(
`integerTypeCastFunction is not a function or is not provided.`
Copy link
Contributor

Choose a reason for hiding this comment

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

“was not provided.”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIxed!

);
}

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 •
edited
Loading

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 •
edited
Loading

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 shouldCustomCast = this.typeCastFunction ? true : false;
if (
this.typeCastProperties &&
!this.typeCastProperties.includes(this._entityPropertyName!)
) {
shouldCustomCast = false;
}

if (shouldCustomCast) {
try {
return this.typeCastFunction!(this.value);
} catch (error) {
error.message = `integerTypeCastFunction threw an error:\n\n - ${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.

Could you do two spaces in front of the hyphen? I think that’s the standard newline+indent formatting we use in other places.

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

throw error;
}
} else {
return decodeIntegerValue({
integerValue: this.value,
propertyName: this._entityPropertyName,
});
}
}

toJSON(): Json {
return {type: this.type, value: this.value};
}
}

Expand Down Expand Up @@ -376,11 +435,53 @@ export namespace entity {
return value instanceof entity.Key;
}

/**
* Convert a protobuf `integerValue`.
*
* @private
* @param {object} value The `integerValue` to convert.
*/
function decodeIntegerValue(value: ValueProto) {
const num = Number(value.integerValue);
if (!Number.isSafeInteger(num)) {
throw new Error(
'We attempted to return all of the numeric values, but ' +
(value.propertyName ? value.propertyName + ' ' : '') +
'value ' +
value.integerValue +
" is out of bounds of 'Number.MAX_SAFE_INTEGER'.\n" +
"Please consider passing 'options.wrapNumbers=true' or\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you put the “To prevent this error, please...”. That way, it doesn’t float on its own line after the demonstration object.

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

"'options.wrapNumbers' as\n" +
'{\n' +
' integerTypeCastFunction: provide <your_custom_function>\n' +
' properties: optionally specify property name(s) to be cutom casted' +
'}\n' +
'to prevent this error.'
);
}
return num;
}

/**
* @typedef {object} IntegerTypeCastOptions Configuration to convert
* values of `integerValue` type to a custom value. Must provide an
* `integerTypeCastFunction` to handle `integerValue` conversion.
* @property {function} integerTypeCastFunction A custom user
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 users will be surprised when they learn integerValues are not converted to what they returned from their function until "valueOf()" is called. Imagining a scenario where a user has their own Int implementation, AppBigInt, I think they would expect instanceof entity.data.myNumber === AppBigInt rather than our DatastoreInt-- which they probably assumed they were opting out of.

If you still think having it work the way it does in this PR now would be best, let's beef up the docs to over-explain how their return value will come into play.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the logic flow you wrote above, this is my proposal:

  1. Default - query.run(q)
    • API will try to convert to Number
    • Will throw on overflow
  2. Pass options.wrapNumbers as boolean (just wrap numbers, no custom cast) - query.run(q, {wrapNumbers: true})
    • API will return a DsInt obj in which
      • #valueOf will act the same way as default behavior, i.e. try to convert to Number, throw on overflow. However now, in case of an exception with #valueOf user can still retrieve a string representation of the numerical value with #value and does not need to make another network call.
  3. Pass options.wrapNumbers as object - query.run(q, {wrapNumbers: {integerTypeCastFunction: = myFunction}})
    • API will return the return value from user-provided wrapNumbers.integerTypeCastFunction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented

* provided function to convert `integerValue`.
* @property {string | string[]} [properties] `Entity` property
* names to be converted using `integerTypeCastFunction`.
*/
/**
* Convert a protobuf Value message to its native value.
*
* @private
* @param {object} valueProto The protobuf Value message to convert.
* @param {boolean | IntegerTypeCastOptions} [wrapNumbers=false] Wrap values of integerValue type in
* {@link Datastore#Int} object.
Copy link
Contributor

Choose a reason for hiding this comment

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

“objects.”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* If a `boolean`, this will wrap values in {@link Datastore#Int}.
Copy link
Contributor

Choose a reason for hiding this comment

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

“objects.” at the end there please!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

* If an `object`, this will return a value returned by
* `wrapNumbers.integerTypeCastFunction`.
* Please see {@link IntegerTypeCastOptions} for options descriptions.
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not sure this will make a difference, but there are two spaces before ‘IntegerTypeCastOptions’.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

* @returns {*}
*
* @example
Expand All @@ -399,13 +500,19 @@ export namespace entity {
* });
* // <Buffer 68 65 6c 6c 6f>
*/
export function decodeValueProto(valueProto: ValueProto) {
export function decodeValueProto(
valueProto: ValueProto,
wrapNumbers?: boolean | 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, wrapNumbers)
);
}

case 'blobValue': {
Expand All @@ -421,11 +528,15 @@ export namespace entity {
}

case 'integerValue': {
return Number(value);
return wrapNumbers
? typeof wrapNumbers === 'object'
? new entity.Int(valueProto, wrapNumbers).valueOf()
: new entity.Int(valueProto, undefined)
: decodeIntegerValue(valueProto);
}

case 'entityValue': {
return entity.entityFromEntityProto(value);
return entity.entityFromEntityProto(value, wrapNumbers);
}

case 'keyValue': {
Expand Down Expand Up @@ -554,6 +665,12 @@ export namespace entity {
*
* @private
* @param {object} entityProto The protocol entity object to convert.
* @param {boolean | IntegerTypeCastOptions} [wrapNumbers=false] Wrap values of integerValue type in
* {@link Datastore#Int} object.
* If a `boolean`, this will wrap values in {@link Datastore#Int}.
* If an `object`, this will return a value returned by
* `wrapNumbers.integerTypeCastFunction`.
* Please see {@link IntegerTypeCastOptions} for options descriptions.
* @returns {object}
*
* @example
Expand All @@ -574,15 +691,18 @@ export namespace entity {
* // }
*/
// tslint:disable-next-line no-any
export function entityFromEntityProto(entityProto: EntityProto): any {
export function entityFromEntityProto(
entityProto: EntityProto,
wrapNumbers?: boolean | 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, wrapNumbers);
}

return entityObject;
Expand Down Expand Up @@ -768,7 +888,12 @@ 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 {boolean | IntegerTypeCastOptions} [wrapNumbers=false] Wrap values of integerValue type in
* {@link Datastore#Int} object.
* If a `boolean`, this will wrap values in {@link Datastore#Int}.
* If an `object`, this will return a value returned by
* `wrapNumbers.integerTypeCastFunction`.
* Please see {@link IntegerTypeCastOptions} for options descriptions.
*
* @example
* request_('runQuery', {}, (err, response) => {
Expand All @@ -782,9 +907,12 @@ export namespace entity {
* //
* });
*/
export function formatArray(results: ResponseResult[]) {
export function formatArray(
results: ResponseResult[],
wrapNumbers?: boolean | IntegerTypeCastOptions
) {
return results.map(result => {
const ent = entity.entityFromEntityProto(result.entity!);
const ent = entity.entityFromEntityProto(result.entity!, wrapNumbers);
ent[entity.KEY_SYMBOL] = entity.keyFromKeyProto(result.entity!.key!);
return ent;
});
Expand Down Expand Up @@ -1274,6 +1402,7 @@ export interface ValueProto {
values?: ValueProto[];
// tslint:disable-next-line no-any
value?: any;
propertyName?: string;
}

export interface EntityProto {
Expand Down Expand Up @@ -1305,3 +1434,7 @@ export interface EntityObject {
data: {[k: string]: Entity};
excludeFromIndexes: string[];
}

export interface Json {
[field: string]: string;
}
14 changes: 14 additions & 0 deletions src/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,14 @@ class Query {
* If not specified, default values are chosen by Datastore for the
* operation. Learn more about strong and eventual consistency
* [here](https://cloud.google.com/datastore/docs/articles/balancing-strong-and-eventual-consistency-with-google-cloud-datastore).
* @param {object} [options.gaxOptions] Request configuration options, outlined
* here: https://googleapis.github.io/gax-nodejs/global.html#CallOptions.
* @param {boolean | IntegerTypeCastOptions} [options.wrapNumbers=false]
* Wrap values of integerValue type in {@link Datastore#Int} object.
* If a `boolean`, this will wrap values in {@link Datastore#Int}.
* If an `object`, this will return a value returned by
* `wrapNumbers.integerTypeCastFunction`.
* Please see {@link IntegerTypeCastOptions} for options descriptions.
* @param {function} [callback] The callback function. If omitted, a readable
* stream instance is returned.
* @param {?error} callback.err An error returned while making this request
Expand Down Expand Up @@ -517,9 +525,15 @@ export interface QueryProto {
*/
export {Query};

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

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

export interface RunQueryCallback {
Expand Down
28 changes: 21 additions & 7 deletions src/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,10 @@ class DatastoreRequest {
return;
}

const entities = entity.formatArray(resp!.found! as ResponseResult[]);
const entities = entity.formatArray(
resp!.found! as ResponseResult[],
options.wrapNumbers
);
const nextKeys = (resp!.deferred || [])
.map(entity.keyFromKeyProto)
.map(entity.keyToKeyProto);
Expand Down Expand Up @@ -432,6 +435,12 @@ class DatastoreRequest {
* [here](https://cloud.google.com/datastore/docs/articles/balancing-strong-and-eventual-consistency-with-google-cloud-datastore).
* @param {object} [options.gaxOptions] Request configuration options, outlined
* here: https://googleapis.github.io/gax-nodejs/global.html#CallOptions.
* @param {boolean | IntegerTypeCastOptions} [options.wrapNumbers=false]
* Wrap values of integerValue type in {@link Datastore#Int} object.
* If a `boolean`, this will wrap values in {@link Datastore#Int}.
* If an `object`, this will return a value returned by
* `wrapNumbers.integerTypeCastFunction`.
* Please see {@link IntegerTypeCastOptions} for options descriptions.
* @param {function} callback The callback function.
* @param {?error} callback.err An error returned while making this request
* @param {object|object[]} callback.entity The entity object(s) which match
Expand Down Expand Up @@ -571,7 +580,6 @@ class DatastoreRequest {
* that uses the end cursor from the previous query as the starting cursor for
* the next query. You can pass that object back to this method to see if more
* results exist.
*
* @param {Query} query Query object.
* @param {object} [options] Optional configuration.
* @param {string} [options.consistency] Specify either `strong` or `eventual`.
Expand All @@ -580,6 +588,12 @@ class DatastoreRequest {
* [here](https://cloud.google.com/datastore/docs/articles/balancing-strong-and-eventual-consistency-with-google-cloud-datastore).
* @param {object} [options.gaxOptions] Request configuration options, outlined
* here: https://googleapis.github.io/gax-nodejs/global.html#CallOptions.
* @param {boolean | IntegerTypeCastOptions} [options.wrapNumbers=false]
* Wrap values of integerValue type in {@link Datastore#Int} object.
* If a `boolean`, this will wrap values in {@link Datastore#Int}.
* If an `object`, this will return a value returned by
* `wrapNumbers.integerTypeCastFunction`.
* Please see {@link IntegerTypeCastOptions} for options descriptions.
* @param {function} [callback] The callback function. If omitted, a readable
* stream instance is returned.
* @param {?error} callback.err An error returned while making this request
Expand Down Expand Up @@ -764,7 +778,10 @@ class DatastoreRequest {
let entities: Entity[] = [];

if (resp.batch.entityResults) {
entities = entity.formatArray(resp.batch.entityResults);
entities = entity.formatArray(
stephenplusplus marked this conversation as resolved.
Show resolved Hide resolved
resp.batch.entityResults,
options.wrapNumbers
);
}

// Emit each result right away, then get the rest if necessary.
Expand Down Expand Up @@ -1400,10 +1417,7 @@ export interface AllocateIdsOptions {
allocations?: number;
gaxOptions?: CallOptions;
}
export interface CreateReadStreamOptions {
consistency?: string;
gaxOptions?: CallOptions;
}
export interface CreateReadStreamOptions extends RunQueryOptions {}
export interface GetCallback {
(err?: Error | null, entity?: Entities): void;
}
Expand Down
Loading