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

AVaksman · 2019-10-13T08:28:31Z

Fixes #147

Tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

integerValue is now decoded into datastore.Int object

entity.Int#valueof() now

On default will try to convert to Number
- will detect if value is out of bounds of Number
Allow user to pass a custom typeCastFunction to be used to convert integer values
- Give user an option to specify property names to be converted by typeCastFunction

…unds integer values instead of truncating, add custom 'integerValue' type cast option

codecov · 2019-10-13T09:14:12Z

Codecov Report

Merging #516 into master will increase coverage by 0.19%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #516      +/-   ##
========================================
+ Coverage   93.81%    94%   +0.19%     
========================================
  Files          12     12              
  Lines         889    918      +29     
  Branches      175    189      +14     
========================================
+ Hits          834    863      +29     
  Misses         44     44              
  Partials       11     11

Impacted Files	Coverage Δ
src/query.ts	`95.83% <ø> (ø)`	⬆️
src/entity.ts	`98.84% <100%> (+0.1%)`	⬆️
src/request.ts	`99.54% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53ddc21...87792e6. Read the comment docs.

AVaksman · 2019-10-13T21:30:32Z

@bcoe please take a look.
Looking at both approaches in this PR and #488, I think this PR came a bit over complex than this feature needs to be 😄. Now I am leaning towards the first option and see add more in the future if there is a need ☺️

src/entity.ts

stephenplusplus · 2019-10-15T14:14:36Z

Should we think about forcing a user to opt-in or opt-out of the new throwing behavior? In Spanner, by default, the user will receive the Int objects. They can call row.toJSON() on the response from a query, which tries to turn everything into native values (e.g., Number(8) instead of new Int(8)). When a number is out of bounds, it will throw at this time.

The user also has the choice to provide an option, row.toJSON({wrapNumbers:true}), which will return the custom Int type, deferring any throwing until the user tries to use the out of bounds number.

In Datastore terms, we could:

Always try to return the normal Number(int) value (as opposed to new DSInt(int)), and error/throw during the .get() only when a number is out of bounds
Allow an option to get (and anywhere else) that allows wrapNumbers: true. We would always return the custom Int class, which would defer throwing until the user tries to use an out of bounds value

I like this approach, as it would make this breaking change more practical for users in both camps-- where out of bounds integers aren't relevant (no errors), as well as where they are (useful errors). It's almost a fix instead of a breaking change at that point, but I won't argue for that :)

bcoe

This is looking good to me, my only ask would be a few more targeted samples that demonstrate the new API; updating the existing conceps.js is great start.

test/entity.ts

bcoe · 2019-10-18T19:32:06Z

samples/concepts.js

@@ -1018,11 +1018,11 @@ class Transaction extends TestHelper {
      // Restore `datastore` to the mock API.
      datastore = datastoreMock;
      assert.strictEqual(
-        accounts[0].balance,
+        accounts[0].balance.valueOf(),


Could we add a couple examples in:

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

That demonstrate:

how to save an entry with an integer value (I believe we already have this).

how to fetch an entry with an integer value.

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.

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

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.

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?

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.

samples/concepts.js

stephenplusplus

Just wanted to hold this to see if #516 (comment) can turn into a discussion :)

system-test/datastore.ts

jkwlui · 2019-10-21T21:15:53Z

src/entity.ts

+      }
+    }
+    // tslint:disable-next-line no-any
+    valueOf(): any {


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[]; }

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..

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;

test/entity.ts

src/entity.ts

stephenplusplus · 2019-11-06T21:24:26Z

src/entity.ts

+          );
+        }
+        this.typeCastFunction = typeCastOptions.integerTypeCastFunction;
+        this.typeCastProperties = typeCastOptions.properties


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

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

stephenplusplus · 2019-11-06T21:25:14Z

src/entity.ts

+    }
+    // tslint:disable-next-line no-any
+    valueOf(): any {
+      let customCast = this.typeCastFunction ? true : false;


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

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

shouldCustomCast is a better name

we already throwing an error above if this.typeCastFunction is not a function.

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".

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.

stephenplusplus · 2019-11-06T21:29:15Z

src/entity.ts

+        try {
+          return this.typeCastFunction!(this.value);
+        } catch (error) {
+          error.message = `integerTypeCastFunction threw an error - ${error.message}`;


Up for debate, but newlining here might print better:

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

stephenplusplus · 2019-11-06T21:50:55Z

src/query.ts

+   * @param {boolean} [options.wrapNumbers=false]
+   *     Indicates if the numbers should be wrapped in Int wrapper.
+   * @param {object} [options.integerTypeCastOptions] Configurations to
+   *     optionally wrap `integerValue` in Datastore Int object and optionally


I think the first sentence can be changed to.

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

stephenplusplus · 2019-11-06T21:51:11Z

src/query.ts

+   * @param {object} [options.integerTypeCastOptions] Configurations to
+   *     optionally wrap `integerValue` in Datastore Int object and optionally
+   *     provide an `integerTypeCastFunction` to handle `integerValue` conversion.
+   *     Note: `integerTypeCastingOptions` values will be ingnored


Note: integerTypeCastOptions is ignored when options.wrapNumbers is set.

@crwilcox I added a few tests in the commit 7c366cc to test the "ignore integerTypeCastOptions if options.wrapNumbers is not set to true"

src/query.ts

stephenplusplus · 2019-11-06T21:54:50Z

src/request.ts

@@ -432,6 +436,17 @@ 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} [options.wrapNumbers=false]


Same notes as above throughout this documentation block.

stephenplusplus · 2019-11-06T21:55:05Z

src/request.ts

@@ -580,6 +594,17 @@ 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} [options.wrapNumbers=false]


Same notes about documentation for this and the properties that follow.

crwilcox · 2019-11-09T02:20:52Z

@stephenplusplus ptal.

…tegerTypeCastOptions

AVaksman · 2019-11-11T18:31:08Z

@stephenplusplus
Latest modifications

combine wrapNumbers: bool and iTCO: iTCO parameters into one wrapNumbersOptions: boolean | iTCO (defaults to false) param.

With new changes user can

Default - query.run(q)
- API will try to convert to Number
- Will throw on overflow
Pass options.wNO = true (just wrap numbers, no custom cast) - query.run(q, {wrapNumbersOptions: 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.
Pass options.wNO = iTCO - query.run(q, {wrapNumbersOptions: {iTCF = myFunction}})
- API will return a DsInt obj in which
  - #valueOf API will use iTCF to convert the numeric values.
  - #value still returns the string representation of the numerical value

PTAL

stephenplusplus · 2019-11-11T18:33:36Z

src/entity.ts

+          'value ' +
+          value.integerValue +
+          " is out of bounds of 'Number.MAX_SAFE_INTEGER'.\n" +
+          "Please consider passing 'options.wrapNumbersOptions=true' or\n" +


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 wrapNumbers be the name, however, so that wrapNumbers: true is possible, compared to wrapNumbersOptions: true. When the object is used, wrapNumbers = {integerTypeCastFunction: () => {}} still seems fine.

stephenplusplus · 2019-11-11T18:39:51Z

src/entity.ts

+   * @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


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.

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

Default - query.run(q)

API will try to convert to Number

Will throw on overflow

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.

Pass options.wrapNumbers as object - query.run(q, {wrapNumbers: {integerTypeCastFunction: = myFunction}})

API will return the return value from user-provided wrapNumbers.integerTypeCastFunction

Implemented

stephenplusplus · 2019-11-11T18:41:38Z

src/query.ts

@@ -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]


The description should explain "If a boolean, this will wrap values... If an object, you can customize the behavior..."

Does below sound ok?

* @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.

Pushed the changes 👍

…by default vs just returning DsInt

package.json

stephenplusplus · 2019-11-12T15:15:10Z

src/entity.ts

+          'value ' +
+          value.integerValue +
+          " is out of bounds of 'Number.MAX_SAFE_INTEGER'.\n" +
+          "Please consider passing 'options.wrapNumbers=true' or\n" +


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

stephenplusplus · 2019-11-12T15:16:21Z

src/entity.ts

+        try {
+          return this.typeCastFunction!(this.value);
+        } catch (error) {
+          error.message = `integerTypeCastFunction threw an error:\n\n - ${error.message}`;


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

stephenplusplus · 2019-11-12T15:17:08Z

src/entity.ts

  /**
   * 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.


“objects.”

stephenplusplus · 2019-11-12T15:17:57Z

src/entity.ts

  /**
   * 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.
+   *     If a `boolean`, this will wrap values in {@link Datastore#Int}.


“objects.” at the end there please!

stephenplusplus · 2019-11-12T15:22:06Z

test/entity.ts

+          const wrapNumbers = {integerTypeCastFunction: stub};
+
+          entity.decodeValueProto(valueProto, wrapNumbers);
+          assert.strictEqual(stub.called, true);


Let’s also check that the value returned is what the iTCF returned.

added functionality to integerTypeCastFunction
added an assert to verify entity.decodeValueProto returnes the value returned by integerTypeCastFunction

stephenplusplus · 2019-11-12T15:24:05Z

src/entity.ts

+        this.typeCastFunction = typeCastOptions.integerTypeCastFunction;
+        if (typeof typeCastOptions.integerTypeCastFunction !== 'function') {
+          throw new Error(
+            `integerTypeCastFunction is not a function or is not provided.`


“was not provided.”

stephenplusplus · 2019-11-12T15:24:31Z

test/entity.ts

+        assert.strictEqual(entity.decodeValueProto(valueProto), expectedValue);
+      });
+
+      it('should wrap nubers with an option', () => {


“numbers”

stephenplusplus · 2019-11-12T15:31:18Z

test/entity.ts

+        assert.strictEqual(entity.decodeValueProto(valueProto), expectedValue);
+      });
+
+      it('should not wrap nubers by default', () => {


“numbers”

Fixed as well!

stephenplusplus · 2019-11-12T15:41:15Z

test/request.ts

@@ -456,6 +456,53 @@ describe('Request', () => {
          .emit('reading');
      });

+      it('should pass `wrapNumbersOprions` to formatArray', done => {


I think these tests should be rewritten to assure all the asserts are called. If necessary, break them apart into multiple tests for each value type.

Split all these tests into single test per each value.

…ided

AVaksman · 2019-11-13T18:22:47Z

@stephenplusplus
I believe I addressed all the most resent comments PTAL
You can review per commit.

stephenplusplus · 2019-11-14T01:42:54Z

Thank you very much for your patience with my reviews! It looks great to me!

integerValues decode in entity.Int object, throw error with out of bo…

834d9a3

…unds integer values instead of truncating, add custom 'integerValue' type cast option

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 13, 2019

fix: system tests, samples tests

64bd8df

JustinBeckwith requested review from stephenplusplus and bcoe October 13, 2019 19:45

AVaksman mentioned this pull request Oct 13, 2019

feat!: throw error with out of bounds integer values instead of truncating, add custom 'integerValue' type cast option #488

Closed

3 tasks

stephenplusplus reviewed Oct 15, 2019

View reviewed changes

address minor comments

5206576

bcoe reviewed Oct 18, 2019

View reviewed changes

samples/concepts.js Outdated Show resolved Hide resolved

crwilcox added 2 commits October 18, 2019 15:58

fix: add equal check, demonstrating auto valueof

21ade5e

fix: add equal check, demonstrating auto valueof for second operation

86baea7

crwilcox marked this pull request as ready for review October 18, 2019 23:10

crwilcox approved these changes Oct 18, 2019

View reviewed changes

lint-lint

8938cc3

AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 20, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 20, 2019

stephenplusplus suggested changes Oct 21, 2019

View reviewed changes

bcoe reviewed Oct 21, 2019

View reviewed changes

system-test/datastore.ts Outdated Show resolved Hide resolved

jkwlui reviewed Oct 21, 2019

View reviewed changes

test/entity.ts Outdated Show resolved Hide resolved

AVaksman and others added 5 commits October 23, 2019 15:50

Merge branch 'master' into object_numeric_typecast

8d93873

test: remove unused vars

c9f7946

Merge branch 'master' into object_numeric_typecast

d24e515

add and refactor with wrapNumbers option

aab5bac

lint: lint

ba4a314

stephenplusplus suggested changes Nov 6, 2019

View reviewed changes

AVaksman and others added 5 commits November 7, 2019 02:18

test: test ignore if is not set

7c366cc

fix: some more refactoring and docs correction

f353bdf

lint: lint

08f6fa3

jsdoc: minor correction

96b0043

Merge branch 'master' into object_numeric_typecast

98f0491

crwilcox and others added 2 commits November 10, 2019 01:48

Merge branch 'master' into object_numeric_typecast

f856765

refactor: refactor into signle param wrapNumbersOptions: boolean | In…

05968bd

…tegerTypeCastOptions

stephenplusplus reviewed Nov 11, 2019

View reviewed changes

AVaksman added 4 commits November 11, 2019 14:32

refactor: rename wrapNumbersOptions to wrapNumbers

e59dd2d

fix: if integerTypeCasFunction is provided API should return call it …

832d31b

…by default vs just returning DsInt

docs: more descriptions about wrapNumbers options

aeda4f4

lint: lint

a193365

stephenplusplus suggested changes Nov 12, 2019

View reviewed changes

AVaksman added 4 commits November 12, 2019 15:40

remove accidently added package

6b440f5

improve error messages and correct typos

ae1d249

improve test for auto-call #valueOf when integerTypeCastFunction prov…

ecde411

…ided

test: split unit tests

db6fb32

AVaksman and others added 2 commits November 13, 2019 18:49

test: move done after assert

0d277c1

Merge branch 'master' into object_numeric_typecast

87792e6

stephenplusplus approved these changes Nov 14, 2019

View reviewed changes

crwilcox approved these changes Nov 14, 2019

View reviewed changes

crwilcox merged commit 6c8cc74 into googleapis:master Nov 14, 2019

This was referenced Nov 14, 2019

chore: release 5.0.0 #537

Closed

chore: release 5.0.0 #538

Merged

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

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

Conversation

AVaksman commented Oct 13, 2019 • edited Loading

codecov bot commented Oct 13, 2019 • edited Loading

Codecov Report

AVaksman commented Oct 13, 2019

stephenplusplus commented Oct 15, 2019

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crwilcox Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenplusplus left a comment

Choose a reason for hiding this comment

jkwlui Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwlui Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crwilcox commented Nov 9, 2019

AVaksman commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVaksman Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVaksman commented Nov 13, 2019 • edited Loading

stephenplusplus commented Nov 14, 2019

AVaksman commented Oct 13, 2019 •

edited

Loading

codecov bot commented Oct 13, 2019 •

edited

Loading

crwilcox Oct 21, 2019 •

edited

Loading

jkwlui Oct 21, 2019 •

edited

Loading

jkwlui Oct 21, 2019 •

edited

Loading

AVaksman Nov 11, 2019 •

edited

Loading

AVaksman commented Nov 13, 2019 •

edited

Loading