Skip to content

fix!: don't unwrap int, double by default, add a warning when a number is autoconverted to double or int #773

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

Closed
wants to merge 18 commits into from
Closed

fix!: don't unwrap int, double by default, add a warning when a number is autoconverted to double or int #773

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
28c2327
fix: discontinue auto-conversion of JavaScript Numbers to datastore t…
crwilcox Dec 7, 2020
8a5aba8
fix!:add a warning when a number is autoconverted to double or int
crwilcox Dec 8, 2020
068b0bf
chore: gts fix
crwilcox Dec 8, 2020
a7152f5
fix: modify warnings to be phrased better
crwilcox Dec 8, 2020
b125553
fix: process.once to avoid debouncing type issues
crwilcox Dec 8, 2020
2d31c03
fix: debounce warning of typing to occur only once
crwilcox Dec 8, 2020
5b62746
chore: gts fix
crwilcox Dec 9, 2020
39e618c
chore: don't use it.only
crwilcox Dec 9, 2020
573880d
test: add test to verify typecast warning is debounced
crwilcox Dec 9, 2020
a2ba674
fix: errant console log committed
crwilcox Dec 10, 2020
beab398
test: alter expected properties to match property
crwilcox Dec 10, 2020
aa86a2f
test: add test that captures current, not desired, behavior
crwilcox Dec 10, 2020
1d80aea
feat!: do not automatically unwrap numbers for users, though allow th…
crwilcox Dec 11, 2020
b2f4e4d
test: modify test to capture now current wrap behavior. other tests n…
crwilcox Dec 11, 2020
3c6eb03
feat: extend Number on Double class
crwilcox Dec 11, 2020
62f4f3e
test: fix system tests
crwilcox Dec 11, 2020
2085357
test: fix unit tests, revert to using number for storing value of double
crwilcox Dec 12, 2020
b28df5f
test: fix system test for number/double string/number change.
crwilcox Dec 12, 2020
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
50 changes: 33 additions & 17 deletions src/entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {PathType} from '.';
import {protobuf as Protobuf} from 'google-gax';
import * as path from 'path';
import {google} from '../protos/protos';
import {type} from 'os';

// eslint-disable-next-line @typescript-eslint/no-namespace
export namespace entity {
Expand Down Expand Up @@ -580,23 +581,6 @@ export namespace entity {
return valueProto;
}

if (typeof value === 'number') {
if (Number.isInteger(value)) {
if (!Number.isSafeInteger(value)) {
process.emitWarning(
'IntegerOutOfBoundsWarning: ' +
"the value for '" +
property +
"' property is outside of bounds of a JavaScript Number.\n" +
"Use 'Datastore.int(<integer_value_as_string>)' to preserve accuracy during the upload."
);
}
value = new entity.Int(value);
} else {
value = new entity.Double(value);
}
}

if (isDsInt(value)) {
valueProto.integerValue = value.value;
return valueProto;
Expand All @@ -607,6 +591,38 @@ export namespace entity {
return valueProto;
}

if (typeof value === 'number') {
const integerOutOfBoundsWarning =
"'IntegerOutOfBoundsWarning: the value for '" +
property +
"' property is outside of bounds of a JavaScript Number.\n" +
"Use 'Datastore.int(<integer_value_as_string>)' to preserve accuracy during the upload.";

const typeCastWarning =
"TypeCastWarning: the value for '" +
property +
"' property is a JavaScript Number.\n" +
"Use 'Datastore.int(<integer_value_as_string>)' or " +
"'Datastore.double(<double_value_as_string>)' to preserve consistent " +
'Datastore types during the upload.';

if (Number.isInteger(value)) {
if (!Number.isSafeInteger(value)) {
process.emitWarning(integerOutOfBoundsWarning);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the else here ?
I don't think the branches are exclusive.

Also maybe move process.emitWarning(typeCastWarning); right after l. 594 as soon as we know that the type is a number ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are exclusive. They both recommend the same outcome, but I think having two separate warnings is a bit confusing. This one warning is it could be dangerous, the other is destructive. I had what you recommended earlier but it is very very noisy :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: added 'debouncing' you will only get a single typecast warning for the entity. that way it doesn't flood the warning feed.

Copy link

Choose a reason for hiding this comment

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

It might be reasonable to augment the out of bounds warning to mention the type issue as well, since that'll still be an issue if they fix their number size issue.

Copy link

Choose a reason for hiding this comment

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

Since we're debouncing the typescast warning any ways, I'd consider just dropping the if and printing both.

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 printing both could get noisy as Chris said. I ended up going with @feywind's suggestion here.

process.emitWarning(typeCastWarning);
}
value = new entity.Int(value);
valueProto.integerValue = value.value;
return valueProto;
} else {
process.emitWarning(typeCastWarning);
value = new entity.Double(value);
valueProto.doubleValue = value.value;
return valueProto;
}
}

if (isDsGeoPoint(value)) {
valueProto.geoPointValue = value.value;
return valueProto;
Expand Down
33 changes: 30 additions & 3 deletions test/entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,13 +744,26 @@ describe('entity', () => {
assert.deepStrictEqual(entity.encodeValue(value), expectedValueProto);
});

it('should encode an int', () => {
it('should encode an int', done => {
const value = 8;

const expectedValueProto = {
integerValue: value,
};

const property = 'value';
const expectedWarning =
"TypeCastWarning: the value for '" +
property +
"' property is a JavaScript Number.\n" +
"Use 'Datastore.int(<integer_value_as_string>)' or " +
"'Datastore.double(<double_value_as_string>)' to preserve consistent " +
'Datastore types during the upload.';
process.on('warning', warning => {
assert.strictEqual(warning.message, expectedWarning);
done();
});

entity.Int = function (value_: {}) {
assert.strictEqual(value_, value);
this.value = value_;
Expand All @@ -759,7 +772,7 @@ describe('entity', () => {
assert.deepStrictEqual(entity.encodeValue(value), expectedValueProto);
});

it('should emit warning on out of bounce int', done => {
it('should emit warning on out of bounds int', done => {
const largeIntValue = 9223372036854775807;
const property = 'largeInt';
const expectedWarning =
Expand All @@ -786,13 +799,27 @@ describe('entity', () => {
assert.deepStrictEqual(entity.encodeValue(value), expectedValueProto);
});

it('should encode a double', () => {
it('should encode a double', done => {
const value = 8.3;

const expectedValueProto = {
doubleValue: value,
};

const property = 'value';
const expectedWarning =
"TypeCastWarning: the value for '" +
property +
"' property is a JavaScript Number.\n" +
"Use 'Datastore.int(<integer_value_as_string>)' or " +
"'Datastore.double(<double_value_as_string>)' to preserve consistent " +
'Datastore types during the upload.';

process.on('warning', warning => {
assert.strictEqual(warning.message, expectedWarning);
done();
});

entity.Double = function (value_: {}) {
assert.strictEqual(value_, value);
this.value = value_;
Expand Down
4 changes: 2 additions & 2 deletions test/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1342,7 +1342,7 @@ describe('Datastore', () => {
data: {
stringField: 'string value',
nullField: null,
arrayField: [0, null],
arrayField: [datastore.int(0), null],
objectField: null,
},
excludeLargeProperties: true,
Expand Down Expand Up @@ -1445,7 +1445,7 @@ describe('Datastore', () => {
data: {
value: {
a: 'b',
c: [1, 2, 3],
c: [datastore.int(1), datastore.int(2), datastore.int(3)],
},
},
},
Expand Down