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

usage of ObjectId.isValid() doesn't match implementation

    • 2
    • Not Needed

      Summary

      The method ObjectId.isValid() returns true for some string values that aren't valid.

      I'm using ObjectId.isValid() to check whether a value can be transformed in to an ObjectId or not. However, it returns true for certain values that will cause an error if used. I expect it to return true for any and all values that can be used to create a new ObjectId.

      The method will return true for any and all string values of length 12. This is incorrect since many such values will throw an error.
      Example:
      const val = "value with é"

      ObjectId.isValid(val)  // true

      new ObjectId(val)  // TypeError: this.id is undefined

       

      The above example will throw an error (at least with the node.js driver.).
      _I propose to update the isValid-method to check any string of length 12 with the checkForHexRegExp-function as is done with strings of length 24.
      Row 301 here: https://github.com/mongodb/js-bson/blob/master/src/objectid.ts

      If this check of the 12 lengths strings is missing from the other drivers I propose to add it there as well.
      My assumption is that that regex-function (checkForHexRegExp) correctly validates the content of the string to be used with new ObjectId().
      _

      Motivation

      Who is the affected end user?

      Anyone checking the validity of a value to create a new ObjectId

      Who are the stakeholders?

      How does this affect the end user?

      Are they blocked? Are they annoyed? Are they confused?

      Confused. They will assume that isValid() returning true means that the value is a valid value to use.

      How likely is it that this problem or use case will occur?

      Main path? Edge case?
      It's somewhat likely.

      If the problem does occur, what are the consequences and how severe are they?

      Minor annoyance at a log message? Performance concern? Outage/unavailability? Failover can't complete?

      The entire admin page of my app is made unavailable by this. I didn't expect the isValid() method to throw a type error and thus didn't handle the exception.

      Is this issue urgent?

      Does this ticket have a required timeline? What is it?
      No. Not really.

      Is this ticket required by a downstream team?

      Needed by e.g. Atlas, Shell, Compass?

      Is this ticket only for tests?

      Is this ticket have any functional impact, or is it just test improvements?
      No

            Assignee:
            sam.zhang@mongodb.com Sam Zhang
            Reporter:
            emanuel.lindstrom@gmail.com Emanuel Lindström
            Neal Beeken
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: