CDRIVER-3929 Include apiVersion in handshake commands (#753)

* Fix tab indentation

* CDRIVER-3929 Include apiVersion in handshake commands

* Protect pooled clients against later API changes

* Fix wrong indentation

* Fix wrong re-initialisation of BSON objects

* Unconditionally destroy ts->api

* Improve handling of server API on pooled clients

* Code review fixes

Author: alcaeus

src/libmongoc/src/mongoc/mongoc-client-pool.c

@@ -51,6 +51,7 @@ struct _mongoc_client_pool_t {
    int32_t error_api_version;
    bool error_api_set;
    mongoc_server_api_t *api;
+   bool client_initialized;
 };
 
 
@@ -240,6 +241,8 @@ _initialize_new_client (mongoc_client_pool_t *pool, mongoc_client_t *client)
       pool->topology->scanner->initiator,
       pool->topology->scanner->initiator_context);
 
+   pool->client_initialized = true;
+   client->is_pooled = true;
    client->error_api_version = pool->error_api_version;
    _mongoc_client_set_apm_callbacks_private (
       client, &pool->apm_callbacks, pool->apm_context);
@@ -528,6 +531,15 @@ mongoc_client_pool_set_server_api (mongoc_client_pool_t *pool,
       return false;
    }
 
+   if (pool->client_initialized) {
+      bson_set_error (error,
+                      MONGOC_ERROR_POOL,
+                      MONGOC_ERROR_POOL_API_TOO_LATE,
+                      "Cannot set server api after a client has been created");
+      return false;
+   }
+
    pool->api = mongoc_server_api_copy (api);
+   _mongoc_topology_scanner_set_server_api (pool->topology->scanner, api);
    return true;
 }

src/libmongoc/src/mongoc/mongoc-client-private.h

@@ -99,6 +99,7 @@ struct _mongoc_client_t {
    mongoc_uri_t *uri;
    mongoc_cluster_t cluster;
    bool in_exhaust;
+   bool is_pooled;
 
    mongoc_stream_initiator_t initiator;
    void *initiator_data;

src/libmongoc/src/mongoc/mongoc-client.c

@@ -3105,6 +3105,14 @@ mongoc_client_set_server_api (mongoc_client_t *client,
    BSON_ASSERT_PARAM (client);
    BSON_ASSERT_PARAM (api);
 
+   if (client->is_pooled) {
+      bson_set_error (error,
+                      MONGOC_ERROR_CLIENT,
+                      MONGOC_ERROR_CLIENT_API_FROM_POOL,
+                      "Cannot set server api on a client checked out from a pool");
+      return false;
+   }
+
    if (client->api) {
       bson_set_error (error,
                       MONGOC_ERROR_CLIENT,
@@ -3114,5 +3122,6 @@ mongoc_client_set_server_api (mongoc_client_t *client,
    }
 
    client->api = mongoc_server_api_copy (api);
+   _mongoc_topology_scanner_set_server_api (client->topology->scanner, api);
    return true;
 }

src/libmongoc/src/mongoc/mongoc-cmd-private.h

@@ -146,6 +146,10 @@ _is_retryable_read (const mongoc_cmd_parts_t *parts,
 void
 _mongoc_cmd_append_payload_as_array (const mongoc_cmd_t *cmd, bson_t *out);
 
+void
+_mongoc_cmd_append_server_api (bson_t *command_body,
+                               const mongoc_server_api_t *api);
+
 BSON_END_DECLS
 
 

src/libmongoc/src/mongoc/mongoc-cmd.c

@@ -796,8 +796,8 @@ _txn_in_progress (mongoc_cmd_parts_t *parts)
    }
 
    return (_mongoc_client_session_txn_in_progress (cs)
-	   /* commitTransaction and abortTransaction count as in progress, too. */
-	   || parts->assembled.is_txn_finish);
+      /* commitTransaction and abortTransaction count as in progress, too. */
+      || parts->assembled.is_txn_finish);
 }
 
 
@@ -827,14 +827,12 @@ mongoc_cmd_parts_assemble (mongoc_cmd_parts_t *parts,
                            bson_error_t *error)
 {
    mongoc_server_description_type_t server_type;
-   mongoc_server_api_t *api;
    mongoc_client_session_t *cs;
    const bson_t *cluster_time = NULL;
    mongoc_read_prefs_t *prefs = NULL;
    const char *cmd_name;
    bool is_get_more;
    const mongoc_read_prefs_t *prefs_ptr;
-   const char *string_version;
    bool ret = false;
 
    ENTRY;
@@ -994,23 +992,8 @@ mongoc_cmd_parts_assemble (mongoc_cmd_parts_t *parts,
          sending a getmore, or if we are in a transaction. */
       if (parts->client->api) {
          if (!is_get_more && !_txn_in_progress (parts)) {
-            api = parts->client->api;
-            string_version = mongoc_server_api_version_to_string (api->version);
-
-            bson_append_utf8 (
-               &parts->assembled_body, "apiVersion", -1, string_version, -1);
-
-            if (api->strict.is_set) {
-               bson_append_bool (
-				 &parts->assembled_body, "apiStrict", -1, api->strict.value);
-            }
-
-            if (api->deprecation_errors.is_set) {
-               bson_append_bool (&parts->assembled_body,
-                                 "apiDeprecationErrors",
-                                 -1,
-                                 api->deprecation_errors.value);
-            }
+            _mongoc_cmd_append_server_api (&parts->assembled_body,
+                                           parts->client->api);
          }
       }
 
@@ -1155,3 +1138,43 @@ _mongoc_cmd_append_payload_as_array (const mongoc_cmd_t *cmd, bson_t *out)
 
    bson_append_array_end (out, &bson);
 }
+
+/*--------------------------------------------------------------------------
+ *
+ * _mongoc_cmd_append_server_api --
+ *    Append versioned API fields to a mongoc_cmd_t
+ *
+ * Arguments:
+ *    cmd The mongoc_cmd_t, which will have versioned API fields added
+ *    api A mongoc_server_api_t holding server API information
+ *
+ * Pre-conditions:
+ *    - @api is initialized.
+ *    - @command_body is initialised
+ *
+ *--------------------------------------------------------------------------
+ */
+void
+_mongoc_cmd_append_server_api (bson_t *command_body,
+                               const mongoc_server_api_t *api)
+{
+   const char *string_version;
+
+   BSON_ASSERT (command_body);
+   BSON_ASSERT (api);
+
+   string_version = mongoc_server_api_version_to_string (api->version);
+
+   bson_append_utf8 (command_body, "apiVersion", -1, string_version, -1);
+
+   if (api->strict.is_set) {
+      bson_append_bool (command_body, "apiStrict", -1, api->strict.value);
+   }
+
+   if (api->deprecation_errors.is_set) {
+      bson_append_bool (command_body,
+                        "apiDeprecationErrors",
+                        -1,
+                        api->deprecation_errors.value);
+   }
+}

src/libmongoc/src/mongoc/mongoc-error.h

@@ -130,7 +130,9 @@ typedef enum {
 
    /* An error related to server version api */
    MONGOC_ERROR_CLIENT_API_ALREADY_SET,
-   MONGOC_ERROR_POOL_API_ALREADY_SET
+   MONGOC_ERROR_CLIENT_API_FROM_POOL,
+   MONGOC_ERROR_POOL_API_ALREADY_SET,
+   MONGOC_ERROR_POOL_API_TOO_LATE
 } mongoc_error_code_t;
 
 MONGOC_EXPORT (bool)

src/libmongoc/src/mongoc/mongoc-topology-scanner-private.h

@@ -109,6 +109,8 @@ typedef struct mongoc_topology_scanner {
    bool negotiate_sasl_supported_mechs;
    bool bypass_cooldown;
    bool speculative_authentication;
+
+   mongoc_server_api_t *api;
 } mongoc_topology_scanner_t;
 
 mongoc_topology_scanner_t *
@@ -223,6 +225,10 @@ bool
 mongoc_topology_scanner_node_in_cooldown (mongoc_topology_scanner_node_t *node,
                                           int64_t when);
 
+void
+_mongoc_topology_scanner_set_server_api (mongoc_topology_scanner_t *ts,
+                                         const mongoc_server_api_t *api);
+
 /* for testing. */
 mongoc_stream_t *
 _mongoc_topology_scanner_tcp_initiate (mongoc_async_cmd_t *acmd);

src/libmongoc/src/mongoc/mongoc-topology-scanner.c

@@ -104,9 +104,32 @@ _jumpstart_other_acmds (mongoc_topology_scanner_node_t *node,
                         mongoc_async_cmd_t *acmd);
 
 static void
-_add_ismaster (bson_t *cmd)
+_add_ismaster (bson_t *cmd, const mongoc_server_api_t *api)
 {
    BSON_APPEND_INT32 (cmd, "isMaster", 1);
+
+   if (api) {
+      _mongoc_cmd_append_server_api (cmd, api);
+   }
+}
+
+static void
+_init_ismaster (mongoc_topology_scanner_t *ts)
+{
+   bson_init (&ts->ismaster_cmd);
+   bson_init (&ts->ismaster_cmd_with_handshake);
+   bson_init (&ts->cluster_time);
+
+   _add_ismaster (&ts->ismaster_cmd, ts->api);
+}
+
+static void
+_reset_ismaster (mongoc_topology_scanner_t *ts)
+{
+   bson_reinit (&ts->ismaster_cmd);
+   bson_reinit (&ts->ismaster_cmd_with_handshake);
+
+   _add_ismaster (&ts->ismaster_cmd, ts->api);
 }
 
 const char *
@@ -226,7 +249,7 @@ _build_ismaster_with_handshake (mongoc_topology_scanner_t *ts)
    int count = 0;
    char buf[16];
 
-   _add_ismaster (doc);
+   _add_ismaster (doc, ts->api);
 
    BSON_APPEND_DOCUMENT_BEGIN (doc, HANDSHAKE_FIELD, &subdoc);
    res = _mongoc_handshake_build_doc_with_application (&subdoc, ts->appname);
@@ -346,21 +369,19 @@ mongoc_topology_scanner_new (
 
    ts->async = mongoc_async_new ();
 
-   bson_init (&ts->ismaster_cmd);
-   _add_ismaster (&ts->ismaster_cmd);
-   bson_init (&ts->ismaster_cmd_with_handshake);
-   bson_init (&ts->cluster_time);
-
    ts->setup_err_cb = setup_err_cb;
    ts->cb = cb;
    ts->cb_data = data;
    ts->uri = uri;
    ts->appname = NULL;
+   ts->api = NULL;
    ts->handshake_ok_to_send = false;
    ts->connect_timeout_msec = connect_timeout_msec;
    /* may be overridden for testing. */
    ts->dns_cache_timeout_ms = DNS_CACHE_TIMEOUT_MS;
 
+   _init_ismaster (ts);
+
    return ts;
 }
 
@@ -398,6 +419,7 @@ mongoc_topology_scanner_destroy (mongoc_topology_scanner_t *ts)
    bson_destroy (&ts->ismaster_cmd);
    bson_destroy (&ts->ismaster_cmd_with_handshake);
    bson_destroy (&ts->cluster_time);
+   mongoc_server_api_destroy (ts->api);
 
    /* This field can be set by a mongoc_client */
    bson_free ((char *) ts->appname);
@@ -1324,3 +1346,15 @@ _jumpstart_other_acmds (mongoc_topology_scanner_node_t *node,
       }
    }
 }
+
+void
+_mongoc_topology_scanner_set_server_api (mongoc_topology_scanner_t *ts,
+                                         const mongoc_server_api_t *api)
+{
+   BSON_ASSERT (ts);
+   BSON_ASSERT (api);
+
+   mongoc_server_api_destroy (ts->api);
+   ts->api = mongoc_server_api_copy (api);
+   _reset_ismaster (ts);
+}