Uploaded image for project: 'Node.js Driver'
  1. Node.js Driver
  2. NODE-2624

BSON Timestamp should wrap its value and present as an unsigned long

XMLWordPrintableJSON

    • Type: Icon: Improvement Improvement
    • Resolution: Unresolved
    • Priority: Icon: Major - P3 Major - P3
    • None
    • Affects Version/s: bson-4.0.4
    • Component/s: None
    • 2
    • 2
    • Hide

      Review the ticket description for general accuracy and completeness

      • Bug - Confirm that the bug still exists
      • Task / Feature / Improvement - Ensure every section of the template is filled out and makes sense
      • Build failure - Investigate and confirm the cause of the build failure
      • Spec change - Check whether any more recent changes have been made to the spec that might affect the implementation requirements

      What is the expected behavior?

      • What do the official driver or server docs currently say about this functionality?
        • What should they say?
          • If revisions or additions are needed, mark the ticket as docs changes needed and fill out the doc changes form
      • What do our api or readme docs currently say about this functionality?
        • Lists inherited Long methods
        • What should they say?
          • Should not list inherited Long methods
        • Capture any revisions or additions in the ticket documentation AC
          • Done
      • If applicable, what does the common drivers spec say? (Note: your kickoff partner should independently review the spec)
        • Are any clarifications or revisions needed?
      • If applicable, what do other drivers do?
        • If there is no common spec, is a common spec needed?
      • What should the behavior be?
        • Should not inherit from Long
      • Update the ticket description and implementation requirements as needed
        • Done

      Review and address any unknowns explicitly called out in the ticket

      • Done

      What will be the impact on users?

      • Who will be impacted?
        • Done
      • Why might users care about this change?
        • Done
      • Capture relevant detail in the "User Impact" section of the ticket description
        • Done

      What will be the impact on any downstream projects? (e.g., shell, mongoose)

      • Update follow up requirements and create subtasks for follow up or coordination actions
        • Done

      What variables affect the feature in question?

      • Server versions
      • Deployment types
      • Auth settings
      • Server and client configuration options
      • Specific apis / api options
      • Runtime or bundler settings
      • Special sequences of operations
      • Any other special conditions

      How should all the identified variables be tested?

      • Identify happy path and error case combinations of variables
        • Given [variables], when [action is performed], [feature] should [behave in the expected way]
      • How will we achieve the necessary coverage for these cases?
        • Automated spec tests?
          • Are there test runner changes required?
          • How up to date are our current tests and runners?
        • New integration or prose tests?
        • Unit tests?
          • Yes, see AC
      • Will we need to modify any existing tests?
        • No
      • Is there technical debt that will affect the implementation of new or existing tests?
        • No
      • Do we have the necessary tooling infrastructure already in place for any new tests?
        • Yes
      • Update test requirements on the ticket to reflect reality
        • Done
      • Create subtasks for any testing groundwork that can happen independently of the implementation

      What is the scope of the code changes?

      • List the code bases and the areas of each code base that will need changes
        • js-bson, src/timestamp.ts
        • note that there are uses in the driver of Long methods on Timestamps, but these uses will be handled in NODE-5242
      • Is there technical debt in any of these areas that will affect the implementation?
      • Identify any existing adjacent functionality that could be impacted by these changes
        • Is there sufficient existing test coverage for the adjacent functionality?
          • Update ticket test AC and create subtask(s) to cover existing functionality if coverage is missing
      • If multiple libraries are affected, determine the order in which changes need to go in
      • Create subtasks for the implementation (at least one per affected codebase)

      What is the expected impact on performance?

      • Unsure, but should not be significant
      • Do we have existing performance coverage for the affected areas?
        • yes, in the driver
      • Do we need to add new coverage?
        • No
        • Update ticket test AC and create subtask(s) as needed

      Consider backport requirements

      • Should this be backported?
        • No; breaking change
      • What would be the cost of a backport?

      Is the metadata of this ticket accurate and complete?

      • Double check the acceptance criteria to ensure it accurately captures the expected behavior, test, and follow-up requirements
        • Done
      • Double check the documentation requirements
        • Done
      • Double check the task breakdown to ensure it covers all actionable items in the ticket AC
        • Done
      Show
      Review the ticket description for general accuracy and completeness Bug - Confirm that the bug still exists Task / Feature / Improvement - Ensure every section of the template is filled out and makes sense Build failure - Investigate and confirm the cause of the build failure Spec change - Check whether any more recent changes have been made to the spec that might affect the implementation requirements What is the expected behavior? What do the official driver or server docs currently say about this functionality? What should they say? If revisions or additions are needed, mark the ticket as docs changes needed and fill out the doc changes form What do our api or readme docs currently say about this functionality? Lists inherited Long methods What should they say? Should not list inherited Long methods Capture any revisions or additions in the ticket documentation AC Done If applicable, what does the common drivers spec say? (Note: your kickoff partner should independently review the spec) Are any clarifications or revisions needed? If applicable, what do other drivers do? If there is no common spec, is a common spec needed? What should the behavior be? Should not inherit from Long Update the ticket description and implementation requirements as needed Done Review and address any unknowns explicitly called out in the ticket Done What will be the impact on users? Who will be impacted? Done Why might users care about this change? Done Capture relevant detail in the "User Impact" section of the ticket description Done What will be the impact on any downstream projects? (e.g., shell, mongoose) Update follow up requirements and create subtasks for follow up or coordination actions Done What variables affect the feature in question? Server versions Deployment types Auth settings Server and client configuration options Specific apis / api options Runtime or bundler settings Special sequences of operations Any other special conditions How should all the identified variables be tested? Identify happy path and error case combinations of variables Given [variables] , when [action is performed] , [feature] should [behave in the expected way] How will we achieve the necessary coverage for these cases? Automated spec tests? Are there test runner changes required? How up to date are our current tests and runners? New integration or prose tests? Unit tests? Yes, see AC Will we need to modify any existing tests? No Is there technical debt that will affect the implementation of new or existing tests? No Do we have the necessary tooling infrastructure already in place for any new tests? Yes Update test requirements on the ticket to reflect reality Done Create subtasks for any testing groundwork that can happen independently of the implementation What is the scope of the code changes? List the code bases and the areas of each code base that will need changes js-bson, src/timestamp.ts note that there are uses in the driver of Long methods on Timestamps, but these uses will be handled in NODE-5242 Is there technical debt in any of these areas that will affect the implementation? Identify any existing adjacent functionality that could be impacted by these changes Is there sufficient existing test coverage for the adjacent functionality? Update ticket test AC and create subtask(s) to cover existing functionality if coverage is missing If multiple libraries are affected, determine the order in which changes need to go in Create subtasks for the implementation (at least one per affected codebase) What is the expected impact on performance? Unsure, but should not be significant Do we have existing performance coverage for the affected areas? yes, in the driver Do we need to add new coverage? No Update ticket test AC and create subtask(s) as needed Consider backport requirements Should this be backported? No; breaking change What would be the cost of a backport? Is the metadata of this ticket accurate and complete? Double check the acceptance criteria to ensure it accurately captures the expected behavior, test, and follow-up requirements Done Double check the documentation requirements Done Double check the task breakdown to ensure it covers all actionable items in the ticket AC Done
    • Hide

      1. What would you like to communicate to the user about this feature?
      2. Would you like the user to see examples of the syntax and/or executable code and its output?
      3. Which versions of the driver/connector does this apply to?

      Show
      1. What would you like to communicate to the user about this feature? 2. Would you like the user to see examples of the syntax and/or executable code and its output? 3. Which versions of the driver/connector does this apply to?

      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

      1. 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.
      2. Which methods do we want to keep as part of our Timestamp API?
        • See AC
      3. 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

            Assignee:
            warren.james@mongodb.com Warren James
            Reporter:
            matt.broadstone@mongodb.com Matt Broadstone
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated: