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

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

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 11 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
58 changes: 41 additions & 17 deletions src/entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,23 +580,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 +590,39 @@ export namespace entity {
return valueProto;
}

if (typeof value === 'number') {
const integerOutOfBoundsWarning =
crwilcox marked this conversation as resolved.
Show resolved Hide resolved
"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 " +
'in your database.';

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 in your database.';

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
Contributor

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.

warn('TypeCastWarning', typeCastWarning);
}
value = new entity.Int(value);
valueProto.integerValue = value.value;
return valueProto;
} else {
warn('TypeCastWarning', typeCastWarning);
value = new entity.Double(value);
valueProto.doubleValue = value.value;
return valueProto;
}
}

if (isDsGeoPoint(value)) {
valueProto.geoPointValue = value.value;
return valueProto;
Expand Down Expand Up @@ -667,6 +683,14 @@ export namespace entity {
throw new Error('Unsupported field value, ' + value + ', was provided.');
}

const warningTypesIssued = new Set<string>();
const warn = (warningName: string, warningMessage: string) => {
if (!warningTypesIssued.has(warningName)) {
warningTypesIssued.add(warningName);
process.emitWarning(warningMessage);
}
};

/**
* Convert any entity protocol to a plain object.
*
Expand Down
69 changes: 64 additions & 5 deletions test/entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,32 +744,77 @@ 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 = 'undefined';
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 in your database.';
process.once('warning', warning => {
assert.strictEqual(warning.message, expectedWarning);
done();
});

entity.Int = function (value_: {}) {
feywind marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(value_, value);
this.value = value_;
};

assert.deepStrictEqual(entity.encodeValue(value), expectedValueProto);
});

it('should encode an int but only warn once', done => {
const value = 8;

const expectedValueProto = {
integerValue: value,
};

const property = 'undefined';
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 in your database.';
process.once('warning', warning => {
assert.strictEqual(warning.message, expectedWarning);
done();
});

entity.Int = function (value_: {}) {
feywind marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(value_, value);
this.value = value_;
};

assert.deepStrictEqual(entity.encodeValue(value), expectedValueProto);
// if an error is reported on this, done() is called more than once and
// should cause failure.
assert.deepStrictEqual(entity.encodeValue(value), expectedValueProto);
crwilcox marked this conversation as resolved.
Show resolved Hide resolved
});

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 =
'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.";
"Use 'Datastore.int(<integer_value_as_string>)' to preserve accuracy " +
'in your database.';

process.on('warning', warning => {
process.once('warning', warning => {
assert.strictEqual(warning.message, expectedWarning);
done();
});
Expand All @@ -786,13 +831,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 = 'undefined';
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 in your database.';

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

entity.Double = function (value_: {}) {
assert.strictEqual(value_, value);
this.value = value_;
Expand Down
12 changes: 9 additions & 3 deletions test/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1316,7 +1316,13 @@ describe('Datastore', () => {
arrayValue: {
values: [
{
integerValue: '0',
entityValue: {
properties: {
value: {
integerValue: '0',
},
},
},
},
{
nullValue: 0,
Expand All @@ -1342,7 +1348,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 +1451,7 @@ describe('Datastore', () => {
data: {
value: {
a: 'b',
c: [1, 2, 3],
c: [datastore.int(1), datastore.int(2), datastore.int(3)],
},
},
},
Expand Down