Skip to content

CDRIVER-4093 fix versioned API sample tests #848

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.

Sign up for GitHub

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 7 commits into from
Aug 18, 2021
Merged

CDRIVER-4093 fix versioned API sample tests #848

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
555d218
update download-mongodb.sh
kevinAlbs Aug 13, 2021
f6eaebe
create clients in vapi examples without vapi
kevinAlbs Aug 13, 2021
ebe34bd
use mongoc_collection_count instead of mongoc_database_command_simple
kevinAlbs Aug 13, 2021
33251f6
remove extra bson_destroy
kevinAlbs Aug 13, 2021
0ddb170
remove debug monitoring
kevinAlbs Aug 13, 2021
9c8a5d4
revert unnecessary count cmd change
kevinAlbs Aug 13, 2021
afa584b
address jesse + colby comments
kevinAlbs Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .evergreen/download-mongodb.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ get_mongodb_download_url_for ()
_DISTRO=$1
_VERSION=$2

VERSION_50="5.0.0"
VERSION_50="5.0.2"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

download-mongodb.sh is copied from mongodb-labs/driver-evergreen-tools. This is required to get 5.0 server version with SERVER-58794 fixed.

On 5.0.0, the collection create command fails against sharded clusters when a versioned API is required. That was causing this test-asan-5.0-sharded-noauth-nosasl-nossl test failure.

VERSION_44="4.4.6"
VERSION_42="4.2.15"
VERSION_40="4.0.25"
Expand Down
74 changes: 49 additions & 25 deletions src/libmongoc/tests/test-mongoc-sample-commands.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3435,6 +3435,20 @@ get_client (void)
return test_framework_new_default_client ();
}

/* Returns a test client without version API options configured. */
static mongoc_client_t *
get_client_for_version_api_example (void) {
mongoc_client_t *client;
mongoc_uri_t *uri;

uri = test_framework_get_uri ();
client = mongoc_client_new_from_uri (uri);
ASSERT (client);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that since this is test code that we expect to just terminate, there's no need to release the "client" resource?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Callers of get_client_for_version_api_example are expected to call mongoc_client_destroy to release the client.

test_framework_set_ssl_opts (client);
mongoc_uri_destroy (uri);
return client;
}

static bool
callback (mongoc_client_session_t *session,
void *ctx,
Expand Down Expand Up @@ -3580,6 +3594,7 @@ _test_sample_versioned_api_example_1 (void)
mongoc_server_api_t *server_api = NULL;
mongoc_server_api_version_t server_api_version;
bson_error_t error;
bool ok;

/* For a replica set, include the replica set name and a seedlist of the
* members in the URI string; e.g.
Expand All @@ -3592,12 +3607,13 @@ _test_sample_versioned_api_example_1 (void)
* client = mongoc_client_new (uri_sharded);
*/

client = get_client ();
client = get_client_for_version_api_example ();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

get_client calls test_framework_new_default_client, which calls test_framework_client_new.

test_framework_client_new applies server API options if required by the test environment.

This test sets server API options on the client. To avoid setting duplicate options, the client should not be created with default server API options.

For more background: the environment variable MONGODB_API_VERSION controls whether test clients have server API options set by default..

MONGODB_API_VERSION is set on the versioned-api variant to run the test suite against servers expecting a server API options in all commands. Those tests are specified in the Versioned API test readme

If the environment variable is set, all clients created in tests MUST declare the ServerApiVersion specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding a comment in the code about the API options note?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, I added a note to say that get_client_for_version_api_example returns a client without server API options configured.


mongoc_server_api_version_from_string ("1", &server_api_version);
server_api = mongoc_server_api_new (server_api_version);

mongoc_client_set_server_api (client, server_api, &error);
ok = mongoc_client_set_server_api (client, server_api, &error);
BSON_ASSERT (ok);
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 care much that BSON_ASSERT is appearing in the API examples (as opposed to the regular C assert())?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. Though BSON_ASSERT is part of the public libbson API, and is defined in bson-macros.h, I think it makes sense to use assert since readers are likely to be familiar with assert.

I looked through other examples code for reference. There aren't many assertions, but there are instances of both uses of BSON_ASSERT and assert.

https://github.com/mongodb/mongo-c-driver/blob/3a41062e397638399a6e491ec94bd6c861ec212c/src/libmongoc/examples/example-update.c#L52:L52 uses BSON_ASSERT.

https://github.com/mongodb/mongo-c-driver/blob/3a41062e397638399a6e491ec94bd6c861ec212c/src/libmongoc/examples/example-gridfs.c#L56:L56 uses assert.

/* End Versioned API Example 1 */

mongoc_client_destroy (client);
Expand All @@ -3612,6 +3628,7 @@ _test_sample_versioned_api_example_2 (void)
mongoc_server_api_t *server_api = NULL;
mongoc_server_api_version_t server_api_version;
bson_error_t error;
bool ok;

/* For a replica set, include the replica set name and a seedlist of the
* members in the URI string; e.g.
Expand All @@ -3624,13 +3641,14 @@ _test_sample_versioned_api_example_2 (void)
* client = mongoc_client_new (uri_sharded);
*/

client = get_client ();
client = get_client_for_version_api_example ();

mongoc_server_api_version_from_string ("1", &server_api_version);
server_api = mongoc_server_api_new (server_api_version);
mongoc_server_api_strict (server_api, true);

mongoc_client_set_server_api (client, server_api, &error);
ok = mongoc_client_set_server_api (client, server_api, &error);
BSON_ASSERT (ok);
/* End Versioned API Example 2 */

mongoc_client_destroy (client);
Expand All @@ -3645,6 +3663,7 @@ _test_sample_versioned_api_example_3 (void)
mongoc_server_api_t *server_api = NULL;
mongoc_server_api_version_t server_api_version;
bson_error_t error;
bool ok;

/* For a replica set, include the replica set name and a seedlist of the
* members in the URI string; e.g.
Expand All @@ -3657,13 +3676,14 @@ _test_sample_versioned_api_example_3 (void)
* client = mongoc_client_new (uri_sharded);
*/

client = get_client ();
client = get_client_for_version_api_example ();

mongoc_server_api_version_from_string ("1", &server_api_version);
server_api = mongoc_server_api_new (server_api_version);
mongoc_server_api_strict (server_api, false);

mongoc_client_set_server_api (client, server_api, &error);
ok = mongoc_client_set_server_api (client, server_api, &error);
BSON_ASSERT (ok);
/* End Versioned API Example 3 */

mongoc_client_destroy (client);
Expand All @@ -3678,6 +3698,7 @@ _test_sample_versioned_api_example_4 (void)
mongoc_server_api_t *server_api = NULL;
mongoc_server_api_version_t server_api_version;
bson_error_t error;
bool ok;

/* For a replica set, include the replica set name and a seedlist of the
* members in the URI string; e.g.
Expand All @@ -3690,13 +3711,14 @@ _test_sample_versioned_api_example_4 (void)
* client = mongoc_client_new (uri_sharded);
*/

client = get_client ();
client = get_client_for_version_api_example ();

mongoc_server_api_version_from_string ("1", &server_api_version);
server_api = mongoc_server_api_new (server_api_version);
mongoc_server_api_deprecation_errors (server_api, true);

mongoc_client_set_server_api (client, server_api, &error);
ok = mongoc_client_set_server_api (client, server_api, &error);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'm ok with "ok", but it might be okay-er to just check the call with BSON_ASSERT directly, if this is the only place we're using it. But, really, I'm ok either way!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, here and in the other changes!

BSON_ASSERT (ok);
/* End Versioned API Example 4 */

mongoc_client_destroy (client);
Expand All @@ -3721,11 +3743,10 @@ static void _test_sample_versioned_api_example_5_6_7_8 (void) {
bson_t *docs[N_DOCS];
int i;
bson_t reply;
bson_t *cmd;
int64_t count;
bson_t *filter;

client = get_client ();
client = get_client_for_version_api_example ();
mongoc_client_set_error_api (client, MONGOC_ERROR_API_VERSION_2);
mongoc_server_api_version_from_string ("1", &server_api_version);
server_api = mongoc_server_api_new (server_api_version);
Expand Down Expand Up @@ -3789,24 +3810,28 @@ static void _test_sample_versioned_api_example_5_6_7_8 (void) {
ASSERT_OR_PRINT (ok, error);
bson_destroy (&reply);

cmd = BCON_NEW ("count", "sales");
ok = mongoc_database_command_simple (
db, cmd, NULL /* read_prefs */, &reply, &error);
ASSERT_ERROR_CONTAINS (error,
MONGOC_ERROR_SERVER,
323,
"Provided apiStrict:true, but the command count is not in API Version 1");
ASSERT (!ok);
bson_destroy (&reply);
BEGIN_IGNORE_DEPRECATIONS;
count = mongoc_collection_count (sales,
MONGOC_QUERY_NONE,
NULL /* query */,
0 /* skip */,
0 /* limit */,
NULL /* read prefs */,
&error);
END_IGNORE_DEPRECATIONS;
ASSERT_ERROR_CONTAINS (
error,
MONGOC_ERROR_SERVER,
323,
"Provided apiStrict:true, but the command count is not in API Version 1");
ASSERT_CMPINT64 (-1, ==, count);
#if 0
/* This block not evaluated, but is inserted into documentation to represent the above reply.
* Don't delete me! */
/* Start Versioned API Example 6 */
char *str = bson_as_json (&reply, NULL /* length */);
printf ("%s", str);
/* Prints the server reply:
* { "ok" : 0, "errmsg" : "Provided apiStrict:true, but the command count is not in API Version 1", "code" : 323, "codeName" : "APIStrictError" } */
bson_free (str);
printf ("%s", error.message);
/* Prints the error message:
* "Provided apiStrict:true, but the command count is not in API Version 1. Information on supported commands and migrations in API Version 1 can be found at https://dochub.mongodb.org/core/manual-versioned-api" */
/* End Versioned API Example 6 */
#endif

Expand All @@ -3825,7 +3850,6 @@ static void _test_sample_versioned_api_example_5_6_7_8 (void) {
/* End Versioned API Example 8 */

bson_destroy (filter);
bson_destroy (cmd);
for (i = 0; i < N_DOCS; i++) {
bson_destroy (docs[i]);
}
Expand Down