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

fix(NODE-3760): ObjectId.isValid string and byte length match #475

Merged
merged 4 commits into from
Dec 2, 2021

Conversation

syz99
Copy link
Contributor

@syz99 syz99 commented Dec 1, 2021

Description

What is changing?

adds validation for string and byte length matching for new ObjectID's.

What is the motivation for this change?

bugs found with strings of char-length 12, but byte-length =/= 12.

Double check the following

  • Ran npm run lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@syz99 syz99 self-assigned this Dec 1, 2021
@syz99 syz99 changed the title fix(NODE-3760): fix isValid str/byte length match bug, add isValid t… fix(NODE-3760): fix ObjectId.isValid str/byte length match bug Dec 1, 2021
@syz99 syz99 marked this pull request as ready for review December 1, 2021 19:19
src/objectid.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Dec 1, 2021
@syz99 syz99 requested a review from nbbeeken December 1, 2021 20:17
nbbeeken
nbbeeken previously approved these changes Dec 1, 2021
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Dec 1, 2021
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this check is only in the isValid method, what we discussed regarding the implementation when we triaged this is in the comment on the ticket:

what we need to do is add logic to catch this case in the constructor and then also add corresponding logic in the isValid check so that isValid uses the same criteria as the constructor to determine validity

@nbbeeken do you guys by any chance have some sort of kickoff notes about what is and isn't in scope for the implementation? because without unifying constructor logic with the validation logic we are just making a bandaid fix without actually improving the validation/constructor duality

@syz99
Copy link
Contributor Author

syz99 commented Dec 1, 2021

@dariakp

Added retroactive kickoff notes; Grace's fix already addresses the user's specific issue in constructor, we're now syncing isValid to match:

Issue with constructor was fixed in NODE-3662; we can instead fix isValid to match the constructor's behavior. User's given example doesn't have the behavior mentioned where .id is undefined. This work should correct isValid to return true/false based on constructor's corrected behavior. 

@dariakp
Copy link
Contributor

dariakp commented Dec 1, 2021

@syz99 thanks for double checking! I am still wondering though if we can consolidate the validation logic so that we don't have to rely on duplicating the correct validation blocks in the isValid function; have we considered if there can be a general validation method that can be used in both places or even the approach of calling the constructor to create an ObjectId in the isValid function?

@syz99 syz99 changed the title fix(NODE-3760): fix ObjectId.isValid str/byte length match bug fix(NODE-3760): ObjectId.isValid string and byte length match Dec 1, 2021
@baileympearson
Copy link
Contributor

@syz99 thanks for double checking! I am still wondering though if we can consolidate the validation logic so that we don't have to rely on duplicating the correct validation blocks in the isValid function; have we considered if there can be a general validation method that can be used in both places or even the approach of calling the constructor to create an ObjectId in the isValid function?

👍 to the idea of removing duplicate logic when possible! @dariakp Are you wondering about replacing the entirety of the isValid method with a call to the ObjectID constructor or just the branches where id is a string?

@dariakp
Copy link
Contributor

dariakp commented Dec 1, 2021

@syz99 thanks for double checking! I am still wondering though if we can consolidate the validation logic so that we don't have to rely on duplicating the correct validation blocks in the isValid function; have we considered if there can be a general validation method that can be used in both places or even the approach of calling the constructor to create an ObjectId in the isValid function?

👍 to the idea of removing duplicate logic when possible! @dariakp Are you wondering about replacing the entirety of the isValid method with a call to the ObjectID constructor or just the branches where id is a string?

The former, try/catch around the call and return true/false accordingly; it's the most straightforward way to maintain parity given how the validation is split up; it'll be less ugly if we break out the overloads into separate functions down the road for the next major versions, but for now there aren't many options to make it pretty aside from the aforementioned one

Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay for less code and more consistency 🎉

Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - nice work!

@nbbeeken nbbeeken merged commit 187d1c4 into master Dec 2, 2021
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.

4 participants