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 20 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
152 changes: 139 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 Config for custom `integerValue` cast.
* @param {boolean} [wrapNumbers=false] Indicates if the numbers should be
AVaksman marked this conversation as resolved.
Show resolved Hide resolved
* wrapped in Int wrapper.
* @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,48 @@ export namespace entity {
* @name Int#value
* @type {string}
*/
this.value = value.toString();
if (typeCastOptions) {
if (
typeCastOptions.integerTypeCastFunction &&
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 •
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 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 {
return decodeIntegerValue({
integerValue: this.value,
propertyName: this._entityPropertyName,
});
}
}

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

Expand Down Expand Up @@ -376,11 +437,44 @@ 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.integerTypeCastOptions' as\n" +
'{\n' +
'wrapNumbers: true,\n' +
'integerTypeCastFunction: <your_custom_function>\n' +
'}\n' +
'to prevent this error.'
);
}
return num;
}

/**
* Convert a protobuf Value message to its native value.
*
* @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 {boolean} [wrapNumbers=false] Indicates if the numbers should be
* wrapped in Int wrapper.
* @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`.
* @returns {*}
*
* @example
Expand All @@ -399,13 +493,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 @@ -421,11 +521,13 @@ export namespace entity {
}

case 'integerValue': {
return Number(value);
return typeCastOptions && typeCastOptions!.wrapNumbers
? new entity.Int(valueProto, typeCastOptions)
: decodeIntegerValue(valueProto);
}

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

case 'keyValue': {
Expand Down Expand Up @@ -554,6 +656,13 @@ 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 {boolean} [wrapNumbers=false] Indicates if the numbers should be
* wrapped in Int wrapper.
* @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`.
* @returns {object}
*
* @example
Expand All @@ -574,15 +683,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 @@ -768,7 +880,13 @@ 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 {boolean} [wrapNumbers=false] Indicates if the numbers should be
* wrapped in Int wrapper.
* @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
* request_('runQuery', {}, (err, response) => {
Expand All @@ -782,9 +900,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 @@ -1274,6 +1395,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 +1427,7 @@ export interface EntityObject {
data: {[k: string]: Entity};
excludeFromIndexes: string[];
}

export interface Json {
[field: string]: string;
}
18 changes: 18 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,17 @@ 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 {object} [options.integerTypeCastOptions] Configurations to
* optionally wrap `integerValue` in Datastore Int object and optionally
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 the first sentence can be changed to.

Configuration to convert values of integerValue type to a custom value.

* provide an `integerTypeCastFunction` to handle `integerValue` conversion
* @param {boolean} [options.integerTypeCastOptions.wrapNumbers=false]
* Indicates if the numbers should be wrapped in Int wrapper.
* @param {function} [options.integerTypeCastOptions.integerTypeCastFunction]
* A custom user provided function to convert `integerValue`.
* @param {sting|string[]} [options.integerTypeCastOptions.properties] `Entity`
* property names to be converted using `integerTypeCastFunction`.
* @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 +528,16 @@ export interface QueryProto {
*/
export {Query};

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

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

export interface RunQueryCallback {
Expand Down
34 changes: 27 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.integerTypeCastOptions
);
const nextKeys = (resp!.deferred || [])
.map(entity.keyFromKeyProto)
.map(entity.keyToKeyProto);
Expand Down Expand Up @@ -432,6 +435,15 @@ 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 {object} [options.integerTypeCastOptions] Configurations to
* optionally wrap `integerValue` in Datastore Int object and optionally
* provide an `integerTypeCastFunction` to handle `integerValue` conversion
* @param {boolean} [options.integerTypeCastOptions.wrapNumbers=false]
* Indicates if the numbers should be wrapped in Int wrapper.
* @param {function} [options.integerTypeCastOptions.integerTypeCastFunction]
* A custom user provided function to convert `integerValue`.
* @param {sting|string[]} [options.integerTypeCastOptions.properties] `Entity`
* property names to be converted using `integerTypeCastFunction`.
* @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 +583,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 +591,15 @@ 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 {object} [options.integerTypeCastOptions] Configurations to
* optionally wrap `integerValue` in Datastore Int object and optionally
* provide an `integerTypeCastFunction` to handle `integerValue` conversion
* @param {boolean} [options.integerTypeCastOptions.wrapNumbers=false]
* Indicates if the numbers should be wrapped in Int wrapper.
* @param {function} [options.integerTypeCastOptions.integerTypeCastFunction]
* A custom user provided function to convert `integerValue`.
* @param {sting|string[]} [options.integerTypeCastOptions.properties] `Entity`
* property names to be converted using `integerTypeCastFunction`.
* @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 +784,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.integerTypeCastOptions
);
}

// Emit each result right away, then get the rest if necessary.
Expand Down Expand Up @@ -1400,10 +1423,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