feat(NODE-3504): add unambiguous Timestamp() constructor overload
#449
Conversation
|
Linter failures are already present on the master branch, PR to address them: #450 |
- 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') { |
There was a problem hiding this comment.
do we want to include any sort of validation message if the t or i parameter is missing?
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
My original thought was that we could do it by checking if one of
toriis 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.
There was a problem hiding this comment.
valueOfis onObject.prototype
🤦 Oh right right, never mind on that check then
There was a problem hiding this comment.
I guess we wouldn't want to validate for NaN here either because of precedent?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I guess we wouldn't want to validate for
NaNhere 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)
| // 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') { |
There was a problem hiding this comment.
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
changed the title
Timestamp() ctor overloadTimestamp() constructor overload
…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.
Description
Add a
Timestamp({ t: <number>, i: <number> })overload,and mark the
Timestamp(low, high)overload as deprecated.