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!: 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
Show file tree
Hide file tree
Changes from all commits
Commits
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
96 changes: 74 additions & 22 deletions src/entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,27 +52,44 @@ export namespace entity {
* provided.
*
* @class
* @param {number} value The double value.
* @param {number|string} value The double value.
*
* @example
* const {Datastore} = require('@google-cloud/datastore');
* const datastore = new Datastore();
* 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are Double / Int included in this type ?

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 ought to be, via the Number type. Though that isn't really a use path. Support for that could be added, but as you might detect a trend, this is a mirroring of the existing style we went with for Ints.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a case where we'd want to construct a Number from another Number? If not, I'd start with accepting a string, number, and potentially ValueProto (as we are).

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like there is never a common case where we would want to construct a Double from a Double / Int. Constructing a Double from a Double is just creating a copy and making a Double from an Int could encourage users to misuse the API by inadvertently changing the column type.

super(typeof value === 'object' ? value.doubleValue : value);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case of constructing from a proto ?
If it needs to be supported int would probably make sense too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Int has the same support from proto, and it is part of the entityFromEntityProto path. This code mirrors the behavior of our Int class.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering the same thing, the fact that we allow this in the Int type seems like good motivation 👍 Let's just make sure we have a test for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a bunch more tests to Double here.

Copy link
Contributor

Choose a reason for hiding this comment

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

These tests and all other changes will be in a separate PR.


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

this._entityPropertyName =
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 really need to keep the property name ?
If yes it would be good to comment why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I am not sure what this is for, but I mirrored behavior in the Int implementation. Maybe @stephenplusplus recalls?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're not 100% what it's here for, I'd be tempted to drop it (let's chat with @stephenplusplus offline).

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't checked the use cases but it seems dangerous

if an entity has the form {oldValue: Int, newValue: Int} and you do (with wrapped Number):

const [entity] = await ds.get(...);

entity.oldValue = entity.newValue;
entity.newValue = ds.Int(123);

await ds.save(entity);

I can only imagine that keeping the entity property name would cause problems as oldValue would have a reference to "newValue".

I am not sure if it is actually a problem in the current code but at least it makes it easy to shoot yourself in the foot.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll remove this. The reason we have this for Int is because we use it when using the valueOf() function.

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

/**
* @name Double#value
* @type {number}
*/
this.value = value;
this.value =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do need a value member ?
It seems like it could be dropped as well as valueOf which are both already in the super class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly? This started as a string, but realized I could store this as a number. We store it as a string for Int mostly due to Int range differences.

Datastore Doubles have 64-bit double precision, IEEE 754, as do Node.js Numbers. So I was able to change this type and simplify a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

feels kind of like this should be private.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know about that. If we make it private then that would be a breaking change. Consider the case where users are using .value in various places for these types. In the Int class this is private.

typeof value === 'object' ? Number(value.doubleValue) : Number(value);
}

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

Choose a reason for hiding this comment

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

Number is redundant with l 83.
But is seems like valueOf should be dropped altogether as you extend Number ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Playing around with extending Number, there isn't access to a this.value unless we store the value ourselves:

class F extends Number {
  constructor(value) {
    super(value)
  }
  blerg() {
    console.info(this.value)
  }
}

const f = new F(99);
console.info(f.valueOf())
f.blerg()

outputs:

99
undefined

Part of me wonders if we should avoid extending the base Number type, and instead create a DatastoreNumber type, since Number implies a a larger interface than we potentially want to support out of the gate?

Edit: I see we're already extending Number in Int, I'd stick with this approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

class F extends Number {
  constructor(value) {
    super(value);
  }
  blerg() {
    console.info(Number(this));
  }
}

const f = new F(99);
f.blerg(); // outputs 99 as expected

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know.

}

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

Choose a reason for hiding this comment

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

This seems quite dangerous if the goal of Double is to fake 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.

can you expand? Sorry, I don't quite understand/follow. Also, class Int has a similar toJson.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am really not sure to get the intent of the change right but my understanding of the current state is that you want to return Ints and Doubles that users would use as if they were Numbers.

If I get this right then Ints and Doubles should behave like Numbers as much a possible.

However with this code:

  • JSON.stringify(1.23) == "1.23"
  • but JSON.stringify(Double(1.23)) == '{"type": ..., "value": ...}'

While not related to toJSON() returning wrapped numbers (Int / Double) could be problematic:

const [entity] = await ds.get(...);

// entity.price is a Double, i.e. Double(10)

entity.price *= 0.90;

// entity.price is now a number (9, "int")

// warning + DB screwed
await ds.save(entity);

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that returning wrapped numbers in Int / Double format is necessary in order to avoid the problem where we change the column type. I see how adding toJson changes the output of JSON.stringify which is technically a breaking change, but since Int currently has toJson implemented and Double / Int are likely being returned interchangably, it is worth the tradeoff to add toJSON here to ensure JSON.stringify will handle each type the same way.

}
}

Expand Down Expand Up @@ -475,7 +492,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 @@ -523,10 +540,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 @@ -580,23 +608,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 +618,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 +711,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
2 changes: 1 addition & 1 deletion src/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,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 @@ -447,7 +447,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 @@ -67,11 +68,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 @@ -268,6 +269,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});
Copy link
Contributor

Choose a reason for hiding this comment

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

As a TODO, I wish we had unit tests for the Int and Double types, vs., only integration tests.

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 a good unit test corresponding to this one would be to mock out the get and the save and ensure that getting a Double/Int leads to saving the same type.

// 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, reget, 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 @@ -513,7 +548,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 @@ -527,7 +562,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 @@ -885,7 +920,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
Loading