-
Type: Improvement
-
Resolution: Unresolved
-
Priority: Major - P3
-
None
-
Affects Version/s: bson-4.0.4
-
Component/s: None
The current implementation of Timestamp extends the Long type, exposing all the functionality of that class and erroneously supporting signed values. Additionally, in later versions of node we have access to the BigInt type, obviating the need for Long in the first place. A few refactors are in order:
- Don't subclass Long but instead wrap an internal value, giving us the option of using BigInt if its available
Change the constructor signature to match the server's Timestamp (time_t, ordinal)- Validate that inputs are not signed
Use Case
As a driver engineer
I want to update the Timestamp class to not subclass Long
So that it only exposes the functionality that actually applies to a timestamp
Unknowns
- Should we wrap a bigint or a Long?
would using one over the other carry any performance implications?Can we make use of bigint without breaking currently supported environments?- Neither; we can store two 32 bit integers (i, t) and provide a toLong method that calls the Long ctor on the user's behalf. These match the field names in the server's implementation.
- Which methods do we want to keep as part of our Timestamp API?
- See AC
- Do we still want to match the server's Timestamp constructor? Why or why not?
- Note that one of our signatures already mirrors the server's Timestamp constructor:
- We will not be changing the constructor's signature here
constructor(low?: bigint | Long | { t: number | Int32; i: number | Int32 });
User Impact
Users making use of Long methods on Timestamps will no longer be able to do so.
Dependencies
- If the driver was making use of any Long methods on Timestamp objects, we will have to update it to remedy this
Acceptance Criteria
Implementation Requirements
- Create internal fields, i and t that are both 32 bit integer values (JS Number) and represent the increment and timestamp respectively.
- Make Timestamp subclass directly from BSONValue instead of Long
- retain/refactor the following methods
- inspect
- fromInt
- toJSON
- fromNumber
- fromBits
- fromString
- Implement a toLong method
- Remove LongWithoutOverridesClass
Testing Requirements
- Unit test that asserts that Timestamp constructor throws when receiving a signed input
- Ensure that the methods being retained/reimplemented have associated unit tests
- Ensure existing timestamp tests continue to pass
Documentation requirements
- Make note that if the long comparisons were being used, the new toLong method can be used to convert to convert the timestamp and restore this functionality
- File DOCSP ticket
Follow-Up Requirements
- N/A