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

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

Merged
merged 2 commits into from
Aug 2, 2021

Conversation

addaleax
Copy link
Contributor

Description

Add a Timestamp({ t: <number>, i: <number> }) overload,
and mark the Timestamp(low, high) overload as deprecated.

@addaleax
Copy link
Contributor Author

Linter failures are already present on the master branch, PR to address them: #450

addaleax added a commit to mongodb-js/mongosh that referenced this pull request Jul 30, 2021
- Accept a `Timestamp({ t: <number>, i: <number> })` signature
- Reverse the order of arguments when two arguments are passed,
  i.e. `Timestamp(t, i)` instead of `Timestamp(i, t)`, to match
  the legacy shell
- Print Timestamps using the `Timestamp({ t, i })` format

This should land together with the corresponding BSON package PR,
mongodb/js-bson#449.
// 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)

test/node/timestamp_tests.js Outdated Show resolved Hide resolved
// 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.

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)

Add a `Timestamp({ t: <number>, i: <number> })` overload,
and mark the `Timestamp(low, high)` overload as deprecated.
@nbbeeken nbbeeken changed the title feat(NODE-3504): add unambiguous Timestamp() ctor overload feat(NODE-3504): add unambiguous Timestamp() constructor overload Aug 2, 2021
@nbbeeken nbbeeken merged commit 0298dd8 into mongodb:master Aug 2, 2021
addaleax added a commit to mongodb-js/mongosh that referenced this pull request Aug 3, 2021
…1049)

- Accept a `Timestamp({ t: <number>, i: <number> })` signature
- Reverse the order of arguments when two arguments are passed,
  i.e. `Timestamp(t, i)` instead of `Timestamp(i, t)`, to match
  the legacy shell
- Print Timestamps using the `Timestamp({ t, i })` format

This should land together with the corresponding BSON package PR,
mongodb/js-bson#449.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants