[GitHub] [incubator-opendal] lithammer commented on a diff in pull request #2192: feat(services/sftp): setup integration tests

["lithammer (via GitHub)" <gi...@apache.org>]

lithammer commented on code in PR #2192:
URL: https://github.com/apache/incubator-opendal/pull/2192#discussion_r1221638905


##########
core/src/services/sftp/backend.rs:
##########
@@ -461,58 +442,102 @@ impl Accessor for SftpBackend {
     }
 
     async fn list(&self, path: &str, args: OpList) -> Result<(RpList, Self::Pager)> {
-        let client = self.sftp_connect().await?;
-        let mut fs = client.sftp.fs();
-        fs.set_cwd(self.root.clone());
+        let client = self.connect().await?;
+        let mut fs = client.fs();
+        fs.set_cwd(&self.root);
 
         let file_path = format!("./{}", path);
 
-        let mut dir = match fs.open_dir(file_path.clone()).await {
+        let dir = match fs.open_dir(&file_path).await {
             Ok(dir) => dir,
             Err(e) => {
                 if is_not_found(&e) {
-                    return Ok((RpList::default(), SftpPager::empty()));
+                    return Ok((RpList::default(), None));
                 } else {
                     return Err(e.into());
                 }
             }
-        };
-        let dir = dir.read_dir().await?;
+        }
+        .read_dir();
 
         Ok((
             RpList::default(),
-            SftpPager::new(dir.into_inner(), path.to_owned(), args.limit()),
+            Some(SftpPager::new(dir, path.to_owned(), args.limit())),
         ))
     }
 }
 
 impl SftpBackend {
-    async fn pool(&self) -> Result<&bb8::Pool<Manager>> {
-        let pool = self
-            .sftp
-            .get_or_try_init(|| async {
-                let manager = Manager {
-                    endpoint: self.endpoint.clone(),
-                    user: self.user.clone(),
-                    key: self.key.clone(),
-                };
-
-                bb8::Pool::builder().max_size(10).build(manager).await
+    async fn connect(&self) -> Result<&Sftp> {
+        let sftp = self
+            .client
+            .get_or_try_init(|| {
+                Box::pin(connect_sftp(
+                    self.endpoint.as_str(),
+                    self.root.clone(),
+                    self.user.clone(),
+                    self.key.clone(),
+                    self.known_hosts_strategy.clone(),
+                ))
             })
             .await?;
 
-        Ok(pool)
+        Ok(sftp)
+    }
+}
+
+async fn connect_sftp(
+    endpoint: &str,
+    root: String,
+    user: String,
+    key: Option<String>,
+    known_hosts_strategy: KnownHosts,
+) -> Result<Sftp> {
+    let mut session = SessionBuilder::default();
+
+    session.user(user);
+
+    if let Some(key) = &key {
+        session.keyfile(key);
     }
 
-    pub async fn sftp_connect(&self) -> Result<PooledConnection<'_, Manager>> {
-        let conn = self.pool().await?.get().await?;
+    // set control directory to avoid temp files in root directory when panic
+    if let Some(dir) = dirs::runtime_dir() {
+        session.control_directory(dir);
+    }
 
-        Ok(conn)
+    #[cfg(target_os = "macos")]
+    {
+        let _ = std::fs::create_dir("/private/tmp/.opendal/");
+        session.control_directory("/private/tmp/.opendal/");
     }
 
-    pub async fn sftp_connect_owned(&self) -> Result<PooledConnection<'static, Manager>> {
-        let conn = self.pool().await?.get_owned().await?;
+    session.server_alive_interval(Duration::from_secs(5));
+    session.known_hosts_check(known_hosts_strategy);
 
-        Ok(conn)
+    let session = session.connect(&endpoint).await?;
+
+    let sftp = Sftp::from_session(session, SftpOptions::default()).await?;
+
+    let mut fs = sftp.fs();
+    fs.set_cwd("/");
+
+    let paths = Path::new(&root).components();
+    let mut current = PathBuf::from("/");

Review Comment:
   Why not use the default directory set by the server instead? This would allow you to use relative paths which is a lot more convenient in an SFTP environment instead of trying to navigate to the user's home directory. On some servers you might not even have permission to change directory to `/`.
   
   How about something like this? It would work for both relative and absolute paths.
   
   ```diff
   diff --git a/core/src/services/sftp/backend.rs b/core/src/services/sftp/backend.rs
   index 8bed2bf0..15fefdb9 100644
   --- a/core/src/services/sftp/backend.rs
   +++ b/core/src/services/sftp/backend.rs
   @@ -535,12 +535,11 @@ async fn connect_sftp(
        let sftp = Sftp::from_session(session, SftpOptions::default()).await?;
    
        let mut fs = sftp.fs();
   -    fs.set_cwd("/");
    
        let paths = Path::new(&root).components();
   -    let mut current = PathBuf::from("/");
   +    let mut current = fs.cwd().to_path_buf();
        for p in paths {
   -        current = current.join(p);
   +        current.push(p);
            let res = fs.create_dir(p).await;
    
            if let Err(e) = res {
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org