Skip to content

CDRIVER-4098 Bump WIRE_VERSION_MIN from 3 to 6 #926

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

Merged
merged 24 commits into from
Jan 28, 2022

Conversation

eramongodb
Copy link
Contributor

Per CDRIVER-4098:

MongoDB 3.4 reached end of life in January of 2020. MongoDB 3.6 (which reached end of life in April 2021, but seems likely to still exist in Atlas for some time) introduced OP_MSG, which replaced the previous wire protocol for MongoDB. 3.6 seems like the right new minimum server version.

Therefore, the minimum wire version supported by the C driver should now be wire version 6. Correspondingly, this PR modifies the WIRE_VERSION_MIN macro defined in mongoc-client-private.h and raises it from 3 to 6.

Bumping WIRE_VERSION_MIN

The minimum wire version is enforced in the client implementation by _mongoc_topology_description_check_compatible(), which is invoked by mongoc_topology_description_handle_hello(). There are also various feature compatibility checks spread throughout the client, tested as demanded by corresponding operations. These checks have been left unchanged. Followup changes to simplify or remove the client implementation according to the new minimum wire version are not included in this PR.

request parameter for mock_server_replies_to_find()

Numerous tests are written using a pattern similar to the following:

future = future_cursor_next (cursor, &doc);
request = mock_server_receives_request (server);
mock_server_replies_to_find (request, /* ... */);

Despite mock_server_receives_request() potentially returning a NULL result (on timeout, such as due to no compatible servers being found), the test code and mock_server_replies_to_find() assume request is not-null. A precondition check was therefore added to improve the resulting error message when this condition is violated.

Minimum wire version compatibility error message

Previously, the C driver reports compatibility failure with the following message:

Server at %{host_and_port} reports wire version %{max_wire_version}, but this version of libmongoc requires at least 3 (MongoDB 3.0)

The hardcoded wire version and server version numbers are replaced with substitution of WIRE_VERSION_MIN and a new _mongoc_wire_version_to_server_version() function to obtain the corresponding server version number:

Server at %{host_and_port} reports wire version %{max_wire_version}, but this version of libmongoc requires at least %{WIRE_VERSION_MIN} (MongoDB %{server_version})

Removal of Evergreen tasks

All tasks designed to be tested against MongoDB server versions earlier than 3.6 (corresponding to wire version < 6) have been removed. 220 tasks were removed in total, bringing the total number of Evergreen tasks for the mongo-c-project down from 1040 to 820.

Minimum wire version check in mock servers

The mock server creation functions were updated to forbid max_wire_version arguments with values below WIRE_VERSION_MIN, and branches conditioned on wire version < 6 were removed accordingly. Tests that require specifying a minWireVersion lower than WIRE_VERSION_MIN (or a maxWireVersion greater than WIRE_VERSION_MAX) are still free to do so using the mock_server_auto_hello() function, such as in test_wire_version().

Test code conditioned on wire version < 6

All checks assuming wire version >= 6 were removed for redundancy, and all tests requiring wire version < 6 were removed and remaining test names modified accordingly. The full diff of old tests vs. new tests are as follows:

Before                                                | After
/BulkOperation/opts/collation/multi/w0/wire4          <
/BulkOperation/opts/collation/multi/w0/wire5          | /BulkOperation/opts/collation/multi/w0
/BulkOperation/opts/collation/multi/w1/wire4          <
/BulkOperation/opts/collation/multi/w1/wire5          | /BulkOperation/opts/collation/multi/w1
/BulkOperation/opts/collation/w0/wire4                <
/BulkOperation/opts/collation/w0/wire5                | /BulkOperation/opts/collation/w0
/BulkOperation/opts/collation/w1/wire4                <
/BulkOperation/opts/collation/w1/wire5                | /BulkOperation/opts/collation/w1
/BulkOperation/update_arrayfilters/unsupported        <
/Client/command_with_opts/legacy                      <
/Cluster/command/notprimary                           <
/Cluster/command_error/op_query                       <
/Cluster/not_primary/pooled/op_msg                    | /Cluster/not_primary/pooled
/Cluster/not_primary/pooled/op_query                  <
/Cluster/not_primary/single/op_msg                    | /Cluster/not_primary/single
/Cluster/not_primary/single/op_query                  <
/Cluster/not_primary_auth/pooled/op_msg               | /Cluster/not_primary_auth/pooled
/Cluster/not_primary_auth/pooled/op_query             <
/Cluster/not_primary_auth/single/op_msg               | /Cluster/not_primary_auth/single
/Cluster/not_primary_auth/single/op_query             <
/Collection/aggregate/collation/wire4                 <
/Collection/aggregate/collation/wire5                 | /Collection/aggregate/collation
/Collection/count/collation/wire4                     <
/Collection/count/collation/wire5                     | /Collection/count/collation
/Collection/find_and_modify/write_concern_pre_32      <
/Collection/find_with_opts/collation/error            <
/Collection/index/collation/wire4                     <
/Collection/index/collation/wire5                     | /Collection/index/collation
/counters/op_getmore_killcursors                      <
/counters/op_query                                    <
/Cursor/client_kill_cursor/with_primary/wv4           <
/Cursor/client_kill_cursor/without_primary/wv4        <
/Cursor/kill/pooled/cmd                               <
/Cursor/kill/single/cmd                               <
/Database/aggregate/writeConcern                      <
/find_and_modify/collation/fail                       <
/find_and_modify/collation/ok                         | /find_and_modify/collation
/find_and_modify/find_and_modify/write_concern_pre_32 <
/ReadConcern/allowed/explicit                         | /ReadConcern/explicit
/ReadConcern/allowed/inherited                        | /ReadConcern/inherited
/ReadConcern/prohibited/explicit                      <
/ReadConcern/prohibited/inherited                     <
/WriteCommand/split_opquery_with_options              <
/WriteCommand/w0_legacy_insert_many                   <
/WriteCommand/w0_legacy_update_and_replace_validation <
/WriteCommand/w0_legacy_update_one                    <
/WriteConcern/allowed                                 | /WriteConcern
/WriteConcern/prohibited                              <

Wire version integer literals -> wire version feature macros

All instances of the wire version being specified using an integer literal (e.g. mock_server_with_auto_hello (6)) were updated to use a corresponding wire version macro instead (e.g. mock_server_with_auto_hello (WIRE_VERSION_OP_MSG)). Any instances of test code using wire version literals < 6 were updated or removed accordingly.

Wire version string literals -> wire version feature macros

Similarly, all instances of the wire version being specified as part of a string literal (e.g. "'minWireVersion': 6") were updated to use a corresponding wire version macro instead (e.g. "'minWireVersion': %d" + WIRE_VERSION_MIN). Any existing instances of hardcoded 'minWireVersion' were replaced with WIRE_VERSION_MIN, and any existing instances of 'maxWireVersion' were replaced with WIRE_VERSION_MAX. Any instances of test code using wire version literals < 6 were updated or removed accordingly.

mock_server_receives_bulk_msg()

This function was added primarily to assist with updating the test_bulk_error_unordered() test.

mock_server_receives_command() -> mock_server_receives_msg()

This constitutes the majority of the changes in this PR. Wire version 6 introduces support for OP_MSG, and the client is implemented to prefer OP_MSG over OP_QUERY if able. Therefore, numerous tests written assuming the mock server receives an OP_QUERY (via mock_server_receives_command() or mock_server_receives_query()) must be updated to expect and handle an OP_MSG (via mock_server_receives_msg()) instead. Most of the changes followed patterns similar to one of the following:

- mock_server_receives_command (server, "db", MONGOC_QUERY_SECONDARY_OK, "{'ping': 1}");
+ mock_server_receives_msg (server, MONGOC_MSG_NONE, tmp_bson ("{'$db': 'db', 'ping': 1}"));
- mock_server_receives_command (mock_server, "test", MONGOC_QUERY_NONE, "{..., 'documents': [{'_id': 1}]}");
+ mock_server_receives_msg (mock_server, MONGOC_MSG_NONE, tmp_bson ("{'$db': 'db', ...}"), tmp_bson ("{'_id': 1}"));
- mock_server_receives_query (server, "db.test", MONGOC_QUERY_SECONDARY_OK, 0, 0, "{}", NULL);
+ mock_server_receives_msg (server, MONGOC_MSG_NONE, tmp_bson ("{'$db': 'db', 'find': 'test', 'filter': {}}"));

The mock_server_receives_query() function cannot yet be removed due to being used for testing support for exhaust cursor fallback behavior.

Several minor changes to contents of messages were also required to address new '$readPreference' inclusion behavior, new '$writeConcern' inclusion behavior, and fields requiring explicit '$numberLong'.

As drive-by fixes, mock_server_auto_endsessions() and capture_logs() were added to several tests to improve both test suite performance and reduce noise in test output.

@eramongodb eramongodb self-assigned this Jan 13, 2022
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Nicely done. The test code removal and Evergreen matrix removal seems like a big step towards better maintainability. Thank you for splitting up the diff into logical commits.

@eramongodb eramongodb requested a review from kevinAlbs January 18, 2022 17:37
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

Many many small changes, but nothing suspicious. LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants