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-3469,NODE-3615,NODE-3507): update min and max wire versions #3014

Merged
merged 25 commits into from
Nov 2, 2021

Conversation

durran
Copy link
Member

@durran durran commented Oct 24, 2021

Description

Updates the minimum wire version for the driver to 6 (MongoDB 3.6) and max wire version to 14 (MongoDB 5.1). Also includes NODE-3615 for bumping max wire version and NODE-3507 for updating SDAM spec tests to raise the max wire version.

What is changing?

The minimum wire version in the wire protocol has been updated to 6, so MongoDB 3.6, as well as the maximum version number to 5.1 and wire version to 14 for the upcoming 5.1 release. (instead of 5.0). Tests have been added to check the relevant constants.

Is there new documentation needed for these changes?

Perhaps the minimum server version that the driver supports is now 3.6 and not 2.6

What is the motivation for this change?

DRIVERS-1315 to deprecate support for EOL MongoDB versions is the motivation.

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>
  • Changes are covered by tests

@durran
Copy link
Member Author

durran commented Oct 24, 2021

Failures can be resolved moving the default mock ismaster to the 3.6 version.

@dariakp
Copy link
Contributor

dariakp commented Oct 25, 2021

@durran The ticket says that this work depends on https://jira.mongodb.org/browse/NODE-3596 - which has some spec changes to pull in, are the failures related to that by any chance?

@nbbeeken nbbeeken self-requested a review October 29, 2021 18:51
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Oct 29, 2021
@dariakp dariakp changed the title feat(NODE-3469): update min wire version to 6 feat(NODE-3469,NODE-3615,NODE-3507): update min and max wire versions Oct 29, 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.

Seems pretty much good to go! Thanks!

test/functional/collations.test.js Outdated Show resolved Hide resolved
src/operations/list_collections.ts Show resolved Hide resolved
@durran durran requested a review from nbbeeken November 1, 2021 09:47
@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 Nov 1, 2021
@nbbeeken nbbeeken requested a review from dariakp November 1, 2021 15:52
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.

Just a few questions here and a suggestion. Also, since we are removing tests but not the tested codepaths, can we file a follow up ticket to clean those up? (i.e., remove anything that checks an out of bound version; in particular, perhaps we can update the ServerCapabilities to always return true for the corresponding functionalities). On that note, there's also a couple of skipped change_stream tests that still have the maxWireVersion set to 4, which should probably be bumped.

test/functional/unit-sdam/srv_polling.test.js Outdated Show resolved Hide resolved
ns: 'test.test'
}
});
} catch (e) {
console.log('error', e);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to keep the console logs here and in L91?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Missed the one in L91, but I took the liberty of deleting it myself since it's the only outstanding thing here to merge

Copy link
Contributor

Choose a reason for hiding this comment

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

I also filed a follow up NODE-3469 for the other clean up

test/tools/mock.js Outdated Show resolved Hide resolved
@durran durran requested a review from dariakp November 2, 2021 08:52
@matthewmayer
Copy link

matthewmayer commented Nov 19, 2021

Shouldn't this a semver-major breaking change, since the minimum wire version is increased to 6, which drops support for MongoDB3.2/3.4

@dariakp
Copy link
Contributor

dariakp commented Nov 20, 2021

Hi @matthewmayer, thanks for reaching out: this change was intentional in accordance with our MongoDB Software Lifecycle Schedules. Given the server versions 3.4 and earlier are outside of our support policy range, and have been since January 2020, we no longer test our driver against them and therefore can no longer guarantee compatibility. In the interest of avoiding undefined behavior or silent failures we opted for a thrown error to make sure users are made fully aware of the updated support.

We understand that this could be an unexpected change for some users; however, given the EOL status of the platforms, we expect the real impact to be minimal. We are working on a strategy to better communicate server version support changes in the future that aligns with our configurable Versioned API feature and, of course, we are always listening to the feedback from our community.

@matthewmayer
Copy link

I understand it is intentional, but it seems this should have been released as library version 5.0.0 not 4.2.0 following correct semver principles to minimize possible breakage.

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