-
Notifications
You must be signed in to change notification settings - Fork 106
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
Changes from 31 commits
834d9a3
64bd8df
5206576
21ade5e
86baea7
8938cc3
8d93873
c9f7946
d24e515
aab5bac
ba4a314
536ac14
063777e
a2eb3e3
2c9ee4e
c4d944e
de30699
d9d0d80
bad51b3
66574dc
aa2a4ca
c64b6fa
58d9767
3d4f8ba
7c366cc
f353bdf
08f6fa3
96b0043
98f0491
f856765
05968bd
e59dd2d
832d31b
aeda4f4
a193365
6b440f5
ae1d249
ecde411
db6fb32
0d277c1
87792e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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} | ||
|
@@ -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.` | ||
); | ||
} | ||
|
||
this.typeCastProperties = typeCastOptions.properties | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fair to just do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it won't work.
|
||
? arrify(typeCastOptions.properties) | ||
: undefined; | ||
} | ||
} | ||
// tslint:disable-next-line no-any | ||
valueOf(): any { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 class Int<T = number> extends Number ... We'll also be able to make export interface IntegerTypeCastOptions<T> {
integerTypeCastFunction: (value: string) => T;
properties?: string | string[];
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind folks.. It's not possible to override the return type of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 // 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}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}; | ||
} | ||
} | ||
|
||
|
@@ -376,11 +435,49 @@ 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.wrapNumbersOptions=true' or\n" + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that you went with a dual-purpose option, where it can be a boolean, for simple mode, or an object for an advanced use case. I would rather have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed |
||
"'options.wrapNumbersOptions' 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think users will be surprised when they learn 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the logic flow you wrote above, this is my proposal:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} [wrapNumbersOptions=false] Wrap values of integerValue type in | ||
* {@link Datastore#Int} object. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. “objects.” There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
* @returns {*} | ||
* | ||
* @example | ||
|
@@ -399,13 +496,19 @@ export namespace entity { | |
* }); | ||
* // <Buffer 68 65 6c 6c 6f> | ||
*/ | ||
export function decodeValueProto(valueProto: ValueProto) { | ||
export function decodeValueProto( | ||
valueProto: ValueProto, | ||
wrapNumbersOptions?: 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, wrapNumbersOptions) | ||
); | ||
} | ||
|
||
case 'blobValue': { | ||
|
@@ -421,11 +524,18 @@ export namespace entity { | |
} | ||
|
||
case 'integerValue': { | ||
return Number(value); | ||
return wrapNumbersOptions | ||
? new entity.Int( | ||
valueProto, | ||
typeof wrapNumbersOptions === 'object' | ||
? (wrapNumbersOptions as IntegerTypeCastOptions) | ||
: undefined | ||
) | ||
: decodeIntegerValue(valueProto); | ||
} | ||
|
||
case 'entityValue': { | ||
return entity.entityFromEntityProto(value); | ||
return entity.entityFromEntityProto(value, wrapNumbersOptions); | ||
} | ||
|
||
case 'keyValue': { | ||
|
@@ -554,6 +664,8 @@ export namespace entity { | |
* | ||
* @private | ||
* @param {object} entityProto The protocol entity object to convert. | ||
* @param {boolean | IntegerTypeCastOptions} [wrapNumbersOptions=false] Wrap values of integerValue type in | ||
* {@link Datastore#Int} object. | ||
* @returns {object} | ||
* | ||
* @example | ||
|
@@ -574,15 +686,21 @@ export namespace entity { | |
* // } | ||
*/ | ||
// tslint:disable-next-line no-any | ||
export function entityFromEntityProto(entityProto: EntityProto): any { | ||
export function entityFromEntityProto( | ||
entityProto: EntityProto, | ||
wrapNumbersOptions?: 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, | ||
wrapNumbersOptions | ||
); | ||
} | ||
|
||
return entityObject; | ||
|
@@ -768,7 +886,8 @@ 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} [wrapNumbersOptions=false] Wrap values of integerValue type in | ||
* {@link Datastore#Int} object. | ||
* | ||
* @example | ||
* request_('runQuery', {}, (err, response) => { | ||
|
@@ -782,9 +901,15 @@ export namespace entity { | |
* // | ||
* }); | ||
*/ | ||
export function formatArray(results: ResponseResult[]) { | ||
export function formatArray( | ||
results: ResponseResult[], | ||
wrapNumbersOptions?: boolean | IntegerTypeCastOptions | ||
) { | ||
return results.map(result => { | ||
const ent = entity.entityFromEntityProto(result.entity!); | ||
const ent = entity.entityFromEntityProto( | ||
result.entity!, | ||
wrapNumbersOptions | ||
); | ||
ent[entity.KEY_SYMBOL] = entity.keyFromKeyProto(result.entity!.key!); | ||
return ent; | ||
}); | ||
|
@@ -1274,6 +1399,7 @@ export interface ValueProto { | |
values?: ValueProto[]; | ||
// tslint:disable-next-line no-any | ||
value?: any; | ||
propertyName?: string; | ||
} | ||
|
||
export interface EntityProto { | ||
|
@@ -1305,3 +1431,7 @@ export interface EntityObject { | |
data: {[k: string]: Entity}; | ||
excludeFromIndexes: string[]; | ||
} | ||
|
||
export interface Json { | ||
[field: string]: string; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -397,6 +397,10 @@ 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.wrapNumbersOptions=false] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The description should explain "If a boolean, this will wrap values... If an object, you can customize the behavior..." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does below sound ok?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pushed the changes 👍 |
||
* Wrap values of integerValue type in {@link Datastore#Int} object. | ||
* @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 | ||
|
@@ -517,9 +521,15 @@ export interface QueryProto { | |
*/ | ||
export {Query}; | ||
|
||
export interface IntegerTypeCastOptions { | ||
integerTypeCastFunction: Function; | ||
properties?: string | string[]; | ||
} | ||
|
||
export interface RunQueryOptions { | ||
consistency?: 'strong' | 'eventual'; | ||
gaxOptions?: CallOptions; | ||
wrapNumbersOptions?: boolean | IntegerTypeCastOptions; | ||
} | ||
|
||
export interface RunQueryCallback { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“was not provided.”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIxed!