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

Query dsl generated by select leak private types to public interface #1037

Closed
weiznich opened this Issue Jul 24, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@weiznich
Contributor

weiznich commented Jul 24, 2017

Setup

Versions

  • Rust: 1.19
  • Diesel: 0.15
  • Database: Postgres
  • Operating System Ubuntu

Feature Flags

  • diesel: default-features = false, features = ["postgres", "chrono", "serde_json"]
  • diesel_codegen: default

Problem Description

What are you trying to accomplish?

Return a common select query part from a function (I know of boxed queries)

What is the expected output?

The code compiles (even if the the returning type is somewhat complex)

What is the actual output?

It is impossible to import all needed types, because some of them are inside of a private module of diesel. Therefore it is not possible to write down those type.

(In theory there should be a lint(private_in_public) for this in rustc, but it seems like the lint failed to catch this types)

Steps to reproduce

table! {
    groups {
        id -> Integer,
        group_name -> Text,
    }
}

fn query_groups() -> ::diesel::query_builder::SelectStatement<self::groups::table,
                                                              ::diesel::query_builder::select_clause::DefaultSelectClause,
                                                              ::diesel::query_builder::distinct_clause::NoDistinctClause,
                                                              ::diesel::query_builder::where_clause::WhereClause<
                                                                      ::diesel::expression::operators::Eq<self::groups::columns::id,
                                                                                                          ::diesel::expression::bound::Bound<::diesel::types::Integer, i32>>>>{
    use self::groups::dsl::*;
    groups.filter(id.eq(1))
}
error[E0603]: module `select_clause` is private
  --> src/lib.rs:77:63
   |
77 |                                                               ::diesel::query_builder::select_clause::DefaultSelectClause,
   |                                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0603]: module `distinct_clause` is private
  --> src/lib.rs:78:63
   |
78 |                                                               ::diesel::query_builder::distinct_clause::NoDistinctClause,
   |                                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Cause

This is basically caused because query_builder::select_clause and similar modules are private. Those types are used in query_builder::select_statement::SelectStatement for example as default types for the generic parameters. SelectStatement is exported as public type in query_builder

Maybe this should also be reported as bug of the private_in_public lint in rustc?

Checklist

  • I have already looked over the issue tracker for similar issues.
@killercup

This comment has been minimized.

Member

killercup commented Jul 24, 2017

Yeah, seems like this lint should trigger, but I don't know the specifics.

I think the original idea was to either use BoxedDsl or impl Trait. Where would you want this to be documented?

@weiznich

This comment has been minimized.

Contributor

weiznich commented Jul 24, 2017

As written above, I know about the alternatives. It simply seems to be inconsistent to have private types in the public interface.

@killercup

This comment has been minimized.

Member

killercup commented Jul 24, 2017

Yes, sorry, that was a bit short. I only wanted to add some links to people finding this issue in the future had some more context. :)

I don't think we can easily fix this right now, as the types are part of the API we don't want to stabilize without carefully thinking about and discussing it. But—I also did only think about this for a minute right now. What would you do? And, do you think we should document BoxedDsl somewhere prominently as a workaround?

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 24, 2017

You aren't meant to ever reference these types correctly. You should use the types in http://docs.diesel.rs/diesel/helper_types/index.html

@weiznich

This comment has been minimized.

Contributor

weiznich commented Jul 24, 2017

How does one use those helper types when the query source is no raw table, but a join?

Something like this?

 Select<Filter<Join<self::groups::table,
                               self::user_is_member_of_group::table,
                               joins::Inner>,
                        Eq<self::user_is_member_of_group::dsl::user_id,
                              Bound<Integer, i32>>>,
             Nullable<self::groups::dsl::id>>

for the following query

groups.inner_join(user_is_member_of_group)
           .filter(group_user_id.eq(self.id))
           .select(id.nullable())

Table dsl:

table! {
    groups {
        id -> Integer,
        group_name -> Text,
    }
}

table! {
    user_is_member_of_group (user_id, group_id) {
        user_id -> Integer,
        group_id -> Integer,
    }
}
@sgrif

This comment has been minimized.

Member

sgrif commented Jul 24, 2017

We should probably add helper types for InnerJoin and LeftOuterJoin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment