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

Allow some clauses to be public #2536

Closed
wants to merge 1 commit into from
Closed

Allow some clauses to be public #2536

wants to merge 1 commit into from

Conversation

pksunkara
Copy link
Member

@pksunkara pksunkara commented Oct 11, 2020

This is a blocker for the proc-macro based ORM I was talking about a few months ago.

let user: Option<User> = User::one().github_id(id).load().await?;

Basically, the proc macro adds methods for all columns using select clauses so that select statements can be used simply.

I am working on more stuff, but I wanted to send this first to make sure you are okay with making these public (even though they are hidden).

@weiznich
Copy link
Member

I'm opposed to this change in the current form. We consider items marked with #[doc(hidden)] not to be part of our public API. Therefore such items can and will change at any time without notice. You should not use them to build another crate on top of them. Additionally using #[allow(missing_docs)] is a no-go for us.
Making those items part of the real public API can be possible if you add the following information and changes to this PR:

  • For each item add a concrete example what currently cannot be written using the public API and how this would change given that this item is public. Your motivating example above does not really tell anything of that.
  • Add proper documentation for each new public item. It's fine for such documentation to mention that normal users normally don't need to interact with this.

@weiznich
Copy link
Member

@pksunkara Do you plan to come back at the questions above? Otherwise I will close this as won't accept.

@pksunkara
Copy link
Member Author

I plan to. Just finishing off some minor stuff first.

@pksunkara
Copy link
Member Author

pksunkara commented Nov 1, 2020

TL;DR: I need to save half-formed SelectStatement which is currently not possible.


Let's say I have an users table and the following diesel struct for it:

pub struct User {
    pub id: i32,
    pub login: String,
    pub email: String,
    pub name: Option<String>,
}

Our use case is selecting a single user whose login is bob and email is bob@gmail.com, We can write that in diesel like below:

use crate::schema::users;

let user = users::table
    .filter(users::login.eq("bob"))
    .filter(users::email.eq("bob@gmail.com"))
    .limit(1)
    .get_result_async::<User>(&DB)
    .await
    .optional()?;

What I want to do is provide a proc macro derive that generates something like the following:

NOTE: I have shortened the AsExpression generics for clarity in the below example.

impl User {
    pub fn one() -> Self {
        Self {
            limit: 1,
            stmt: users::table::table().as_query()
        }
    }
}

impl UserSelect {
    pub fn login(mut self, login: AsExpression<users::login>) -> Self {
        Self {
            limit: self.limit,
            stmt: self.filter(users::login.eq(login))
        }
    }

    pub fn login(mut self, email: AsExpression<users::email>) -> Self {
        Self {
            limit: self.limit,
            stmt: self.filter(users::email.eq(email))
        }
    }

    pub async fn load(mut self) -> Result<Option<User>, _> {
        let select = self.stmt.select((
            users::id,
            users::login,
            users::email,
            users::name,
        ));

        select.limit(self.limit)
            .get_result_async::<User>(&DB)
            .await
            .optional()
    }
}

Now, the above use case can be written in an ORM syntax:

let user = User::one().login("bob").email("bob@gmail.com").load().await?;

I require the following:

  • diesel::query_builder::distinct_clause::NoDistinctClause
  • diesel::query_builder::where_clause::ValidWhereClause

For the load to work, the stmt field in the above UserSelect struct needs to be of type SelectStatement over a generic W: where_clause::ValidWhereClause<users::table>.

But since distinct generic appears before where generic in SelectStatement, I would have to compulsorily specify the default distinct_clause type NoDistinctClause.


I have read the diesel code quite a bit and this is the only solution I could come up with. Please correct me if I am wrong. I am pasting below the full generated code for reference:

Code generated by the existing proc-macro implementation
#[allow(dead_code, unreachable_code)]
impl User {
    pub fn one() -> QueryUser<
        ::diesel::query_builder::SelectStatement<schema::users::table>,
    > {
        QueryUser::new()
    }
}
pub struct QueryUser<SS> {
    limit: Option<i64>,
    offset: Option<i64>,
    statement: SS,
}
impl QueryUser<::diesel::query_builder::SelectStatement<schema::users::table>> {
    fn new() -> Self {
        use ::diesel::associations::HasTable;
        use ::diesel::query_builder::AsQuery;
        Self {
            limit: None,
            offset: None,
            statement: schema::users::table::table().as_query(),
        }
    }
}
#[allow(dead_code, unreachable_code)]
impl<SS> QueryUser<SS> {
    pub fn limit(mut self, limit: i64) -> Self {
        self.limit = Some(limit);
        self
    }
    pub fn offset(mut self, offset: i64) -> Self {
        self.offset = Some(offset);
        self
    }
}
impl<SS> QueryUser<SS> {
    pub fn id<E, X, O>(self, id: E) -> QueryUser<O>
    where
        SS: ::diesel::query_dsl::filter_dsl::FilterDsl<
            ::diesel::expression::grouped::Grouped<
                ::diesel::expression::operators::Eq<schema::users::id, X>,
            >,
            Output = O,
        >,
        E: ::diesel::expression::AsExpression<
            ::diesel::dsl::SqlTypeOf<schema::users::id>,
            Expression = X,
        >,
        X: ::diesel::expression::Expression<
            SqlType = ::diesel::dsl::SqlTypeOf<schema::users::id>,
        >,
    {
        QueryUser {
            limit: self.limit,
            offset: self.offset,
            statement: self.statement.filter(schema::users::id.eq(id)),
        }
    }
    pub fn login<E, X, O>(self, login: E) -> QueryUser<O>
    where
        SS: ::diesel::query_dsl::filter_dsl::FilterDsl<
            ::diesel::expression::grouped::Grouped<
                ::diesel::expression::operators::Eq<schema::users::login, X>,
            >,
            Output = O,
        >,
        E: ::diesel::expression::AsExpression<
            ::diesel::dsl::SqlTypeOf<schema::users::login>,
            Expression = X,
        >,
        X: ::diesel::expression::Expression<
            SqlType = ::diesel::dsl::SqlTypeOf<schema::users::login>,
        >,
    {
        QueryUser {
            limit: self.limit,
            offset: self.offset,
            statement: self.statement.filter(schema::users::login.eq(login)),
        }
    }
    pub fn email<E, X, O>(self, email: E) -> QueryUser<O>
    where
        SS: ::diesel::query_dsl::filter_dsl::FilterDsl<
            ::diesel::expression::grouped::Grouped<
                ::diesel::expression::operators::Eq<schema::users::email, X>,
            >,
            Output = O,
        >,
        E: ::diesel::expression::AsExpression<
            ::diesel::dsl::SqlTypeOf<schema::users::email>,
            Expression = X,
        >,
        X: ::diesel::expression::Expression<
            SqlType = ::diesel::dsl::SqlTypeOf<schema::users::email>,
        >,
    {
        QueryUser {
            limit: self.limit,
            offset: self.offset,
            statement: self.statement.filter(schema::users::email.eq(email)),
        }
    }
    pub fn name<E, X, O>(self, name: E) -> QueryUser<O>
    where
        SS: ::diesel::query_dsl::filter_dsl::FilterDsl<
            ::diesel::expression::grouped::Grouped<
                ::diesel::expression::operators::Eq<schema::users::name, X>,
            >,
            Output = O,
        >,
        E: ::diesel::expression::AsExpression<
            ::diesel::dsl::SqlTypeOf<schema::users::name>,
            Expression = X,
        >,
        X: ::diesel::expression::Expression<
            SqlType = ::diesel::dsl::SqlTypeOf<schema::users::name>,
        >,
    {
        QueryUser {
            limit: self.limit,
            offset: self.offset,
            statement: self.statement.filter(schema::users::name.eq(name)),
        }
    }
}
impl<S, W>
    QueryUser<
        ::diesel::query_builder::SelectStatement<
            schema::users::table,
            S,
            ::diesel::query_builder::distinct_clause::NoDistinctClause,
            W,
        >,
    >
where
    W: ::diesel::query_builder::QueryId
        + ::diesel::query_builder::QueryFragment<::diesel::pg::Pg>
        + ::diesel::query_builder::where_clause::ValidWhereClause<schema::users::table>
        + Send
        + 'static,
{
    pub async fn load(
        self,
    ) -> Result<Option<User>, ::tokio_diesel::AsyncError> {
        use ::tokio_diesel::{AsyncRunQueryDsl, OptionalExtension};
        let select = self.statement.select((
            schema::users::id,
            schema::users::login,
            schema::users::email,
            schema::users::name,
        ));
        select
            .limit(1)
            .get_result_async::<User>(&DB)
            .await
            .optional()
    }
}

This is just the beginning though. And only the select statement. I might want to open up more parts of the query_builder as I expand the ORM-y syntax support.

@weiznich
Copy link
Member

weiznich commented Nov 1, 2020

I have read the diesel code quite a bit and this is the only solution I could come up with. Please correct me if I am wrong.

There are already multiple ways to solve this problem exposed via the public API:

  • boxed queriers constructed via into_boxed(), which have much less generic parameters that need to be handled. (I would likely use this variant)
  • Box<dyn BoxableExpression> for storing different expressions in the same variable. (That could be used to just store your filter instead of having to store the whole select statement)
  • Type constructors in diesel::dsl, which should allow you to refer to whatever type is returned from some query where diesel does not expose this type via it's public API.

Given those existing API's where each one on it's own should allow you to write something like your generated code I really would like to hear some justication why we do need yet another way to solve this. Additionally I would like to underdtand why you failed to find at least on of the proposed solutions as all of them ar documented in our API docs + at least for the first one there is a guide that mentions this variant.

@pksunkara
Copy link
Member Author

  • boxed queriers constructed via into_boxed(), which have much less generic parameters that need to be handled. (I would likely use this variant)
  • Box<dyn BoxableExpression> for storing different expressions in the same variable. (That could be used to just store your filter instead of having to store the whole select statement)

My first attempt did the same but I ran into a few issues which I don't remember now, which is why I took another approach and ruled out boxed approach as a way to store the half-formed statement.

I didn't fail to find them or anything.

Now, I used BoxedQuery built in schema.rs (which I weirdly missed until now) and it is working without the need for this branch. Thanks for the alternate solution.


I want to pick your brain to see if I can shorten the trait bounds in the below (maybe using the types in diesel::dsl?):

pub fn id<E, X>(mut self, id: E) -> Self
where
    E: ::diesel::expression::AsExpression<
        ::diesel::dsl::SqlTypeOf<schema::users::id>,
        Expression = X,
    >,
    X: ::diesel::expression::Expression<SqlType = ::diesel::dsl::SqlTypeOf<schema::users::id>>
        + diesel::query_builder::QueryFragment<diesel::pg::Pg>
        + diesel::SelectableExpression<schema::users::table>
        + diesel::expression::ValidGrouping<
            (),
            IsAggregate = diesel::expression::is_aggregate::Never,
        > + Send
        + 'static,
{
    self.statement = self.statement.filter(schema::users::id.eq(id));
    self
}

where statement is of type schema::users::BoxedQuery<'static, DB>

@pksunkara pksunkara closed this Nov 1, 2020
@pksunkara pksunkara deleted the model branch November 1, 2020 13:07
@weiznich
Copy link
Member

weiznich commented Nov 2, 2020

I want to pick your brain to see if I can shorten the trait bounds in the below (maybe using the types in diesel::dsl?):

Without having tested that it should be something like this:

pub fn id<E, X>(mut self, id: E) -> Self
where
    E: ::diesel::dsl::AsExpr<X, schema::users::id>
    X: ::diesel::expression::BoxableExpression<schema::users::table, diesel::pg::Pg, SqlType = ::diesel::dsl::SqlTypeOf<schema::users::id>> + Send + 'static,
{
    self.statement = self.statement.filter(schema::users::id.eq(id));
    self
}

@pksunkara
Copy link
Member Author

diesel::dsl::AsExpr won't work because type aliases can't be used as traits.

And I get the following for X:

error[E0271]: type mismatch resolving `<diesel::expression::bound::Bound<diesel::sql_types::Integer, i32> as diesel::expression::ValidGrouping<()>>::IsAggregate == diesel::expression::is_aggregate::No`
   --> src/main.rs:156:35
    |
156 |     println!("{:#?}", User::one().id(1).load().await);
    |                                   ^^ expected struct `diesel::expression::is_aggregate::Never`, found struct `diesel::expression::is_aggregate::No`
    |
    = note: required because of the requirements on the impl of `diesel::BoxableExpression<schema::users::table, diesel::pg::Pg>` for `diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>`

I am looking into why the error is coming, but just wanted to let you know.

@weiznich
Copy link
Member

weiznich commented Nov 2, 2020

diesel::dsl::AsExpr won't work because type aliases can't be used as traits.

Oh, right. In that case there is not much left that could be improved there. (Maybe removing the second generic parameter and put the constraint on the associated type directly. So instead of having Expression = X + X: … just do E::Expression: ….

I am looking into why the error is coming, but just wanted to let you know.

That's because I've forgot that I did not merge #2551 yet. That should fix this error.

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.

None yet

2 participants