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

feat(NODE-3504): add unambiguous Timestamp() constructor overload #449

Merged
merged 2 commits into from
Aug 2, 2021
Merged

feat(NODE-3504): add unambiguous Timestamp() constructor overload #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6de2da
feat(NODE-3504): add unambiguous `Timestamp()` ctor overload
addaleax Jul 30, 2021
0a58875
fixup: review comment
addaleax Aug 2, 2021
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
19 changes: 13 additions & 6 deletions src/timestamp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { Long } from './long';
import { isObjectLike } from './parser/utils';

/** @public */
export type TimestampOverrides = '_bsontype' | 'toExtendedJSON' | 'fromExtendedJSON' | 'inspect';
/** @public */
export type LongWithoutOverrides = new (low: number | Long, high?: number, unsigned?: boolean) => {
export type LongWithoutOverrides = new (low: unknown, high?: number, unsigned?: boolean) => {
[P in Exclude<keyof Long, TimestampOverrides>]: Long[P];
};
/** @public */
Expand All @@ -27,20 +28,26 @@ export class Timestamp extends LongWithoutOverridesClass {
/**
* @param low - A 64-bit Long representing the Timestamp.
*/
constructor(low: Long);
constructor(long: Long);
/**
* @param value - A pair of two values indicating timestamp and increment.
*/
constructor(value: { t: number; i: number });
/**
* @param low - the low (signed) 32 bits of the Timestamp.
* @param high - the high (signed) 32 bits of the Timestamp.
* @deprecated Please use `Timestamp({ t: high, i: low })` or `Timestamp(Long(low, high))` instead.
*/
constructor(low: Long);
constructor(low: number, high: number);
constructor(low: number | Long, high?: number) {
constructor(low: number | Long | { t: number; i: number }, high?: number) {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
///@ts-expect-error
if (!(this instanceof Timestamp)) return new Timestamp(low, high);

if (Long.isLong(low)) {
super(low.low, low.high, true);
} else if (isObjectLike(low) && typeof low.t !== 'undefined' && typeof low.i !== 'undefined') {
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 want to include any sort of validation message if the t or i parameter is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So … one problem here is that the Long() constructor coerces its arguments to number, rather than validating them, and so it accepted anything with a working .valueOf() implementation (and consequently, Timestamp did so as well).

For example, something like new bson.Timestamp(new bson.Int32(123)) is valid right now and works, and would be broken if this line was turned into something like else if (isObjectLike(low)) and then throwing if t or i are missing.

I would probably lean towards keeping the behavior as-is for any input that is not matching the { t: ..., i: ...} format and avoiding possible breakage. If you prefer something else, I’m happy to do that as well, of course. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, thank you for the clarification. It would be nice if we could detect whether they're explicitly trying to use this new constructor and give an error instead of just weird behavior or errors down the line. My original thought was that we could do it by checking if one of t or i is set and then throwing if the other one isn't, and if that's not reliable, maybe checking whether it can, in fact be coerced to a number before passing it on? Not sure if that's too convoluted. If we can't think of a good idea here, it's probably fine to leave as is since we don't seem very concerned with validation elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could check: if its an object and typeof low.valueOf === 'function' then pass it along to the super otherwise we should be able to assert what we want about t and i. Would we want { t: new Int32(1), i: new Number(3) } to work? (I think so, so we wouldn't assert anything about the types, just existence but up for discussion)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My original thought was that we could do it by checking if one of t or i is set and then throwing if the other one isn't

Yeah, we can do this, but I think it’s of pretty limited practical usefulness. What we would ideally want to catch are situations in which neither of these are set.

and if that's not reliable, maybe checking whether it can, in fact be coerced to a number before passing it on

I don’t think there’s a really good way to verify whether this is the case in JS, because:

if its an object and typeof low.valueOf === 'function'

valueOf is on Object.prototype, so for almost all objects typeof low.valueOf === 'function' will be true anyway.

Would we want { t: new Int32(1), i: new Number(3) } to work? (I think so, so we wouldn't assert anything about the types, just existence but up for discussion)

Right, I would also agree that it’s something that I’d intuitively expect to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

valueOf is on Object.prototype

🤦 Oh right right, never mind on that check then

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we wouldn't want to validate for NaN here either because of precedent?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess some other ideas could be to whitelist allowed BSON types (or allow BSON types in general here and only validate if the params object isn't BSON), but I don't know if that has other gotchas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we wouldn't want to validate for NaN here either because of precedent?

Uff … 😄 Yeah, I guess I would also be careful about that and keep the coercion behavior. Fwiw, that’s also what the legacy shell does:

> Timestamp(NaN, NaN)
Timestamp(0, 0)

super(low.i, low.t, true);
} else {
super(low, high, true);
}
Expand Down Expand Up @@ -95,7 +102,7 @@ export class Timestamp extends LongWithoutOverridesClass {

/** @internal */
static fromExtendedJSON(doc: TimestampExtended): Timestamp {
return new Timestamp(doc.$timestamp.i, doc.$timestamp.t);
return new Timestamp(doc.$timestamp);
}

/** @internal */
Expand All @@ -104,6 +111,6 @@ export class Timestamp extends LongWithoutOverridesClass {
}

inspect(): string {
return `new Timestamp(${this.getLowBits().toString()}, ${this.getHighBits().toString()})`;
return `new Timestamp({ t: ${this.getHighBits()}, i: ${this.getLowBits()} })`;
}
}
4 changes: 2 additions & 2 deletions test/node/bson_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2120,8 +2120,8 @@ describe('BSON', function () {
* @ignore
*/
it('Timestamp', function () {
const timestamp = new Timestamp(1, 100);
expect(inspect(timestamp)).to.equal('new Timestamp(1, 100)');
const timestamp = new Timestamp({ t: 100, i: 1 });
expect(inspect(timestamp)).to.equal('new Timestamp({ t: 100, i: 1 })');
});
});
});
17 changes: 16 additions & 1 deletion test/node/timestamp_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ describe('Timestamp', function () {
new BSON.Timestamp(-1, -1),
new BSON.Timestamp(new BSON.Timestamp(0xffffffff, 0xffffffff)),
new BSON.Timestamp(new BSON.Long(0xffffffff, 0xfffffffff, false)),
new BSON.Timestamp(new BSON.Long(0xffffffff, 0xfffffffff, true))
new BSON.Timestamp(new BSON.Long(0xffffffff, 0xfffffffff, true)),
new BSON.Timestamp({ t: 0xffffffff, i: 0xfffffffff }),
new BSON.Timestamp({ t: -1, i: -1 }),
new BSON.Timestamp({ t: new BSON.Int32(0xffffffff), i: new BSON.Int32(0xffffffff) })
].forEach(timestamp => {
expect(timestamp).to.have.property('unsigned', true);
});
Expand All @@ -29,4 +32,16 @@ describe('Timestamp', function () {
$timestamp: { t: 4294967295, i: 4294967295 }
});
});

it('should accept a { t, i } object as constructor input', function () {
const input = { t: 89, i: 144 };
const timestamp = new BSON.Timestamp(input);
expect(timestamp.toExtendedJSON()).to.deep.equal({ $timestamp: input });
});

it('should accept a { t, i } object as constructor input and coerce to integer', function () {
const input = { t: new BSON.Int32(89), i: new BSON.Int32(144) };
const timestamp = new BSON.Timestamp(input);
expect(timestamp.toExtendedJSON()).to.deep.equal({ $timestamp: { t: 89, i: 144 } });
});
});