RUST-1765 Make slash between hosts and options in the URI optional (#1314)

Author: isabelatkinson

src/client/options.rs

@@ -1317,6 +1317,34 @@ impl ClientOptions {
     }
 }
 
+/// Splits the string once on the first instance of the given delimiter. If the delimiter is not
+/// present, returns the entire string as the "left" side.
+///
+/// e.g.
+/// "abc.def" split on "." -> ("abc", Some("def"))
+/// "ab.cd.ef" split on "." -> ("ab", Some("cd.ef"))
+/// "abcdef" split on "." -> ("abcdef", None)
+fn split_once_left<'a>(s: &'a str, delimiter: &str) -> (&'a str, Option<&'a str>) {
+    match s.split_once(delimiter) {
+        Some((l, r)) => (l, Some(r)),
+        None => (s, None),
+    }
+}
+
+/// Splits the string once on the last instance of the given delimiter. If the delimiter is not
+/// present, returns the entire string as the "right" side.
+///
+/// e.g.
+/// "abd.def" split on "." -> (Some("abc"), "def")
+/// "ab.cd.ef" split on "." -> (Some("ab.cd"), "ef")
+/// "abcdef" split on "." -> (None, "abcdef")
+fn split_once_right<'a>(s: &'a str, delimiter: &str) -> (Option<&'a str>, &'a str) {
+    match s.rsplit_once(delimiter) {
+        Some((l, r)) => (Some(l), r),
+        None => (None, s),
+    }
+}
+
 /// Splits a string into a section before a given index and a section exclusively after the index.
 /// Empty portions are returned as `None`.
 fn exclusive_split_at(s: &str, i: usize) -> (Option<&str>, Option<&str>) {
@@ -1338,12 +1366,12 @@ fn percent_decode(s: &str, err_message: &str) -> Result<String> {
     }
 }
 
-fn validate_userinfo(s: &str, userinfo_type: &str) -> Result<()> {
+fn validate_and_parse_userinfo(s: &str, userinfo_type: &str) -> Result<String> {
     if s.chars().any(|c| USERINFO_RESERVED_CHARACTERS.contains(&c)) {
-        return Err(ErrorKind::InvalidArgument {
-            message: format!("{} must be URL encoded", userinfo_type),
-        }
-        .into());
+        return Err(Error::invalid_argument(format!(
+            "{} must be URL encoded",
+            userinfo_type
+        )));
     }
 
     // All instances of '%' in the username must be part of an percent-encoded substring. This means
@@ -1352,13 +1380,13 @@ fn validate_userinfo(s: &str, userinfo_type: &str) -> Result<()> {
         .skip(1)
         .any(|part| part.len() < 2 || part[0..2].chars().any(|c| !c.is_ascii_hexdigit()))
     {
-        return Err(ErrorKind::InvalidArgument {
-            message: "username/password cannot contain unescaped %".to_string(),
-        }
-        .into());
+        return Err(Error::invalid_argument(format!(
+            "{} cannot contain unescaped %",
+            userinfo_type
+        )));
     }
 
-    Ok(())
+    percent_decode(s, &format!("{} must be URL encoded", userinfo_type))
 }
 
 impl TryFrom<&str> for ConnectionString {
@@ -1390,116 +1418,60 @@ impl ConnectionString {
     /// malformed or one of the options has an invalid value, an error will be returned.
     pub fn parse(s: impl AsRef<str>) -> Result<Self> {
         let s = s.as_ref();
-        let end_of_scheme = match s.find("://") {
-            Some(index) => index,
-            None => {
-                return Err(ErrorKind::InvalidArgument {
-                    message: "connection string contains no scheme".to_string(),
-                }
-                .into())
-            }
+
+        let Some((scheme, after_scheme)) = s.split_once("://") else {
+            return Err(Error::invalid_argument(
+                "connection string contains no scheme",
+            ));
         };
 
-        let srv = match &s[..end_of_scheme] {
+        let srv = match scheme {
             "mongodb" => false,
+            #[cfg(feature = "dns-resolver")]
             "mongodb+srv" => true,
-            _ => {
-                return Err(ErrorKind::InvalidArgument {
-                    message: format!("invalid connection string scheme: {}", &s[..end_of_scheme]),
-                }
-                .into())
+            #[cfg(not(feature = "dns-resolver"))]
+            "mongodb+srv" => {
+                return Err(Error::invalid_argument(
+                    "mongodb+srv connection strings cannot be used when the 'dns-resolver' \
+                     feature is disabled",
+                ))
             }
-        };
-        #[cfg(not(feature = "dns-resolver"))]
-        if srv {
-            return Err(Error::invalid_argument(
-                "mongodb+srv connection strings cannot be used when the 'dns-resolver' feature is \
-                 disabled",
-            ));
-        }
-
-        let after_scheme = &s[end_of_scheme + 3..];
-
-        let (pre_slash, post_slash) = match after_scheme.find('/') {
-            Some(slash_index) => match exclusive_split_at(after_scheme, slash_index) {
-                (Some(section), o) => (section, o),
-                (None, _) => {
-                    return Err(ErrorKind::InvalidArgument {
-                        message: "missing hosts".to_string(),
-                    }
-                    .into())
-                }
-            },
-            None => {
-                if after_scheme.find('?').is_some() {
-                    return Err(ErrorKind::InvalidArgument {
-                        message: "Missing delimiting slash between hosts and options".to_string(),
-                    }
-                    .into());
-                }
-                (after_scheme, None)
+            other => {
+                return Err(Error::invalid_argument(format!(
+                    "unsupported connection string scheme: {}",
+                    other
+                )))
             }
         };
 
-        let (database, options_section) = match post_slash {
-            Some(section) => match section.find('?') {
-                Some(index) => exclusive_split_at(section, index),
-                None => (post_slash, None),
-            },
-            None => (None, None),
-        };
-
-        let db = match database {
-            Some(db) => {
-                let decoded = percent_decode(db, "database name must be URL encoded")?;
-                if decoded
-                    .chars()
-                    .any(|c| ILLEGAL_DATABASE_CHARACTERS.contains(&c))
-                {
-                    return Err(ErrorKind::InvalidArgument {
-                        message: "illegal character in database name".to_string(),
-                    }
-                    .into());
-                }
-                Some(decoded)
-            }
-            None => None,
-        };
+        let (pre_options, options) = split_once_left(after_scheme, "?");
+        let (user_info, hosts_and_auth_db) = split_once_right(pre_options, "@");
 
-        let (authentication_requested, cred_section, hosts_section) = match pre_slash.rfind('@') {
-            Some(index) => {
-                // if '@' is in the host section, it MUST be interpreted as a request for
-                // authentication, even if the credentials are empty.
-                let (creds, hosts) = exclusive_split_at(pre_slash, index);
-                match hosts {
-                    Some(hs) => (true, creds, hs),
-                    None => {
-                        return Err(ErrorKind::InvalidArgument {
-                            message: "missing hosts".to_string(),
-                        }
-                        .into())
-                    }
-                }
+        // if '@' is in the host section, it MUST be interpreted as a request for authentication
+        let authentication_requested = user_info.is_some();
+        let (username, password) = match user_info {
+            Some(user_info) => {
+                let (username, password) = split_once_left(user_info, ":");
+                let username = if username.is_empty() {
+                    None
+                } else {
+                    Some(validate_and_parse_userinfo(username, "username")?)
+                };
+                let password = match password {
+                    Some(password) => Some(validate_and_parse_userinfo(password, "password")?),
+                    None => None,
+                };
+                (username, password)
             }
-            None => (false, None, pre_slash),
-        };
-
-        let (username, password) = match cred_section {
-            Some(creds) => match creds.find(':') {
-                Some(index) => match exclusive_split_at(creds, index) {
-                    (username, None) => (username, Some("")),
-                    (username, password) => (username, password),
-                },
-                None => (Some(creds), None), // Lack of ":" implies whole string is username
-            },
             None => (None, None),
         };
 
-        let hosts = hosts_section
-            .split(',')
+        let (hosts, auth_db) = split_once_left(hosts_and_auth_db, "/");
+
+        let hosts = hosts
+            .split(",")
             .map(ServerAddress::parse)
             .collect::<Result<Vec<ServerAddress>>>()?;
-
         let host_info = if !srv {
             HostInfo::HostIdentifiers(hosts)
         } else {
@@ -1527,17 +1499,32 @@ impl ConnectionString {
             }
         };
 
+        let db = match auth_db {
+            Some("") | None => None,
+            Some(db) => {
+                let decoded = percent_decode(db, "database name must be URL encoded")?;
+                for c in decoded.chars() {
+                    if ILLEGAL_DATABASE_CHARACTERS.contains(&c) {
+                        return Err(Error::invalid_argument(format!(
+                            "illegal character in database name: {}",
+                            c
+                        )));
+                    }
+                }
+                Some(decoded)
+            }
+        };
+
         let mut conn_str = ConnectionString {
             host_info,
             #[cfg(test)]
             original_uri: s.into(),
             ..Default::default()
         };
 
-        let mut parts = if let Some(opts) = options_section {
-            conn_str.parse_options(opts)?
-        } else {
-            ConnectionStringParts::default()
+        let mut parts = match options {
+            Some(options) => conn_str.parse_options(options)?,
+            None => ConnectionStringParts::default(),
         };
 
         if conn_str.srv_service_name.is_some() && !srv {
@@ -1566,19 +1553,10 @@ impl ConnectionString {
             }
         }
 
-        // Set username and password.
-        if let Some(u) = username {
+        if let Some(username) = username {
             let credential = conn_str.credential.get_or_insert_with(Default::default);
-            validate_userinfo(u, "username")?;
-            let decoded_u = percent_decode(u, "username must be URL encoded")?;
-
-            credential.username = Some(decoded_u);
-
-            if let Some(pass) = password {
-                validate_userinfo(pass, "password")?;
-                let decoded_p = percent_decode(pass, "password must be URL encoded")?;
-                credential.password = Some(decoded_p)
-            }
+            credential.username = Some(username);
+            credential.password = password;
         }
 
         if parts.auth_source.as_deref() == Some("") {

src/client/options/test.rs

@@ -22,6 +22,9 @@ static SKIPPED_TESTS: Lazy<Vec<&'static str>> = Lazy::new(|| {
         "maxPoolSize=0 does not error",
         #[cfg(not(feature = "cert-key-password"))]
         "Valid tlsCertificateKeyFilePassword is parsed correctly",
+        // TODO RUST-1954: unskip these tests
+        "Colon in a key value pair",
+        "Comma in a key value pair causes a warning",
     ];
 
     // TODO RUST-1896: unskip this test when openssl-tls is enabled
@@ -72,11 +75,26 @@ struct TestAuth {
 }
 
 impl TestAuth {
-    fn matches_client_options(&self, options: &ClientOptions) -> bool {
+    fn assert_matches_client_options(&self, options: &ClientOptions, description: &str) {
         let credential = options.credential.as_ref();
-        self.username.as_ref() == credential.and_then(|cred| cred.username.as_ref())
-            && self.password.as_ref() == credential.and_then(|cred| cred.password.as_ref())
-            && self.db.as_ref() == options.default_database.as_ref()
+        assert_eq!(
+            self.username.as_ref(),
+            credential.and_then(|c| c.username.as_ref()),
+            "{}",
+            description
+        );
+        assert_eq!(
+            self.password.as_ref(),
+            credential.and_then(|c| c.password.as_ref()),
+            "{}",
+            description
+        );
+        assert_eq!(
+            self.db.as_ref(),
+            options.default_database.as_ref(),
+            "{}",
+            description
+        );
     }
 }
 
@@ -177,7 +195,8 @@ async fn run_tests(path: &[&str], skipped_files: &[&str]) {
                 }
 
                 if let Some(test_auth) = test_case.auth {
-                    assert!(test_auth.matches_client_options(&client_options));
+                    test_auth
+                        .assert_matches_client_options(&client_options, &test_case.description);
                 }
             } else {
                 let error = client_options_result.expect_err(&test_case.description);

src/test/spec/json/connection-string/README.md

@@ -0,0 +1,55 @@
+# Connection String Tests
+
+The YAML and JSON files in this directory tree are platform-independent tests that drivers can use to prove their
+conformance to the Connection String Spec.
+
+As the spec is primarily concerned with parsing the parts of a URI, these tests do not focus on host and option
+validation. Where necessary, the tests use options known to be (un)supported by drivers to assert behavior such as
+issuing a warning on repeated option keys. As such these YAML tests are in no way a replacement for more thorough
+testing. However, they can provide an initial verification of your implementation.
+
+## Version
+
+Files in the "specifications" repository have no version scheme. They are not tied to a MongoDB server version.
+
+## Format
+
+Each YAML file contains an object with a single `tests` key. This key is an array of test case objects, each of which
+have the following keys:
+
+- `description`: A string describing the test.
+- `uri`: A string containing the URI to be parsed.
+- `valid:` A boolean indicating if the URI should be considered valid.
+- `warning:` A boolean indicating whether URI parsing should emit a warning (independent of whether or not the URI is
+    valid).
+- `hosts`: An array of host objects, each of which have the following keys:
+    - `type`: A string denoting the type of host. Possible values are "ipv4", "ip_literal", "hostname", and "unix".
+        Asserting the type is *optional*.
+    - `host`: A string containing the parsed host.
+    - `port`: An integer containing the parsed port number.
+- `auth`: An object containing the following keys:
+    - `username`: A string containing the parsed username. For auth mechanisms that do not utilize a password, this may be
+        the entire `userinfo` token (as discussed in [RFC 2396](https://www.ietf.org/rfc/rfc2396.txt)).
+    - `password`: A string containing the parsed password.
+    - `db`: A string containing the parsed authentication database. For legacy implementations that support namespaces
+        (databases and collections) this may be the full namespace eg: `<db>.<coll>`
+- `options`: An object containing key/value pairs for each parsed query string option.
+
+If a test case includes a null value for one of these keys (e.g. `auth: ~`, `port: ~`), no assertion is necessary. This
+both simplifies parsing of the test files (keys should always exist) and allows flexibility for drivers that might
+substitute default values *during* parsing (e.g. omitted `port` could be parsed as 27017).
+
+The `valid` and `warning` fields are boolean in order to keep the tests flexible. We are not concerned with asserting
+the format of specific error or warnings messages strings.
+
+### Use as unit tests
+
+Testing whether a URI is valid or not should simply be a matter of checking whether URI parsing (or MongoClient
+construction) raises an error or exception. Testing for emitted warnings may require more legwork (e.g. configuring a
+log handler and watching for output).
+
+Not all drivers may be able to directly assert the hosts, auth credentials, and options. Doing so may require exposing
+the driver's URI parsing component.
+
+The file `valid-db-with-dotted-name.yml` is a special case for testing drivers that allow dotted namespaces, instead of
+only database names, in the Auth Database portion of the URI.