please review r2d2-oracle library

[rursprung]

Hi

i have just published v0.1.0 of the r2d2-oracle library: https://github.com/rursprung/r2d2-oracle
could you please have a look at it and check if it looks good to you?

Thanks!

[kubo]

Thanks for letting me know.

In the file https://github.com/rursprung/r2d2-oracle/blob/master/src/lib.rs:

    /// let mut connector = oracle::Connector::new("system", "manager", "");
    /// let connector = connector.privilege(oracle::Privilege::Sysdba);

Could you remove let connector =?

    /// let manager = OracleConnectionManager::new_with_connector(connector.clone());

Could you remove .clone()? It is required only when the connector is referred later.

    pub fn new_with_connector(connector: oracle::Connector) -> OracleConnectionManager {

IMO, rust doesn't tend to use methods starting with new_xxx. For example the method name to create a new Vec with capacity is Vec::with_capacity, not Vec::new_with_capacity. In addition, the standard library has no methods starting with new_with except two deprecated methods.
(Open this page and put new_with in the search box at the top of the page.)
I think from_connector is preferable though what do you choose is up to you.

    fn is_valid(&self, conn: &mut oracle::Connection) -> Result<(), oracle::Error> {
        conn.query("SELECT 1 FROM dual", &[]).map(|_| ())
    }

Could you use conn.ping() instead? It checks the network connection between the client and the server without executing a SQL statement.

    fn has_broken(&self, conn: &mut oracle::Connection) -> bool {
        self.is_valid(conn).is_err()
    }

According to this document, has_broken should not block. However it makes a network round trip so it may block when the network traffic is in trouble. Could you change it simply false as the document say?

I may add a similar method to return the connection status by using OCI_ATTR_SERVER_STATUS after I check what "doing a light weight connection health check" means.

[rursprung]

thanks a lot for your feedback!

this doesn't work:

    let manager = OracleConnectionManager::new_with_connector(oracle::Connector::new("system", "manager", "").privilege(oracle::Privilege::Sysdba));

---- src\lib.rs - OracleConnectionManager::new_with_connector (line 56) stdout ----
error[E0308]: mismatched types
 --> src\lib.rs:59:59
  |
6 | let manager = OracleConnectionManager::new_with_connector(oracle::Connector::new("system", "manager", "").privilege(oracle::Privilege::Sysdba));
  |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `r2d2_oracle::oracle::Connector`, found mutable reference
  |
  = note: expected type `r2d2_oracle::oracle::Connector`
             found type `&mut r2d2_oracle::oracle::Connector`

and this doesn't work either:

    let connector = oracle::Connector::new("system", "manager", "")
        .privilege(oracle::Privilege::Sysdba);
    let manager = OracleConnectionManager::new_with_connector(connector.clone());

---- src\lib.rs - OracleConnectionManager::new_with_connector (line 56) stdout ----
error[E0716]: temporary value dropped while borrowed
 --> src\lib.rs:59:17
  |
6 | let connector = oracle::Connector::new("system", "manager", "")
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary which is freed while still in use
7 |     .privilege(oracle::Privilege::Sysdba);
  |                                          - temporary value is freed at the end of this statement
8 | let manager = OracleConnectionManager::new_with_connector(connector.clone());
  |                                                           --------- borrow later used here
  |
  = note: consider using a `let` binding to create a longer lived value

i think this might actually be a problem of your API design of Connector: you return &mut Connector in e.g. privilege. what i have seen elsewhere was that it would return Connector instead (the instance, not a reference to it). so it consumes itself and then forwards it again. i think this would be more idiomatic rust.

thanks for the naming suggestion for from_connector, i'll take that over.

and thanks for pointing out Connection::ping, i'll switch to that. wasn't aware of it (guess i should've paid a closer look at the docs...)

same goes for has_broken: i copied this from r2d2-mysql: https://github.com/outersky/r2d2-mysql/blob/master/src/pool.rs
i'll wait with changing has_broken until i hear back from you about OCI_ATTR_SERVER_STATUS

[kubo]

i think this might actually be a problem of your API design of Connector: you return &mut Connector in e.g. privilege. what i have seen elsewhere was that it would return Connector instead (the instance, not a reference to it). so it consumes itself and then forwards it again. i think this would be more idiomatic rust.

std::process::Command and std::fs::DirBuilder return &mut Self. They are non-consuming builders described in the Rust API Guidelines.

I understand the guideline about builders as:

• Use non-consuming builders if the terminal methods take &self
• Use consuming builders if the terminal methods take self

I chose non-consuming builders because Connector.connect takes &self.

[rursprung]

thanks for your review!
rursprung/r2d2-oracle#2 has been merged and will soon be released as v0.2.0