Tweak the unix_path interface (#407)

We need a small tweak to the unix_path interface so that we can load the associated port from the instance credentials.

UnixPath::with_port_suffix(...)

Author: mmastrac

gel-dsn/Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "gel-dsn"
 license = "MIT/Apache-2.0"
-version = "0.1.1"
+version = "0.2.0"
 authors = ["MagicStack Inc. <[email protected]>"]
 edition = "2021"
 description = "Data-source name (DSN) parser for Gel and PostgreSQL databases."

gel-dsn/src/gel/config.rs

@@ -1,11 +1,12 @@
 use super::{error::*, BuildContextImpl, FromParamStr, InstanceName, Param, Params};
 use crate::{
     gel::parse_duration,
-    host::{Host, HostTarget, HostType},
+    host::{Host, HostType},
 };
 use rustls_pki_types::CertificateDer;
 use serde::{Deserialize, Serialize};
 use std::{
+    borrow::Cow,
     collections::HashMap,
     fmt,
     path::{Path, PathBuf},
@@ -102,7 +103,7 @@ pub struct Config {
 impl Default for Config {
     fn default() -> Self {
         Self {
-            host: Host::new(DEFAULT_HOST.clone(), DEFAULT_PORT, HostTarget::Gel),
+            host: Host::new(DEFAULT_HOST.clone(), DEFAULT_PORT),
             db: DatabaseBranch::Default,
             user: DEFAULT_USER.to_string(),
             instance_name: None,
@@ -194,17 +195,6 @@ impl Config {
         }
     }
 
-    pub fn with_unix_path(&self, path: &Path) -> Self {
-        Self {
-            host: Host::new(
-                HostType::from_unix_path(PathBuf::from(path)),
-                DEFAULT_PORT,
-                HostTarget::Raw,
-            ),
-            ..self.clone()
-        }
-    }
-
     pub fn with_branch(&self, branch: &str) -> Self {
         Self {
             db: DatabaseBranch::Branch(branch.to_string()),
@@ -600,6 +590,68 @@ impl FromStr for TcpKeepalive {
     }
 }
 
+#[derive(derive_more::Debug, Clone, PartialEq, Eq)]
+enum UnixPathInner {
+    /// The selected port will be appended to the path.
+    #[debug("{:?}{{port}}", _0)]
+    PortSuffixed(PathBuf),
+    /// The path will be used as-is.
+    #[debug("{:?}", _0)]
+    Exact(PathBuf),
+}
+
+#[derive(Clone, PartialEq, Eq, derive_more::Debug)]
+pub struct UnixPath {
+    #[debug("{:?}", inner)]
+    inner: UnixPathInner,
+}
+
+impl UnixPath {
+    /// The selected port will be appended to the path.
+    pub fn with_port_suffix(path: PathBuf) -> Self {
+        UnixPath {
+            inner: UnixPathInner::PortSuffixed(path),
+        }
+    }
+
+    /// The path will be used as-is.
+    pub fn exact(path: PathBuf) -> Self {
+        UnixPath {
+            inner: UnixPathInner::Exact(path),
+        }
+    }
+
+    /// Returns a path with the port suffix appended.
+    pub fn path_with_port(&self, port: u16) -> Cow<Path> {
+        match &self.inner {
+            UnixPathInner::PortSuffixed(path) => {
+                let Some(filename) = path.file_name() else {
+                    return Cow::Owned(path.join(port.to_string()));
+                };
+                let mut path = path.clone();
+                let mut filename = filename.to_owned();
+                filename.push(&port.to_string());
+                path.set_file_name(filename);
+                Cow::Owned(path)
+            }
+            UnixPathInner::Exact(path) => Cow::Borrowed(path),
+        }
+    }
+}
+
+impl FromStr for UnixPath {
+    type Err = ParseError;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Ok(UnixPath::exact(PathBuf::from(s)))
+    }
+}
+
+impl<T: Into<PathBuf>> From<T> for UnixPath {
+    fn from(path: T) -> Self {
+        UnixPath::exact(path.into())
+    }
+}
+
 /// Classic-style connection options.
 #[derive(Clone, Default, Debug, Serialize, Deserialize)]
 #[serde(default)]

gel-dsn/src/gel/mod.rs

@@ -357,7 +357,7 @@ mod tests {
     use serde_json::json;
 
     use super::*;
-    use crate::host::{Host, HostTarget, HostType};
+    use crate::host::{Host, HostType};
     use std::{collections::HashMap, time::Duration};
 
     #[test]
@@ -370,11 +370,7 @@ mod tests {
         assert_eq!(
             cfg.unwrap(),
             Config {
-                host: Host::new(
-                    HostType::try_from_str("hostname").unwrap(),
-                    1234,
-                    HostTarget::Gel
-                ),
+                host: Host::new(HostType::try_from_str("hostname").unwrap(), 1234,),
                 ..Default::default()
             }
         );
@@ -398,10 +394,7 @@ mod tests {
             .build()
             .expect("Failed to build credentials");
 
-        assert_eq!(
-            credentials.host,
-            Host::new(DEFAULT_HOST.clone(), 10702, HostTarget::Gel)
-        );
+        assert_eq!(credentials.host, Host::new(DEFAULT_HOST.clone(), 10702));
         assert_eq!(&credentials.user, "test3n");
         assert_eq!(
             credentials.db,
@@ -441,12 +434,21 @@ mod tests {
 
         // Test unix path with a port
         let cfg = Builder::new()
-            .port(8888_u16)
+            .port(8888)
             .unix_path("/test")
             .build()
             .unwrap();
         let host = cfg.host.target_name().unwrap();
-        assert_eq!(host.path(), Some(Path::new("/test/.s.EDGEDB.admin.8888")));
+        assert_eq!(host.path(), Some(Path::new("/test")));
+
+        // Test unix path with a port
+        let cfg = Builder::new()
+            .port(8888)
+            .unix_path(UnixPath::with_port_suffix(PathBuf::from("/prefix.")))
+            .build()
+            .unwrap();
+        let host = cfg.host.target_name().unwrap();
+        assert_eq!(host.path(), Some(Path::new("/prefix.8888")));
     }
 
     /// Test that the hidden CloudCerts env var is parsed correctly.
@@ -455,7 +457,7 @@ mod tests {
         let cloud_cert =
             HashMap::from_iter([("_GEL_CLOUD_CERTS".to_string(), "local".to_string())]);
         let cfg = Builder::new()
-            .port(5656_u16)
+            .port(5656)
             .without_system()
             .with_env_impl(cloud_cert)
             .build()
@@ -466,7 +468,7 @@ mod tests {
     #[test]
     fn test_tcp_keepalive() {
         let cfg = Builder::new()
-            .port(5656_u16)
+            .port(5656)
             .tcp_keepalive(TcpKeepalive::Explicit(Duration::from_secs(10)))
             .without_system()
             .build()

gel-dsn/src/gel/param.rs

@@ -8,7 +8,7 @@ use url::Url;
 
 use super::{
     duration, error::*, BuildContext, ClientSecurity, CloudCerts, CloudCredentialsFile,
-    CredentialsFile, InstanceName, TcpKeepalive, TlsSecurity,
+    CredentialsFile, InstanceName, TcpKeepalive, TlsSecurity, UnixPath,
 };
 use crate::{gel::context_trace, host::HostType, EnvVar, FileAccess};
 
@@ -43,7 +43,8 @@ impl_from_param_str!(
     ClientSecurity,
     CloudCredentialsFile,
     CloudCerts,
-    TcpKeepalive
+    TcpKeepalive,
+    UnixPath
 );
 
 impl FromParamStr for std::time::Duration {

gel-dsn/src/gel/params.rs

@@ -16,14 +16,14 @@ use super::{
     error::*,
     project::{find_project_file, ProjectDir},
     BuildContext, BuildContextImpl, ClientSecurity, CloudCerts, Config, DatabaseBranch,
-    FromParamStr, InstanceName, Logging, Param, ParamSource, TcpKeepalive, TlsSecurity,
+    FromParamStr, InstanceName, Logging, Param, ParamSource, TcpKeepalive, TlsSecurity, UnixPath,
     DEFAULT_CONNECT_TIMEOUT, DEFAULT_PORT, DEFAULT_WAIT,
 };
 use crate::{
     env::SystemEnvVars,
     file::SystemFileAccess,
     gel::{context_trace, Authentication},
-    host::{Host, HostTarget, HostType},
+    host::{Host, HostType},
     user::SystemUserProfile,
     EnvVar, FileAccess, UserProfile,
 };
@@ -233,12 +233,7 @@ define_params!(
     /// The port.
     port: u16,
     /// The unix socket path.
-    ///
-    /// If set, the client will connect to the database via a Unix socket.
-    /// If a port is also set, this must refer to a directory in the filesystem
-    /// containing a Gel admin socket. If no port is set, the client will
-    /// connect to the database via a Unix socket on the exact given path.
-    unix_path: PathBuf,
+    unix_path: UnixPath,
     /// The database name. Used for EdgeDB < 5. For Gel or EdgeDB >= 5, use
     /// [`Builder::branch`].
     database: String,
@@ -788,42 +783,38 @@ impl Params {
 
         let instance = instance?;
         if let Some(instance) = &instance {
-            // Special case: if the unix path is set we ignore the instance
-            // credentials.
-            if explicit.unix_path.is_none() {
-                match &instance {
-                    InstanceName::Local(local) => {
-                        let instance = parse_instance(local, context)?;
-                        context_trace!(context, "Instance: {:?}", instance);
-                        explicit.merge(instance);
-                    }
-                    InstanceName::Cloud { .. } => {
-                        let profile = explicit
-                            .cloud_profile
-                            .get(context)?
-                            .unwrap_or("default".to_string());
-                        let cloud = parse_cloud(&profile, context)?;
-                        context_trace!(context, "Cloud: {:?}", cloud);
-                        explicit.merge(cloud);
-
-                        if let Some(secret_key) = explicit.secret_key.get(context)? {
-                            match instance.cloud_address(&secret_key) {
-                                Ok(Some(address)) => {
-                                    explicit.host = Param::Unparsed(address);
-                                }
-                                Ok(None) => {
-                                    unreachable!();
-                                }
-                                Err(e) => {
-                                    // Special case: we ignore the secret key error until the final phase
-                                    if phase == BuildPhase::Project {
-                                        return Err(e);
-                                    }
+            match &instance {
+                InstanceName::Local(local) => {
+                    let instance = parse_instance(local, context)?;
+                    context_trace!(context, "Instance: {:?}", instance);
+                    explicit.merge(instance);
+                }
+                InstanceName::Cloud { .. } => {
+                    let profile = explicit
+                        .cloud_profile
+                        .get(context)?
+                        .unwrap_or("default".to_string());
+                    let cloud = parse_cloud(&profile, context)?;
+                    context_trace!(context, "Cloud: {:?}", cloud);
+                    explicit.merge(cloud);
+
+                    if let Some(secret_key) = explicit.secret_key.get(context)? {
+                        match instance.cloud_address(&secret_key) {
+                            Ok(Some(address)) => {
+                                explicit.host = Param::Unparsed(address);
+                            }
+                            Ok(None) => {
+                                unreachable!();
+                            }
+                            Err(e) => {
+                                // Special case: we ignore the secret key error until the final phase
+                                if phase == BuildPhase::Project {
+                                    return Err(e);
                                 }
                             }
-                        } else {
-                            return Err(ParseError::SecretKeyNotFound);
                         }
+                    } else {
+                        return Err(ParseError::SecretKeyNotFound);
                     }
                 }
             }
@@ -859,27 +850,15 @@ impl Params {
         }
 
         let host = if let Some(unix_path) = computed.unix_path {
-            match port {
-                Some(port) => Host::new(
-                    HostType::from_unix_path(unix_path),
-                    port,
-                    HostTarget::GelAdmin,
-                ),
-                None => Host::new(
-                    HostType::from_unix_path(unix_path),
-                    DEFAULT_PORT,
-                    HostTarget::Raw,
-                ),
-            }
+            let path = unix_path
+                .path_with_port(port.unwrap_or(DEFAULT_PORT))
+                .into_owned();
+            Host::new(HostType::from_unix_path(path), DEFAULT_PORT)
         } else {
             let host = match (computed.host, port) {
-                (Some(host), Some(port)) => Host::new(host, port, HostTarget::Gel),
-                (Some(host), None) => Host::new(host, DEFAULT_PORT, HostTarget::Gel),
-                (None, Some(port)) => Host::new(
-                    HostType::try_from_str("localhost").unwrap(),
-                    port,
-                    HostTarget::Gel,
-                ),
+                (Some(host), Some(port)) => Host::new(host, port),
+                (Some(host), None) => Host::new(host, DEFAULT_PORT),
+                (None, Some(port)) => Host::new(HostType::try_from_str("localhost").unwrap(), port),
                 (None, None) => {
                     return Ok(None);
                 }
@@ -1524,26 +1503,32 @@ mod tests {
         eprintln!("{:?}", params);
 
         let params = Builder::default()
-            .unix_path(Path::new("/"))
+            .unix_path(UnixPath::with_port_suffix(PathBuf::from(
+                "/.s.EDGEDB.admin.",
+            )))
             .port(1234)
             .without_system()
             .build()
             .expect("Unix path and port is OK");
         assert_eq!(params.host.to_string(), "/.s.EDGEDB.admin.1234");
         eprintln!("{:?}", params);
 
-        // This is allowed, but instance credentials are ignored in this case
-        // and just transferred to the output Config.
+        // Pull the port from the credentials.
         let params = Builder::default()
-            .instance(InstanceName::Local("instancedoesnotexist".to_string()))
-            .unix_path(Path::new("/"))
+            .instance(InstanceName::Local("instancename".to_string()))
+            .unix_path(UnixPath::with_port_suffix(PathBuf::from("/tmp/port.")))
             .without_system()
+            .with_fs_impl(HashMap::from_iter([(
+                PathBuf::from("/home/edgedb/.config/edgedb/credentials/instancename.json"),
+                r#"{ "user": "user", "port": 12345 }"#.to_string(),
+            )]))
+            .with_user_impl("edgedb")
             .build()
             .expect("Unix path and instance is OK");
-        assert_eq!(params.host.to_string(), "/");
+        assert_eq!(params.host.to_string(), "/tmp/port.12345");
         assert_eq!(
             params.instance_name,
-            Some(InstanceName::Local("instancedoesnotexist".to_string()))
+            Some(InstanceName::Local("instancename".to_string()))
         );
         eprintln!("{:?}", params);
     }