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.

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: Fixes number autoconversion #941

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 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
1713a9a
tests for wrapping ints
danieljbruce May 3, 2022
b32d3cc
Double values test
danieljbruce May 3, 2022
5e1b0f8
nit: clarify comment
danieljbruce May 3, 2022
dd9e711
Remove dependency.
danieljbruce May 4, 2022
e01fec0
Add a test to ensure that doubles are received
danieljbruce May 5, 2022
8aa7c31
Added more tests for Double
danieljbruce May 5, 2022
4d15a47
remove repeated code fragment
danieljbruce May 5, 2022
8dc7b62
Changed warning message
danieljbruce May 5, 2022
d6eff51
linting fix
danieljbruce May 6, 2022
70de408
Merge branch 'main' of https://github.com/googleapis/nodejs-datastore…
danieljbruce May 6, 2022
074d218
Remove file
danieljbruce May 9, 2022
1bf4254
Attempts to fix sample test
danieljbruce May 9, 2022
2cdf4c5
Linting fix
danieljbruce May 9, 2022
6044901
Updates for sample test
danieljbruce May 9, 2022
4f0c8b4
Fix the datastore int constructions
danieljbruce May 9, 2022
85ae2cc
linting fix
danieljbruce May 9, 2022
56733e7
before and after balance
danieljbruce May 9, 2022
80b993b
use deep strict equal
danieljbruce May 10, 2022
9371dcb
Fix sample
danieljbruce May 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 10 additions & 8 deletions samples/concepts.js
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,14 @@ class Transaction extends TestHelper {

// Overwrite so the real Datastore instance is used in `transferFunds`.
datastore = this.datastore;
const beforeBalance = datastore.int({
propertyName: 'balance',
integerValue: originalBalance - amountToTransfer,
});
const afterBalance = datastore.int({
propertyName: 'balance',
integerValue: originalBalance + amountToTransfer,
});
try {
await this.restoreBankAccountBalances({
keys: [fromKey, toKey],
Expand All @@ -1033,14 +1041,8 @@ class Transaction extends TestHelper {
const accounts = results.map(result => result[0]);
// Restore `datastore` to the mock API.
datastore = datastoreMock;
assert.strictEqual(
accounts[0].balance,
originalBalance - amountToTransfer
);
assert.strictEqual(
accounts[1].balance,
originalBalance + amountToTransfer
);
assert.deepStrictEqual(accounts[0].balance, beforeBalance);
assert.deepStrictEqual(accounts[1].balance, afterBalance);
} catch (err) {
datastore = datastoreMock;
throw err;
Expand Down
12 changes: 10 additions & 2 deletions samples/test/concepts.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ describe('concepts', () => {
it('performs a query with multi sort', () => query.testMultiSort());
it('performs a kindless query', () => query.testKindlessQuery());
it('performs a projection query', () => {
const priorities = transaction.datastore.int({
integerValue: 4,
propertyName: 'priority',
});
const percentCompletes = transaction.datastore.int({
integerValue: 10,
propertyName: 'percent_complete',
});
return entity
.testProperties()
.then(() => {
Expand All @@ -103,8 +111,8 @@ describe('concepts', () => {
})
.then(results => {
assert.deepStrictEqual(results, {
priorities: [4],
percentCompletes: [10],
priorities: [priorities],
percentCompletes: [percentCompletes],
});
});
});
Expand Down
98 changes: 76 additions & 22 deletions src/entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export namespace entity {
* provided.
*
* @class
* @param {number} value The double value.
* @param {number|string} value The double value.
*
* @example
* ```
Expand All @@ -61,20 +61,38 @@ export namespace entity {
* const aDouble = datastore.double(7.3);
* ```
*/
export class Double {
export class Double extends Number {
private _entityPropertyName: string | undefined;
type: string;
value: number;
constructor(value: number) {
constructor(value: number | string | ValueProto) {
const inferredValue =
typeof value === 'object' ? value.doubleValue : value;
super(inferredValue);

/**
* @name Double#type
* @type {string}
*/
this.type = 'DatastoreDouble';

this._entityPropertyName =
typeof value === 'object' ? value.propertyName : undefined;

/**
* @name Double#value
* @type {number}
*/
this.value = value;
this.value = Number(inferredValue);
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
valueOf(): any {
return Number(this.value);
}

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

Expand Down Expand Up @@ -495,7 +513,7 @@ export namespace entity {
*
* @private
* @param {object} valueProto The protobuf Value message to convert.
* @param {boolean | IntegerTypeCastOptions} [wrapNumbers=false] Wrap values of integerValue type in
* @param {boolean | IntegerTypeCastOptions} [wrapNumbers=true] Wrap values of integerValue type in
* {@link Datastore#Int} objects.
* If a `boolean`, this will wrap values in {@link Datastore#Int} objects.
* If an `object`, this will return a value returned by
Expand Down Expand Up @@ -545,10 +563,21 @@ export namespace entity {
}

case 'doubleValue': {
if (wrapNumbers === undefined) {
wrapNumbers = true;
}

if (wrapNumbers) {
return new entity.Double(valueProto);
}
return Number(value);
}

case 'integerValue': {
if (wrapNumbers === undefined) {
wrapNumbers = true;
}

return wrapNumbers
? typeof wrapNumbers === 'object'
? new entity.Int(valueProto, wrapNumbers).valueOf()
Expand Down Expand Up @@ -604,23 +633,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 @@ -631,6 +643,40 @@ 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>)' or " +
"'Datastore.double(<double_value_as_string>)' to preserve consistent " +
'Datastore types 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 {
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 @@ -691,6 +737,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
4 changes: 2 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1369,11 +1369,11 @@ class Datastore extends DatastoreRequest {
* ]);
* ```
*/
static int(value: number | string) {
static int(value: number | string | ValueProto) {
return new entity.Int(value);
}

int(value: number | string) {
int(value: number | string | ValueProto) {
return Datastore.int(value);
}

Expand Down
2 changes: 1 addition & 1 deletion src/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ class Query {
* [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.wrapNumbers=false]
* @param {boolean | IntegerTypeCastOptions} [options.wrapNumbers=True]
* Wrap values of integerValue type in {@link Datastore#Int} objects.
* If a `boolean`, this will wrap values in {@link Datastore#Int} objects.
* If an `object`, this will return a value returned by
Expand Down
2 changes: 1 addition & 1 deletion src/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ 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 | IntegerTypeCastOptions} [options.wrapNumbers=false]
* @param {boolean | IntegerTypeCastOptions} [options.wrapNumbers=true]
* Wrap values of integerValue type in {@link Datastore#Int} objects.
* If a `boolean`, this will wrap values in {@link Datastore#Int} objects.
* If an `object`, this will return a value returned by
Expand Down
47 changes: 41 additions & 6 deletions system-test/datastore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import * as yaml from 'js-yaml';
import {Datastore, Index} from '../src';
import {google} from '../protos/protos';
import {Storage} from '@google-cloud/storage';
import {entity} from '../src/entity';

describe('Datastore', () => {
const testKinds: string[] = [];
Expand Down Expand Up @@ -72,11 +73,11 @@ describe('Datastore', () => {
publishedAt: new Date(),
author: 'Silvano',
isDraft: false,
wordCount: 400,
rating: 5.0,
wordCount: new entity.Int({propertyName: 'wordCount', integerValue: 400}),
rating: new entity.Double({propertyName: 'rating', doubleValue: 5.0}),
likes: null,
metadata: {
views: 100,
views: new entity.Int({propertyName: 'views', integerValue: 100}),
},
};

Expand Down Expand Up @@ -273,6 +274,40 @@ describe('Datastore', () => {
await datastore.delete(postKey);
});

it('should save/get an int-able double value via Datastore.', async () => {
const postKey = datastore.key('Team');
const points = Datastore.double(2);
await datastore.save({key: postKey, data: {points}});
let [entity] = await datastore.get(postKey, {wrapNumbers: true});
// Expect content is stored in datastore as a double with wrapping to
// return a wrapped double
assert.strictEqual(entity.points.type, 'DatastoreDouble');
assert.strictEqual(entity.points.value, 2);

[entity] = await datastore.get(postKey, {wrapNumbers: false});
// Verify that when requested with wrapNumbers false, we get a plain
// javascript Number 2.
assert.strictEqual(entity.points, 2);

[entity] = await datastore.get(postKey);
// Expect without any options, a wrapped double to be returned.
assert.strictEqual(entity.points.type, 'DatastoreDouble');
assert.strictEqual(entity.points.value, 2);

// Save the data again, get again, ensuring that along the way it isn't
// somehow changed to another numeric type.
await datastore.save(entity);
[entity] = await datastore.get(postKey);
// expect as we saved, that this property is still a DatastoreDouble.
assert.strictEqual(entity.points.type, 'DatastoreDouble');
assert.strictEqual(entity.points.value, 2);

// Verify that DatastoreDouble implement Number behavior
assert.strictEqual(entity.points + 1, 3);

await datastore.delete(postKey);
});

it('should wrap specified properties via IntegerTypeCastOptions.properties', async () => {
const postKey = datastore.key('Scores');
const largeIntValueAsString = '9223372036854775807';
Expand Down Expand Up @@ -518,7 +553,7 @@ describe('Datastore', () => {
},
});
const [entity] = await datastore.get(key);
assert.strictEqual(entity.year, integerValue);
assert.strictEqual(entity.year.valueOf(), integerValue);
});

it('should save and decode a double', async () => {
Expand All @@ -532,7 +567,7 @@ describe('Datastore', () => {
},
});
const [entity] = await datastore.get(key);
assert.strictEqual(entity.nines, doubleValue);
assert.strictEqual(entity.nines.valueOf(), doubleValue);
});

it('should save and decode a geo point', async () => {
Expand Down Expand Up @@ -890,7 +925,7 @@ describe('Datastore', () => {
datastore.get(key),
]);
assert.strictEqual(typeof deletedEntity, 'undefined');
assert.strictEqual(fetchedEntity.rating, 10);
assert.strictEqual(fetchedEntity.rating.valueOf(), 10);
});

it('should use the last modification to a key', async () => {
Expand Down