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

AVaksman · 2019-09-05T14:23:12Z

Fixes #147

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

As per discussion, implementation follows design in the document

On default 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

…ue' type cast option

codecov · 2019-09-05T14:27:59Z

Codecov Report

Merging #488 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   98.22%   98.27%   +0.04%     
==========================================
  Files           6        6              
  Lines         734      753      +19     
  Branches      172      178       +6     
==========================================
+ Hits          721      740      +19     
  Misses          2        2              
  Partials       11       11

Impacted Files	Coverage Δ
src/query.ts	`95.83% <ø> (ø)`	⬆️
src/entity.ts	`98.76% <100%> (+0.07%)`	⬆️
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 3022c65...4b7cb3e. Read the comment docs.

bcoe · 2019-09-05T17:45:30Z

@crwilcox were we comfortable with making this a major release of datastore? I know we're trying to keep major bumps infrequent, but this seems like a reasonably useful refactor to consider one.

callmehiphop

So just a general note, I didn't realize that the type cast function would only apply to integers. Not sure if I missed a separate conversation leading to this PR, but I think this would be useful for all data types, not just integers.

src/entity.ts

AVaksman · 2019-09-06T14:37:32Z

So just a general note, I didn't realize that the type cast function would only apply to integers. Not sure if I missed a separate conversation leading to this PR, but I think this would be useful for all data types, not just integers.

My logic here is, that library doesn't have an issue with correctly obtaining all other value types and if any conversions/manipulations are required, the user can do them after getting the values from this library.

With regards to numeric types there is an issue of obtaining accurate large integer values.

Another point is, I wouldn't want to incorporate third party code more than I have to. Not to subjugate the library to performance impact, bugs, etc... from third party.

callmehiphop · 2019-09-06T21:49:02Z

@AVaksman makes sense, I might be bikeshedding but if this only applies to integers we might want to revisit the name? Though if no one else feels the same way I don't feel super strongly about it 😄

AVaksman · 2019-09-09T20:57:43Z

Refactored to
typeCastConfig => integerTypeCastOptions
typeCastFunction => integerTypeCastFunction

JustinBeckwith

Based on the PR description, I can't really figure out what the breaking change is. Can you add a really clear message in there for the release notes, including before code and after code?

AVaksman · 2019-09-12T00:35:12Z

Based on the PR description, I can't really figure out what the breaking change is. Can you add a really clear message in there for the release notes, including before code and after code?

The breaking part is that now in case of integerValue is out of bounds of Number the API will throw an error instead of truncating the value as before.

Users who needed to accurately obtain out of bounds integer values, couldn't. So they probably didn't use this functionality, hence practically speaking this is not breaking change.

stephenplusplus · 2019-09-12T01:26:03Z

src/entity.ts

@@ -372,7 +435,7 @@ export namespace entity {
      }

      case 'integerValue': {
-        return Number(value);
+        return decodeIntegerValue(valueProto, integerTypeCastOptions);


This is definitely a more robust option, but was just returning the DS.Int type considered?

Sorry, I mixed up Datastore's "Int" with Spanner's: https://github.com/googleapis/nodejs-spanner/blob/f8ad0ece28e99dd9c6d7e985de56bd1b376f4712/src/codec.ts#L139 -- for simplicity's sake, would it be worth considering using that format instead? When a value is out of bounds, it will throw. The value is accessible via the "value" property in string form.

I do like the ability to still access the value in string form without extra network call in case of value is out of balance.

it will definitely be a breaking change.

IMO it will add complexity to the user's code.
will need to try=>catch for valueOf and if error still pass the value to custom typeCastFunction.

Is it overkill to combine these two approaches?

return Spanner's style object for integerValue.

for valueOf on default throw in case of out of bounds.

able to pass integerTypeCastFunction and valueOf will return the custom function result.

I do like the ability to still access the value in string form without extra network call in case of value is out of balance.

Sorry if I misunderstand. Neither of the solutions make an extra network call, do they?

it will definitely be a breaking change.

I think the throwing behavior is identical to what is in this PR. It will first try to just use it as a number, but if it's out of bounds when called upon, it throws. Or, is it because the new "Int" wrapper isn't technically a Number? Lots of gray area in determining a breaking change 😕

Is it overkill to combine these two approaches?

I quite like that approach! Maybe @callmehiphop has a thought on this, too.

Thanks for clearing that up for me! For the Number vs customer "Int" type issue, they can at least be used interchangeably, the only difference would be typeof checks for Number wouldn't pass... unless Int extended Number? Curious to hear other thoughts on the best way to go forward.

Did we ever make progress on this, maybe in another chat?

Quick summary
Would like to hear the thoughts on the approach

Throw on default, let user pass integerTypeCastFunction (currently implemented in this PR)

Return an object Integer similar to Spanner, where

#valueOf would try to convert to Number and throw if out of bounds

#value returns string representation of the value

Combine tow approaches above

do return an Integer object

let user provide integerTypeCast function to be used by #valueOf

still return string via #value

My vote would go for #3. Ping to @callmehiphop and @bcoe for more votes.

My vote is starting simple with option 1, as a breaking change, and we can move towards an object representation in the future, if there's good reason.

JustinBeckwith · 2019-09-12T04:18:10Z

@AVaksman ah in that case - can you drop the ! from the commit title and PR title?

AVaksman · 2019-09-12T05:48:24Z

Roger that

deferring to others

bcoe

Let's call this feature out in the README.md, specifically explaining how to setup a typecast function to avoid an out of bounds.

src/entity.ts

BenWhitehead · 2019-09-26T20:40:10Z

@jkwlui Does everything look good here for you after the latest fixes?

AVaksman · 2019-10-02T00:16:55Z

Quick summary
Would like to hear the thoughts on the approach

Throw on default, let user pass integerTypeCastFunction (currently implemented in this PR)
Return an object Integer similar to Spanner, where
- #valueOf would try to convert to Number and throw if out of bounds
- #value returns string representation of the value
Combine tow approaches above
- do return an Integer object
- let user provide integerTypeCast function to be used by #valueOf
- still return string via #value

from @stephenplusplus

My vote would go for #3.

stephenplusplus · 2019-10-03T17:14:45Z

@AVaksman maybe it'll just be us deciding 😆 What is your preference?

AVaksman · 2019-10-03T19:57:53Z

@stephenplusplus I do like option 3 as well, @bcoe 's vote goes to # 1

JustinBeckwith · 2019-10-12T18:53:12Z

@AVaksman @stephenplusplus @bcoe what's the next steps here?

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

The next step would be to decide between approach in this PR vs #516

stephenplusplus · 2019-11-04T16:13:40Z

I don't see us going back to this after where we landed in #516, so I'll just close for now to avoid any confusion.

AVaksman added 2 commits September 5, 2019 00:25

feat: handle outside of bounds numeric values, add custom 'integerVal…

60c0900

…ue' type cast option

chore: add unit tests

6d1c7cc

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

AVaksman changed the title ~~feat: throw error with out of bounds integer values, add custom 'integerValue' type cast option~~ (BREAKING CHANGE) feat: throw error with out of bounds integer values, add custom 'integerValue' type cast option Sep 5, 2019

Merge branch 'master' into numeric_typeCast

edeec3e

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

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

feat: add validation agains MIN_SAFE_INTEGER

e882977

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

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

AVaksman marked this pull request as ready for review September 5, 2019 15:47

bcoe changed the title ~~(BREAKING CHANGE) feat: throw error with out of bounds integer values, add custom 'integerValue' type cast option~~ feat!: throw error with out of bounds integer values, add custom 'integerValue' type cast option Sep 5, 2019

callmehiphop reviewed Sep 5, 2019

View reviewed changes

src/entity.ts Outdated Show resolved Hide resolved

refactor: remove extra condition

5863365

AVaksman added 2 commits September 6, 2019 15:21

Merge branch 'master' into numeric_typeCast

9358b68

Merge branch 'master' into numeric_typeCast

a7856d8

AVaksman and others added 2 commits September 9, 2019 13:49

Merge branch 'master' into numeric_typeCast

5f5be01

refactor: more specific name

8668bd5

AVaksman and others added 2 commits September 11, 2019 15:58

Merge branch 'master' into numeric_typeCast

e5b8635

Merge branch 'master' into numeric_typeCast

d66115b

JustinBeckwith previously requested changes Sep 11, 2019

View reviewed changes

AVaksman changed the title ~~feat!: throw error with out of bounds integer values, add custom 'integerValue' type cast option~~ feat!: throw error with out of bounds integer values instead of truncating, add custom 'integerValue' type cast option Sep 12, 2019

stephenplusplus reviewed Sep 12, 2019

View reviewed changes

AVaksman changed the title ~~feat!: throw error with out of bounds integer values instead of truncating, add custom 'integerValue' type cast option~~ feat: throw error with out of bounds integer values instead of truncating, add custom 'integerValue' type cast option Sep 12, 2019

Merge branch 'master' into numeric_typeCast

40f2f90

JustinBeckwith requested review from JustinBeckwith and removed request for JustinBeckwith September 12, 2019 18:07

refactor: use Number.isSafeInteger

4e6f530

bcoe changed the title ~~feat: throw error with out of bounds integer values instead of truncating, add custom 'integerValue' type cast option~~ feat!: throw error with out of bounds integer values instead of truncating, add custom 'integerValue' type cast option Sep 16, 2019

bcoe approved these changes Sep 16, 2019

View reviewed changes

jkwlui reviewed Sep 16, 2019

View reviewed changes

src/entity.ts Outdated Show resolved Hide resolved

src/entity.ts Outdated Show resolved Hide resolved

src/entity.ts Outdated Show resolved Hide resolved

src/entity.ts Outdated Show resolved Hide resolved

AVaksman and others added 4 commits September 16, 2019 17:15

fix: docs

b856dc8

fix: docs2

e201f6e

Merge branch 'master' into numeric_typeCast

4c2ba71

Merge branch 'master' into numeric_typeCast

1366a7a

Merge branch 'master' into numeric_typeCast

4fb2306

AVaksman and others added 2 commits October 2, 2019 10:23

Merge branch 'master' into numeric_typeCast

6770518

Merge branch 'master' into numeric_typeCast

68ce578

AVaksman and others added 2 commits October 7, 2019 17:55

Merge branch 'master' into numeric_typeCast

7aa3617

Merge branch 'master' into numeric_typeCast

4b7cb3e

AVaksman mentioned this pull request Oct 13, 2019

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

Merged

3 tasks

stephenplusplus closed this Nov 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AVaksman commented Sep 5, 2019 •

edited

Loading

codecov bot commented Sep 5, 2019 •

edited

Loading

bcoe commented Sep 5, 2019

callmehiphop left a comment

AVaksman commented Sep 6, 2019 •

edited

Loading

callmehiphop commented Sep 6, 2019

AVaksman commented Sep 9, 2019

JustinBeckwith left a comment

AVaksman commented Sep 12, 2019

stephenplusplus Sep 12, 2019

stephenplusplus Sep 12, 2019

AVaksman Sep 12, 2019 •

edited

Loading

AVaksman Sep 12, 2019

stephenplusplus Sep 12, 2019

stephenplusplus Sep 12, 2019

stephenplusplus Sep 19, 2019

AVaksman Sep 19, 2019 •

edited

Loading

stephenplusplus Sep 19, 2019

bcoe Oct 3, 2019

JustinBeckwith commented Sep 12, 2019

AVaksman commented Sep 12, 2019

bcoe left a comment

BenWhitehead commented Sep 26, 2019

AVaksman commented Oct 2, 2019

stephenplusplus commented Oct 3, 2019

AVaksman commented Oct 3, 2019

JustinBeckwith commented Oct 12, 2019

AVaksman commented Oct 13, 2019

stephenplusplus commented Nov 4, 2019

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

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

Conversation

AVaksman commented Sep 5, 2019 • edited Loading

codecov bot commented Sep 5, 2019 • edited Loading

Codecov Report

bcoe commented Sep 5, 2019

callmehiphop left a comment

Choose a reason for hiding this comment

AVaksman commented Sep 6, 2019 • edited Loading

callmehiphop commented Sep 6, 2019

AVaksman commented Sep 9, 2019

JustinBeckwith left a comment

Choose a reason for hiding this comment

AVaksman commented Sep 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVaksman Sep 12, 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

AVaksman Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinBeckwith commented Sep 12, 2019

AVaksman commented Sep 12, 2019

bcoe left a comment

Choose a reason for hiding this comment

BenWhitehead commented Sep 26, 2019

AVaksman commented Oct 2, 2019

stephenplusplus commented Oct 3, 2019

AVaksman commented Oct 3, 2019

JustinBeckwith commented Oct 12, 2019

AVaksman commented Oct 13, 2019

stephenplusplus commented Nov 4, 2019

AVaksman commented Sep 5, 2019 •

edited

Loading

codecov bot commented Sep 5, 2019 •

edited

Loading

AVaksman commented Sep 6, 2019 •

edited

Loading

AVaksman Sep 12, 2019 •

edited

Loading

AVaksman Sep 19, 2019 •

edited

Loading