RunQueryDsl: match same trait constraints as diesel

[eakoli]

This resolves issue #142 which stems from the async RunQueryDsl being implemented for ALL types.

In addition it adds some constraints that were missing from async LoadQuery when compared to diesel LoadQuery

Comparing the original diesel RunQueryDsl to the async one, the original only implements RunQueryDsl for types that implement Table and a few other explicitly tagged (like SqlQuery)

https://github.com/diesel-rs/diesel/blob/v2.2.7/diesel/src/query_dsl/mod.rs#L1785-L1791

the following shows how there is not an issue with diesel proper, only with diesel_async::RunQueryDsl

mod sub {

    pub(crate) struct A {
        pub(crate) items: Vec<String>,
    }
}

mod success_1 {
    fn test(value: super::sub::A) -> String {
        value.items.first().map(|s| s.into()).unwrap() // properly compiles and uses Vec::first
    }
}

mod success_2 {
    use diesel::prelude::*;
    fn test(value: super::sub::A) -> String {
        value.items.first().map(|s| s.into()).unwrap() // properly compiles and uses Vec::first
    }
}

mod failure {
    use diesel::prelude::*;
    use diesel_async::RunQueryDsl;

    fn test(value: super::sub::A) -> String {
        value.items.first().map(|s| s.into()).unwrap() // complains Table is not implemented by Vec<std::string::String>
    }
}

[weiznich]

Thanks for submitting this PR. I fear this approach won't work for the reasons outlined below.

[weiznich]

I fear this assumption is not correct. See for example this impl: https://docs.diesel.rs/master/diesel/prelude/trait.RunQueryDsl.html#impl-RunQueryDsl%3CConn%3E-for-BoxedSqlQuery%3C'_,+%3CConn+as+Connection%3E::Backend,+Query%3E

For the linked impl these trait bounds would imply that Conn implements AsyncConnection and Connection at the same time, which is generally not fulfilled by any type.

Also even if we get it so that diesel doesn't have this kind of constraints internally that wouldn't help with third party crates which totally could write connection specific impls.

[eakoli]

Im not sure I follow what the reason is, are you saying that the where ends up being too restrictive?

For the linked impl these trait bounds would imply that Conn implements AsyncConnection and Connection at the same time, which is generally not fulfilled by any type.

Where does it imply that Conn implements anything? It only makes assertion with respect to T, not to Conn.

Also even if we get it so that diesel doesn't have this kind of constraints internally that wouldn't help with third party crates which totally could write connection specific impls.

What internal constraint are you referring to? And connection specific impl of what?

What about something a little more explicit?

    impl <T, Conn: Connection, AsyncConn> RunQueryDsl<AsyncConn> for T where T: RunQueryDsl<Conn> {}

[weiznich]

I'm not sure if we are talking about the same impl, because the linked impl literally reads as follows:

impl<Conn: Connection, Query> RunQueryDsl<Conn> for BoxedSqlQuery<'_, Conn::Backend, Query>

which obviously does restrict Conn to the Connection trait from diesel.

For this trait implementation the changed impl would imply that Conn needs at the same time implement AsyncConnection and Connection (via the T: diesel::RunQueryDsl<Conn> bound). That's not possible to be fulfilled by any connection type.

What about something a little more explicit?

 impl <T, Conn: Connection, AsyncConn> RunQueryDsl<AsyncConn> for T where T: RunQueryDsl<Conn> {}

I fear it's unfortunally not possible to use different generics for these impl due to how the trait system in rust works. You will get an error about an unconstrained generic parametr (Conn) in that case. Otherwise this would likely be the right way to solve this issue.

[eakoli]

Ok, I see what you mean with the conn :Connection, and unmatched type.

What about T where T: AsQuery, as this comment implies it really should be ?

I pushed up a changes to match on AsQuery, as well as tests to verify all the current types that work for RunQueryDsl also work for diesel_async::RunQueryDsl<conn: AsyncConnection>

[weiznich]

Sorry for taking that long to answer, but I really did not have the time to actually check whether the comment there is still relevant.
I did now have some time and checked with the following example:

async fn test(conn: &mut AsyncPgConnection) {
    users::table
        .filter(posts::id.eq(42))
        .load::<(i32,)>(conn)
        .await;
}

without your patch that generates the following error:

error[E0271]: type mismatch resolving `<table as AppearsInFromClause<table>>::Count == Once`
  --> src/main.rs:11:10
   |
11 |         .load::<(i32,)>(conn)
   |          ^^^^ expected `Once`, found `Never`
   |
note: required for `posts::columns::id` to implement `AppearsOnTable<users::table>`
  --> src/main.rs:6:25
   |
6  | diesel::table! { posts{ id -> Integer}}
   |                         ^^
   = note: associated types for the current `impl` cannot be restricted in `where` clauses
   = note: 2 redundant requirements hidden
   = note: required for `Grouped<Eq<id, Bound<Integer, i32>>>` to implement `AppearsOnTable<users::table>`
   = note: required for `WhereClause<Grouped<Eq<id, Bound<Integer, i32>>>>` to implement `query_builder::where_clause::ValidWhereClause<FromClause<users::table>>`
   = note: required for `SelectStatement<FromClause<table>, DefaultSelectClause<...>, ..., ...>` to implement `Query`
   = note: required for `SelectStatement<FromClause<table>, DefaultSelectClause<...>, ..., ...>` to implement `diesel_async::methods::LoadQuery<'_, _, (i32,)>`
   = note: the full name for the type has been written to '/tmp/diesel_test/target/debug/deps/diesel_test-acb1381933ce240a.long-type-13903671074851835938.txt'
   = note: consider using `--verbose` to print the full type name to the console

error[E0277]: the trait bound `users::table: TableNotEqual<posts::table>` is not satisfied
  --> src/main.rs:11:10
   |
11 |         .load::<(i32,)>(conn)
   |          ^^^^ the trait `TableNotEqual<posts::table>` is not implemented for `users::table`
   |
   = note: double check that `posts::table` and `users::table` appear in the same `allow_tables_to_appear_in_same_query!` 
           call if both are tables
   = help: the following other types implement trait `TableNotEqual<T>`:
             `Only<diesel::pg::metadata_lookup::pg_namespace::table>` implements `TableNotEqual<diesel::pg::metadata_lookup::pg_type::table>`
             `Only<diesel::pg::metadata_lookup::pg_type::table>` implements `TableNotEqual<diesel::pg::metadata_lookup::pg_namespace::table>`
             `Tablesample<diesel::pg::metadata_lookup::pg_namespace::table, TSM>` implements `TableNotEqual<diesel::pg::metadata_lookup::pg_type::table>`
             `Tablesample<diesel::pg::metadata_lookup::pg_type::table, TSM>` implements `TableNotEqual<diesel::pg::metadata_lookup::pg_namespace::table>`
             `diesel::pg::metadata_lookup::pg_namespace::table` implements `TableNotEqual<Only<diesel::pg::metadata_lookup::pg_type::table>>`
             `diesel::pg::metadata_lookup::pg_namespace::table` implements `TableNotEqual<Tablesample<diesel::pg::metadata_lookup::pg_type::table, TSM>>`
             `diesel::pg::metadata_lookup::pg_namespace::table` implements `TableNotEqual<diesel::pg::metadata_lookup::pg_type::table>`
             `diesel::pg::metadata_lookup::pg_type::table` implements `TableNotEqual<Only<diesel::pg::metadata_lookup::pg_namespace::table>>`
           and 2 others
   = note: required for `users::table` to implement `AppearsInFromClause<posts::table>`
note: required for `posts::columns::id` to implement `AppearsOnTable<users::table>`
  --> src/main.rs:6:25
   |
6  | diesel::table! { posts{ id -> Integer}}
   |                         ^^
   = note: 2 redundant requirements hidden
   = note: required for `Grouped<Eq<id, Bound<Integer, i32>>>` to implement `AppearsOnTable<users::table>`
   = note: required for `WhereClause<Grouped<Eq<id, Bound<Integer, i32>>>>` to implement `query_builder::where_clause::ValidWhereClause<FromClause<users::table>>`
   = note: required for `SelectStatement<FromClause<table>, DefaultSelectClause<...>, ..., ...>` to implement `Query`
   = note: required for `SelectStatement<FromClause<table>, DefaultSelectClause<...>, ..., ...>` to implement `diesel_async::methods::LoadQuery<'_, _, (i32,)>`
   = note: the full name for the type has been written to '/tmp/diesel_test/target/debug/deps/diesel_test-acb1381933ce240a.long-type-13903671074851835938.txt'
   = note: consider using `--verbose` to print the full type name to the console

with your patch this generates that error:

error[E0599]: the method `load` exists for struct `SelectStatement<FromClause<table>, DefaultSelectClause<...>, ..., ...>`, but its trait bounds were not satisfied
  --> src/main.rs:11:10
   |
9  | /     users::table
10 | |         .filter(posts::id.eq(42))
11 | |         .load::<(i32,)>(conn)
   | |         -^^^^ method cannot be called due to unsatisfied trait bounds
   | |_________|
   |
   |
  ::: /home/weiznich/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/diesel-2.2.4/src/query_builder/select_statement/mod.rs:48:1
   |
48 | / #[diesel_derives::__diesel_public_if(
49 | |     feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes",
50 | |     public_fields(
51 | |         select,
...  |
61 | | )]
   | |__- doesn't satisfy `_: AsQuery` or `_: RunQueryDsl<_>`
   |
   = note: the full type name has been written to '/tmp/diesel_test/target/debug/deps/diesel_test-7a841758257f912e.long-type-11988839005461461334.txt'
   = note: consider using `--verbose` to print the full type name to the console
   = note: the following trait bounds were not satisfied:
           `SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: AsQuery`
           which is required by `SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: diesel_async::RunQueryDsl<_>`
           `&SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: AsQuery`
           which is required by `&SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: diesel_async::RunQueryDsl<_>`
           `&mut SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: AsQuery`
           which is required by `&mut SelectStatement<FromClause<users::table>, query_builder::select_clause::DefaultSelectClause<FromClause<users::table>>, query_builder::distinct_clause::NoDistinctClause, query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<posts::columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>>>>: diesel_async::RunQueryDsl<_>`
   = help: items from traits can only be used if the trait is in scope

So it seems like the comment over there in the diesel code is still relevant today. I'm sorry to say that, but in my opinion that's a too bad regression of the error messages emitted in such failure cases to be acceptable.