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-4770)!: remove 12 length string support from ObjectId constructor #601

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Jul 26, 2023

Description

What is changing?

  • Removing the ability to pass a string that reports a length of 12 from creating an ObjectId
  • Updating the .equals method to correspond
Is there new documentation needed for these changes?

The usage was never reported by the API prior and worked inconsistently, 12 length strings interpreted as utf8 / iso88591 interchangeably. I think this release note serves as enough documentation.

What is the motivation for this change?

See notes, and doc section.

Release Highlight

Strings of length 12 can no longer make an ObjectId

(From String.length): [The String length] property returns the number of code units in the string. JavaScript uses UTF-16 encoding, where each Unicode character may be encoded as one or two code units, so it's possible for the value returned by length to not match the actual number of Unicode characters in the string.

The ObjectId constructor erroneously interpreted a string with length of 12 as UTF8 bytes that could be converted to an ObjectId. This is unexpected for at least two reasons. The first is that a legacy approach (pre- Uint8Arrays) to handling binary data was to pass around "binary strings", where each character represents a single byte, this is not the same as interpreting a sting as UTF8, which has restrictions on how each byte can be formatted. The second is that a string of length 12 does not result in 12 bytes of data when converted to utf8 (ex. '🐶🐶🐶🐶🐶🐶'.length === 12, but as UTF8 bytes this is a 24-byte sequence).

Despite the bugginess of the behavior discussed above, the right string in the right context does create the proper byte sequence, so we are considering this a breaking change and removing it in this major release.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken force-pushed the NODE-4770-oid-string branch from 4785076 to 0d4588c Compare July 26, 2023 17:37
@nbbeeken nbbeeken marked this pull request as ready for review July 26, 2023 17:58
@W-A-James W-A-James self-requested a review July 27, 2023 19:46
@W-A-James W-A-James self-assigned this Jul 27, 2023
@W-A-James W-A-James added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jul 27, 2023
test/node/object_id.test.ts Outdated Show resolved Hide resolved
@W-A-James
Copy link
Contributor

Also, if you could put the dependency changes in the ticket AC, that would be great.

@nbbeeken nbbeeken requested a review from W-A-James July 28, 2023 18:25
@W-A-James W-A-James added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jul 28, 2023
@W-A-James W-A-James merged commit 409c592 into main Aug 1, 2023
@W-A-James W-A-James deleted the NODE-4770-oid-string branch August 1, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants