More elegant way to call "generic" methods on a struct?

help
1

I have an FTP client using the suppaftp crate. The client needs to support both plain FTP, and FTPS (over TLS), depending on the protocol specified in a user-supplied URL string.

For the most part things work well, but the challenge is that the type of struct (from the suppaftp crate) that's used for the connection is different based on whether TLS is used. In the no-TLS case it's FtpStream = ImplFtpStream<NoTlsStream> and in the TLS case it's NativeTlsFtpStream = ImplFtpStream<NativeTlsStream>.

Both types of stream essentially support the same methods (e.g. .login(), .quit(), .nlst() to get a directory listing, .retr_as_buffer() to download a file)...but since they're different types, it doesn't seem that I'm able to set up a "generic" FTP stream, and execute these methods on it regardless of whether the stream uses TLS. The crate doesn't appear to expose a common trait that enables this.

So the best I've been able to come up with is the below implementation of a "wrapper struct" that aims to abstract away the TLS vs no-TLS cases. This obviously has a high level of code duplication, as I need separate method calls that handles each of the cases. Is there any way to improve on this? Is this a shortcoming of the crate that I'm using? (it seems to be the only actively maintained FTP client).. Or maybe I just need to live with it?

use std::io::Cursor;

use native_tls::TlsConnector;
use suppaftp::{FtpError, FtpStream, NativeTlsConnector, NativeTlsFtpStream};
use thiserror::Error;

#[derive(Error, Debug)]
pub enum FtpConnError {
    #[error(transparent)]
    FtpError(#[from] FtpError),
    #[error("path {0} is not valid")]
    PathError(String),
    #[error("not connected")]
    NotConnected,
    #[error(transparent)]
    UrlParse(#[from] url::ParseError),
}

pub enum Stream {
    Plain(FtpStream),
    NativeTls(NativeTlsFtpStream),
}

pub struct FtpConnection {
    host: String,
    port: u16,
    user: String,
    password: String,
    secure: bool,
    base_path: String,
    ftp_stream: Option<Stream>,
}

impl FtpConnection {
    pub fn new(url: &str) -> Self {
        let url = url::Url::parse(url).unwrap();
        let host = url.host_str().unwrap().to_string();
        let port = url.port().unwrap_or(21);
        let user = url.username().to_string();
        let password = url.password().unwrap_or("").to_string();
        let secure = url.scheme() == "ftps";
        let base_path = url
            .path()
            .strip_prefix('/')
            .unwrap_or_else(|| url.path())
            .to_owned();
        FtpConnection {
            host,
            port,
            user,
            password,
            secure,
            base_path,
            ftp_stream: None,
        }
    }

    pub fn connect(&mut self) -> Result<(), FtpConnError> {
        let addr = &format!("{}:{}", self.host, self.port);
        match self.secure {
            false => {
                let mut ftp_stream = FtpStream::connect(addr)?;
                ftp_stream.login(&self.user, &self.password)?;
                ftp_stream.set_passive_nat_workaround(true);
                ftp_stream.cwd(&self.base_path)?;
                self.ftp_stream = Some(Stream::Plain(ftp_stream));
        
            }
            true => {
                let mut ftp_stream = NativeTlsFtpStream::connect(addr)?.into_secure(
                    NativeTlsConnector::from(TlsConnector::new().unwrap()),
                    &self.host,
                )?;
                ftp_stream.login(&self.user, &self.password)?;
                ftp_stream.set_passive_nat_workaround(true);
                ftp_stream.cwd(&self.base_path)?;
                self.ftp_stream = Some(Stream::NativeTls(ftp_stream));
            }
        }
        Ok(())
    }

    pub fn disconnect(&mut self) -> Result<(), FtpConnError> {
        match &mut self.ftp_stream {
            Some(Stream::Plain(ftp_stream)) => ftp_stream.quit().map_err(Into::into),
            Some(Stream::NativeTls(ftp_stream)) => ftp_stream.quit().map_err(Into::into),
            None => Ok(()),
        }
    }

    pub fn list_files(&mut self) -> Result<Vec<String>, FtpConnError> {
        match &mut self.ftp_stream {
            Some(Stream::Plain(ftp_stream)) => ftp_stream.nlst(None).map_err(Into::into),
            Some(Stream::NativeTls(ftp_stream)) => ftp_stream.nlst(None).map_err(Into::into),
            None => Err(FtpConnError::NotConnected),
        }
    }

    pub fn download_file(&mut self, filename: &str) -> Result<Cursor<Vec<u8>>, FtpConnError> {
        match &mut self.ftp_stream {
            Some(Stream::Plain(ftp_stream)) => ftp_stream.retr_as_buffer(filename).map_err(Into::into),
            Some(Stream::NativeTls(ftp_stream)) => ftp_stream.retr_as_buffer(filename).map_err(Into::into),
            None => Err(FtpConnError::NotConnected),
        }
    }
}
2

There are at least two ways to do this better:

  1. Propagate the generic. Make your FtpConnection generic over the stream type just like the underlying type of the ftp_stream field (which should no longer be an enum but a generic type parameter).
  2. Create an object-safe trait and implement it for both supported stream types, then use type erasure (e.g. Box<dyn FtpStream>, name up to debate) to get a concrete dynamic type out of the underlying static types.
2 Likes
3

Thanks for the pointers @H2CO3 ! I actually tried some variations of both of these but couldn't quite get them to work for me (undoubtedly as a result of my own ignorance).

For option 1, would that involve having FtpConnection<Stream>? I guess not - more likely FtpConnection<T> where T is either FtpStream or NativeTlsFtpStream? (or just a true/false const bool that determines whether TLS is enabled?). I tried some of these, but specifically struggled with instantiating FtpConnection::new() from a URL string; since Rust insisted that I need to specify the generic in that instantiation statement. Yet the code calling FtpConnection::new() shouldn't care what stream type is being used.

For option 2, it seemed that Box<dyn Xyz> only works where Xyz is a trait rather than a struct. In this case FtpStream is a struct. But my understanding is probably lacking here. Looks like I should wrap my head around the discussion in What's the best approach to dynamic types chosen at runtime? Box, or enum, or something else?.

Either way, if you're able to point me to any bits of code that use the above approaches in practice that would be hugely appreciated!

4

Yes.

If you are going with the generic approach, the calling code will need to care.

Yes.

That's why I suggested to create your own trait.

5

Thanks for the clarifications!

Right. That's not a deal-breaker - I can always have a wrapper that parses the URL and sets up the relevant type of FtpConnection<T>. Though I'm still a bit hazy on how FtpConnection<T> could be implemented, without separately writing out the implementations of FtpConnection<FtpStream> and FtpConnection<NativeTlsFtpStream> (possibly under a trait). I say that in the context of not having a common trait of the two types that's exposed by the crate...so I don't even see how I would "tell" Rust that T could be those two things. Anyway...

Yes, apologies, I missed the start of that sentence! (and assumed that FtpStream in your example is the struct exported by the crate). I can see how this would be a neat solution.

That said, I assume the net result wouldn't necessarily be a reduction in code duplication? I.e. my trait definition would still need to spell out that downloading a file involves calling .retr_as_buffer() on one type of stream, and also on the other. I.e.

pub trait AnyFtpConnection {
    fn retr_as_buffer(&mut self, path: &str) -> Result<Cursor<Vec<u8>>, FtpError>;
    // And so forth for the other methods I want to use
}

impl AnyFtpConnection for suppaftp::FtpStream {
    fn retr_as_buffer(&mut self, path: &str) -> Result<Cursor<Vec<u8>>, FtpError> {
        self.retr_as_buffer(path)
    }
    // ...
}

impl AnyFtpConnection for suppaftp::NativeTlsFtpStream {
    fn retr_as_buffer(&mut self, path: &str) -> Result<Cursor<Vec<u8>>, FtpError> {
        self.retr_as_buffer(path)
    }
    // ...
}

So on balance I assume the only real shortcut here would involve a macro? Something like the suggestions in rust - Implementing a trait for multiple types at once - Stack Overflow?

Thanks again for helping explore the possibilities here!

6

There's no way around writing code that doesn't yet exist. Trait implementations have to specify what their methods are doing.

The point is not to write less code in itself, anyway. The point is to organize code in a way that is easier to understand, change, optimize, and fix (or write correctly in the first place).

Indeed, you can easily create a macro for writing repetitive trait implementations. The dynamic dispatch approach is better in this case IMO precisely because the increased degree of decoupling allows a macro-based implementation that is a lot less complex than an equivalent macro for the enum-based implementation would be. See e.g. Rust Explorer.

1 Like
7

Amazing, thank you for the patient explanations (and the code example) @H2CO3 !

8

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.