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-4892)!: error on bson types not from this version #543

Merged
merged 26 commits into from
Jan 13, 2023

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Dec 15, 2022

Description

What is changing?

  • Added new BSONValue super class that all BSON type classes now extend from
  • Added a version number to all BSON types via inheritance
  • Added assertions to serialize, stringify, calculateObjectSize that all fail if the version is not the same as the constant

What is the motivation for this change?

Explicitly breaks cases where old BSON types fed into this version of the library may be silently ignored / serialized in a corrupted form.

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
    • 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-4892-version-tag branch from f9cabe5 to 1be51f5 Compare December 15, 2022 20:21
@nbbeeken nbbeeken marked this pull request as ready for review December 15, 2022 20:56
@nbbeeken
Copy link
Contributor Author

This PR still needs a migration guide update, but code is ready

@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Dec 16, 2022
src/binary.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken force-pushed the NODE-4892-version-tag branch from 1be51f5 to c95f4c1 Compare December 16, 2022 18:41
durran
durran previously approved these changes Dec 20, 2022
@nbbeeken nbbeeken added the Blocked Blocked on other work label Dec 20, 2022
@nbbeeken nbbeeken removed the Blocked Blocked on other work label Jan 5, 2023
@nbbeeken nbbeeken requested a review from durran January 5, 2023 21:36
@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 Jan 10, 2023
durran
durran previously approved these changes Jan 12, 2023
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

Yeah I think I like this now even with the super() calls. Nice work.

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.

We need to update the migration guide as well.

src/parser/calculate_size.ts Outdated Show resolved Hide resolved
test/node/bson_type_classes.test.ts Show resolved Hide resolved
test/node/cross_compat.test.ts Outdated Show resolved Hide resolved
test/node/bson_type_classes.test.ts Show resolved Hide resolved
src/symbol.ts Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
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.

@nbbeeken migration guide should be updated here

src/constants.ts Outdated Show resolved Hide resolved
src/error.ts Outdated Show resolved Hide resolved
src/max_key.ts Show resolved Hide resolved
src/min_key.ts Show resolved Hide resolved
test/node/symbol.test.ts Show resolved Hide resolved
test/node/parser/serializer.test.ts Show resolved Hide resolved
test/node/parser/calculate_size.test.ts Show resolved Hide resolved
docs/upgrade-to-v5.md Show resolved Hide resolved
@nbbeeken nbbeeken requested a review from dariakp January 12, 2023 23:29
src/bson_value.ts Show resolved Hide resolved
@durran durran merged commit d9f0eaa into main Jan 13, 2023
@durran durran deleted the NODE-4892-version-tag branch January 13, 2023 16:39
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