Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DatabaseConnection always derive Clone #665

Closed
wants to merge 1 commit into from

Conversation

billy1624
Copy link
Member

PR Info

Adds

  • Docs on DatabaseConnection enum
    • All connections including mock connection can be cloned. However, cloning mock connection is in fact cloning the shared reference, std::sync::Arc, of it. If your test cases involved concurrent operations on mock connection, query results and transaction logs could vary depending on the sequence in which concurrent operations are executed.

Changes

  • DatabaseConnection always derive Clone even with mock feature enabled

@billy1624 billy1624 self-assigned this Apr 6, 2022
@tyt2y3
Copy link
Member

tyt2y3 commented Apr 6, 2022

I am more inclined to keep the semantics of a non cloneable MockConnection. It leads to weird behaviour anyway.

@erwagasore
Copy link

@tyt2y3 For example when creating a Tide application with state we have to present a clone-able State which include DbConn and when mock feature is enabled we always get an error. Here is a simple example below as a demonstration.

# Cargo.toml
[dependencies.sea-orm]
version = "0.7.0"
default-features = false
features = [
  "sqlx-postgres",
  "runtime-async-std-rustls",
  "macros",
  "mock",
]
// /src/main.rs
//...
#[derive(Clone)]
pub struct State {
    // the DbConn shows the error below when `mock` is present into `Cargo.toml`:
    // the trait bound `DatabaseConnection: Clone` is not satisfied
    db: DbConn,
}

#[async_std::main]
async fn main() -> tide::Result<()> {
    tide::log::start();
    let mut opts = ConnectOptions::new(configs.database.get_uri());
    opts.min_connections(5).max_connections(100);
    let db = Database::connect(opts).await.expect("Failed to connect to the database");
    let state = State { db };
    let mut app = tide::with_state(state);
    let address = format!("127.0.0.1:{}", configs.app_port);
    app.listen(address).await?;
    Ok(())
}
//...

This behaviour of attaching Clone-able state is expected by Tide and I saw same behavior with Actix. All examples we have in this repo avoids the element of test which is critical to have for any production application.

It would be great to have a path where we are able to do some testing without the Clone issue. A big percentage of issues raised here are related to this.

I am new to rust thus don't have much experience but this seems to be a bottleneck for adoption of this great library.

@tyt2y3
Copy link
Member

tyt2y3 commented Apr 9, 2022

I think the canonical way is to enable mock only as dev dependency. Enabling it in production is not recommended. So I think the current behaviour is what it's intended. You can reference https://stackoverflow.com/questions/27872009/how-do-i-use-a-feature-of-a-dependency-only-for-testing on how to do so.

I would imagine a framework that has it all setup in the intended way (unit test with mocking) & (integration test with SQLite or a real DB).

The sea-orm crate itself is such a setup.

@tyt2y3 tyt2y3 closed this Apr 9, 2022
@erwagasore
Copy link

I think the canonical way is to enable mock only as dev dependency. Enabling it in production is not recommended. So I think the current behaviour is what it's intended. You can reference https://stackoverflow.com/questions/27872009/how-do-i-use-a-feature-of-a-dependency-only-for-testing on how to do so.

I would imagine a framework that has it all setup in the intended way (unit test with mocking) & (integration test with SQLite or a real DB).

The sea-orm crate itself is such a setup.

Unfortunately, the suggested solution above doesn't solve the issue. https://discord.com/channels/873880840487206962/900758302294704188/961221662064410634

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants