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

No mechanism for generic Connection instances #2384

Closed
chpatton013 opened this issue May 6, 2020 · 3 comments
Closed

No mechanism for generic Connection instances #2384

chpatton013 opened this issue May 6, 2020 · 3 comments

Comments

@chpatton013
Copy link

chpatton013 commented May 6, 2020

I might be misunderstanding something, but as far as I can tell I've found an unnecessary limitation in the Connection API design. This limitation prevents Connection from being used in a generic way, forcing the backend type to be known statically.

Including Backend and TransactionManager in Connection statically (as types) means that you cannot create a generic Connection (such as a BoxedConnection, or something similar) that abstracts the specific implementation at runtime. If these properties were structs that could be accessed through Connection trait functions, this limitation would not be present.

I would like to be able to do something like this (where I instantiate the pool from a runtime host url):

use diesel::r2d2::{ConnectionManager, Pool};
use diesel::prelude::*;

enum DatabasePool {
    Mysql(Pool<ConnectionManager<MysqlConnection>>),
    Pg(Pool<ConnectionManager<PgConnection>>),
    Sqlite(Pool<ConnectionManager<SqliteConnection>>),
}

impl DatabasePool {
    fn connection(&self) -> Box<dyn diesel::Connection> {
        match self {
            DatabasePool::Mysql(pool) => Box::new(pool.get()),
            DatabasePool::Pg(pool) => Box::new(pool.get()),
            DatabasePool::Sqlite(pool) => Box::new(pool.get()),
        }
    }
}

However, due to the Backend and TransactionManager types I get the expected error:

error[E0191]: the value of the associated types `Backend` (from trait `diesel::Connection`), `TransactionManager` (from trait `diesel::Connection`) must be specified

So first, I'd like to know if I've missed something fundamental in the API that would resolve this problem for me. High probability; it's a large API.

If not, then I'd like to know if this is intended or just an implementation detail. And what is the likelihood of changing that implementation detail?

@weiznich
Copy link
Member

weiznich commented May 6, 2020

Short answer

Diesel is designed that way, such that dyn diesel::Connection is not possible, because we want to have something that allows use to use as much database specific sql extension as we require. From this point of view I will close this issue, as everything works as designed/expected from our point of view.

Possible workarounds for client applications

  • Make the database a compile time choice: Instead of letting the user decide on runtime which backend should be used, just conditional compile for one or the other backend. Basically you would have a #[cfg(feature = "postgres")] pub type Connection = PgConnection; somewhere in your crate and usecrate::Connection` after that in place of a concrete connection type
  • Write code that is generic over a specific connection type: This requires to write every function accepting a connection in a generic way, such that it accepts C: Connection (and a lot of other trait bounds that depend on the actual executed query). (I wouldn't recommend anyone to choose this route as it involves quite heavy where clauses. So if you can choose the first solution use that one, otherwise: Just follow the compiler error messages, they are right in most cases. So good luck with the compiler 😉)

Explanation why things are as they are

Diesel tries to check and build most of the generated SQL at compile time using the type system. That implies that we need to know certain things, like the used backend, at compile time. Additionally those type information allowing us to provide certain functionality based in such a way that a specific backend is required as only those backend supports the underlying sql construct. Additionally we use all those type information to do some clever prepared connection statement caching for completely static queries, in such a way that we won't even need to build the SQL again if you executed the query once for a given connection. Those information are just not available if someone tries to use something like dyn Connection, as they would be intentionally removed by the compiler. In the end it's a trade of between different goals: Would we like to have something that tries to abstract away the differences between different databases or do we want something that let us fully utilize the complete power of a specific database implementation. For us the answer was: We want the second thing, but that does not necessarily mean that this answer will be the same for all people. So if your reasoning differs here you can try one of the workarounds presented above or look for another solution beside of diesel.

That all written: It does not mean that at least I personally would be completely opposed to support some kind of generic connection type with diesel. Likely I would like to see this as an extension crate, at least for an initial prototype version. So if someone is interested in working on that feel free to open a topic in our forum outlining at least a rough plan how such an API would look like and how this could possibly be implemented with the current infrastructure.

@weiznich weiznich closed this as completed May 6, 2020
@chpatton013
Copy link
Author

Thanks for the thorough explanation! Your reasoning makes complete sense. I didn't realize that so much functionality hinged on that static type information.

And thank you for the build-time config suggestion. That's a great option to get me moving forward again.

I don't have the breadth of knowledge of diesel necessary to back this opinion up, but my intuition is that there's a path forward that could accomplish both objectives. Maybe if there was a generic connection enum that could be accepted by the various execute functions that would match and defer to the underlying specific calls.
So instead of only:

query.execute(&cxn);

where cxn is a specific implemetation, you could use the following:

enum GenericConnection {
    Mysql(...),
    Pg(...),
    Sqlite(...),
}
// ...
fn execute_generic(&self, generic_cxn: &GenericConnection) {
    match generic_cxn {
        GenericConnection::Mysql(cxn) => self.execute(cxn),
        GenericConnection::Pg(cxn) => self.execute(cxn),
        GenericConnection::Sqlite(cxn) => self.execute(cxn),
    }
}
// ...
query.execute_generic(&generic_cxn);

There may be other hurdles I don't know about yet, but I think this is something I could do all on my application's side without built-in support within diesel (obviously the query objects wouldn't have the function I spec'd out, but I could do that with a free-function or macro of my own). And at least for my limited use-case this would be sufficient.

I'm interested in hearing your opinion of this idea. Is this a fool's errand? Or is there some potential with this path?

@weiznich
Copy link
Member

weiznich commented May 7, 2020

There may be other hurdles I don't know about yet, but I think this is something I could do all on my application's side without built-in support within diesel (obviously the query objects wouldn't have the function I spec'd out, but I could do that with a free-function or macro of my own). And at least for my limited use-case this would be sufficient.

In theory something like this can work, but it requires a lot of work to make this working for the function case as you would need to proof at compile time that query implements all required traits for not only one backend, but all backends. Additionally for anything else than execute you would need to have trait bounds for the return type for all backends. This quickly leads to a horrific amount of trait bounds for even simple queries.

Using a macro will just remove the requirement of writing out those trait bounds initially. That works, but will lead to horrific compile time errors if something is wrong. (Additionally I think using a macro for this is really bad style as it's hard to document that in some meaningful way.)

To write up some requirements for a future reference:

  • Such a generic connection should have a possibility to support out of the box third party diesel connection implementations
  • We should find a way to support as much sql statements as possible there. Using a naive approach will just restrict the amount of supported SQL to that subset that is supported by all backends equally, which means not even something basic like LIMIT/OFFSET would be fully supported.
  • We should minimize code duplication as much as possible, so just implementing everything again for this specfic backend is no solution. Internally the corresponding backend specific methods to build the query should be called.

So again: Yes I think something can be possible, but I do not have the bandwidth to invest much time here. So anyone interested in this is welcome to try implementing this, but should not expect detailed mentoring here.

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

No branches or pull requests

2 participants