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.

Sign up for GitHub

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

Jump to bottom

Improve implementation of associations!! #363

Closed
wants to merge 2 commits into from
Closed

Improve implementation of associations!! #363

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b710b16
Improve implementation of associations!!
weiznich Jun 28, 2016
4b52fe4
Temporary change diesel dependency in codegen
weiznich Jul 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions diesel/src/associations/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,25 @@
use std::hash::Hash;
use query_source::Column;

pub trait Identifiable {
type Id: Hash + Eq + Copy;

fn id(&self) -> Self::Id;
}

pub trait BelongsTo<Parent: Identifiable> {
pub trait BelongsTo<Parent: Identifiable, FK> where FK: Column
{
fn foreign_key(&self) -> Parent::Id;
}

pub trait GroupedBy<Parent>: IntoIterator + Sized {
pub trait GroupedBy<Parent, FK>: IntoIterator + Sized {
fn grouped_by(self, parents: &[Parent]) -> Vec<Vec<Self::Item>>;
}

impl<Parent, Child> GroupedBy<Parent> for Vec<Child> where
Child: BelongsTo<Parent>,
Parent: Identifiable,
impl<Parent, Child, FK> GroupedBy<Parent, FK> for Vec<Child> where
FK: Column,
Child: BelongsTo<Parent, FK>,
Parent: Identifiable
{
fn grouped_by(self, parents: &[Parent]) -> Vec<Vec<Child>> {
use std::collections::HashMap;
Expand Down
6 changes: 2 additions & 4 deletions diesel/src/expression/expression_methods/global_expression_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,15 +313,13 @@ pub trait ExpressionMethods: Expression + Sized {
/// # }
/// #
/// # joinable!(posts -> users (user_id));
/// # select_column_workaround!(posts -> users (id, user_id, author_name));
/// # select_column_workaround!(users -> posts (id, name));
///
/// fn main() {
/// use self::users::dsl::*;
/// use self::posts::dsl::{posts, author_name};
/// use self::posts::dsl::{posts, author_name, user_id};
/// let connection = establish_connection();
///
/// let data = users.inner_join(posts)
/// let data = users.inner_join(posts, user_id)
/// .filter(name.nullable().eq(author_name))
/// .select(name)
/// .load::<String>(&connection);
Expand Down
1 change: 1 addition & 0 deletions diesel/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ pub mod prelude {
pub use query_dsl::*;
pub use query_source::{QuerySource, Queryable, Table, Column, JoinTo};
pub use result::{QueryResult, TransactionError, TransactionResult, ConnectionError, ConnectionResult, OptionalExtension};
pub use query_source::joins::{InnerJoinable, LeftJoinable};
}

pub use prelude::*;
Expand Down
143 changes: 79 additions & 64 deletions diesel/src/macros/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ macro_rules! column {
#[derive(Debug, Clone, Copy)]
pub struct $column_name;

impl Default for $column_name {
fn default() -> Self {
$column_name
}
}

impl $crate::expression::Expression for $column_name {
type SqlType = $Type;
}
Expand Down Expand Up @@ -45,6 +51,26 @@ macro_rules! column {
{
}

impl<Left, Right, FK> $crate::expression::SelectableExpression<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This impl would allow the following incorrect query to compile:

users.inner_join(posts).select(comments::text)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked that I wasn't missing something, and I'm not. I've explored this change numerous times. We cannot write

impl<Other> SelectableExpression<InnerJoinSource<table, Other>> for column where
    Other: Table,
    table: JoinTo<Other, Inner>,
{}

impl<Other> SelectableExpression<InnerJoinSource<Other, table>> for column where
    Other: Table + JoinTo<table, Inner>,
{}

because they overlap if Other == table. For your impl to be correct, you'd need to add a SelectableExpression constraint, which would be:

impl<Left, Right> SelectableExpression<InnerJoinSource<Left, Right>> for column where
    Left: JoinTo<Right, Inner>,
    Right: Table,
    column: SelectableExpression<Left>,
{}

impl<Left, Right> SelectableExpression<InnerJoinSource<Left, Right>> for column where
    Left: JoinTo<Right, Inner>,
    Right: Table,
    column: SelectableExpression<Right>,
{}

which again, overlaps if Left and Right are the same type, or if column: SelectableExpression<Left> + SelectableExpression<Right>. There's no way to properly resolve this in the language today without a more in depth rethink of this code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely there should be a compile_test for this. I did not expect that to happen.
Maybe we could use JoinTo::JoinAllColumns to solve this. SelectableExpression<InnerJoinSource<Left, Right, FK>> should be implemented for each column there. Is there any "Trait" to check if a Tuple contains a certain type? If not, is it possible to write such thing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that trait doesn't exist and it would not be possible to write it today.

Copy link
Member Author

@weiznich weiznich Jun 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that trait doesn't exist and it would not be possible to write it today.

Using specialization this seems to be possible. Currently there seems to be a problem with the resolution of associated types when using specialization. See the following minimal example. Because of this the explicit specialization from line 27 to line 37 and from line 66 to line 72 is needed. There are already rust-lang issues for this.

$crate::query_source::joins::InnerJoinSource<Left, Right, FK>,
<$column_name as $crate::Expression>::SqlType,
> for $column_name where
Left: $crate::query_source::joins::JoinTo<Right, $crate::query_source::joins::Inner, FK>,
Right: $crate::query_source::Table,
FK: $crate::query_source::Column,
{
}

impl<Left, Right, FK, ST> $crate::expression::SelectableExpression<
$crate::query_source::LeftOuterJoinSource<Left, Right, FK>,
ST,
> for $column_name where
Left: $crate::query_source::Table + $crate::query_source::joins::JoinTo<Right, $crate::query_source::joins::LeftOuter, FK>,
Right: $crate::query_source::Table,
FK: $crate::query_source::Column,
{
}

impl $crate::expression::NonAggregate for $column_name {}

impl $crate::query_source::Column for $column_name {
Expand Down Expand Up @@ -226,6 +252,12 @@ macro_rules! table_body {
}
}

impl Default for table {
fn default() -> Self {
table
}
}

impl Table for table {
type PrimaryKey = columns::$pk;
type AllColumns = ($($column_name,)+);
Expand Down Expand Up @@ -288,17 +320,23 @@ macro_rules! table_body {
#[macro_export]
#[doc(hidden)]
macro_rules! joinable {
($child:ident -> $parent:ident ($source:ident)) => {
joinable_inner!($child::table => $parent::table : ($child::$source = $parent::table));
joinable_inner!($parent::table => $child::table : ($child::$source = $parent::table));
($child:ident -> $parent:ident ($fk:ident)) => {
joinable_inner!($child::table => $parent::table : ($child::$fk = $parent::table = $child::table));
joinable_inner!($parent::table => $child::table : ($child::$fk = $parent::table = $child::table));
}
}

#[macro_export]
#[doc(hidden)]
macro_rules! joinable_inner {
($left_table:path => $right_table:path : ($foreign_key:path = $parent_table:path)) => {
impl<JoinType> $crate::JoinTo<$right_table, JoinType> for $left_table {
($left_table:path => $right_table:path : ($foreign_key:path = $parent_table:path = $child_table:path)) => {
impl $crate::JoinTo<$right_table, $crate::query_source::joins::LeftOuter, $foreign_key> for $left_table {
type JoinSqlType = (<$parent_table as $crate::query_builder::AsQuery>::SqlType,
<<$child_table as $crate::query_builder::AsQuery>::SqlType as $crate::types::IntoNullable>::Nullable);
type JoinAllColumns = (<$parent_table as $crate::query_source::Table>::AllColumns,
<$child_table as $crate::query_source::Table>::AllColumns);

type ParentTable = $parent_table;
type JoinClause = $crate::query_builder::nodes::Join<
<$left_table as $crate::QuerySource>::FromClause,
<$right_table as $crate::QuerySource>::FromClause,
Expand All @@ -307,10 +345,10 @@ macro_rules! joinable_inner {
$crate::expression::nullable::Nullable<
<$parent_table as $crate::query_source::Table>::PrimaryKey>,
>,
JoinType,
$crate::query_source::joins::LeftOuter,
>;

fn join_clause(&self, join_type: JoinType) -> Self::JoinClause {
fn join_clause(&self, join_type: $crate::query_source::joins::LeftOuter) -> Self::JoinClause {
use $crate::QuerySource;

$crate::query_builder::nodes::Join::new(
Expand All @@ -320,74 +358,51 @@ macro_rules! joinable_inner {
join_type,
)
}
}
}
}

#[macro_export]
#[doc(hidden)]
macro_rules! select_column_workaround {
($parent:ident -> $child:ident ($($column_name:ident),+)) => {
$(select_column_inner!($parent -> $child $column_name);)+
select_column_inner!($parent -> $child star);
}
}

#[macro_export]
#[doc(hidden)]
macro_rules! select_column_inner {
($parent:ident -> $child:ident $column_name:ident) => {
impl $crate::expression::SelectableExpression<
$crate::query_source::InnerJoinSource<$child::table, $parent::table>,
> for $parent::$column_name
{
fn join_all_columns() -> Self::JoinAllColumns {
(<$parent_table as $crate::query_source::Table>::all_columns(),
<$child_table as $crate::query_source::Table>::all_columns())
}
}

impl $crate::expression::SelectableExpression<
$crate::query_source::InnerJoinSource<$parent::table, $child::table>,
> for $parent::$column_name
{
}
impl $crate::JoinTo<$right_table, $crate::query_source::joins::Inner, $foreign_key> for $left_table {
type JoinSqlType = (<$parent_table as $crate::query_builder::AsQuery>::SqlType,
<$child_table as $crate::query_builder::AsQuery>::SqlType);
type JoinAllColumns = (<$parent_table as $crate::query_source::Table>::AllColumns,
<$child_table as $crate::query_source::Table>::AllColumns);

type ParentTable = $parent_table;
type JoinClause = $crate::query_builder::nodes::Join<
<$left_table as $crate::QuerySource>::FromClause,
<$right_table as $crate::QuerySource>::FromClause,
$crate::expression::helper_types::Eq<
$crate::expression::nullable::Nullable<$foreign_key>,
$crate::expression::nullable::Nullable<
<$parent_table as $crate::query_source::Table>::PrimaryKey>,
>,
$crate::query_source::joins::Inner,
>;

impl $crate::expression::SelectableExpression<
$crate::query_source::LeftOuterJoinSource<$child::table, $parent::table>,
<<$parent::$column_name as $crate::Expression>::SqlType
as $crate::types::IntoNullable>::Nullable,
> for $parent::$column_name
{
}
fn join_clause(&self, join_type: $crate::query_source::joins::Inner) -> Self::JoinClause {
use $crate::QuerySource;

impl $crate::expression::SelectableExpression<
$crate::query_source::LeftOuterJoinSource<$parent::table, $child::table>,
> for $parent::$column_name
{
}
}
}
$crate::query_builder::nodes::Join::new(
self.from_clause(),
$right_table.from_clause(),
$foreign_key.nullable().eq($parent_table.primary_key().nullable()),
join_type,
)
}

#[macro_export]
#[doc(hidden)]
macro_rules! join_through {
($parent:ident -> $through:ident -> $child:ident) => {
impl<JoinType: Copy> $crate::JoinTo<$child::table, JoinType> for $parent::table {
type JoinClause = <
<$parent::table as $crate::JoinTo<$through::table, JoinType>>::JoinClause
as $crate::query_builder::nodes::CombinedJoin<
<$through::table as $crate::JoinTo<$child::table, JoinType>>::JoinClause,
>>::Output;

fn join_clause(&self, join_type: JoinType) -> Self::JoinClause {
use $crate::query_builder::nodes::CombinedJoin;
let parent_to_through = $crate::JoinTo::<$through::table, JoinType>
::join_clause(&$parent::table, join_type);
let through_to_child = $crate::JoinTo::<$child::table, JoinType>
::join_clause(&$through::table, join_type);
parent_to_through.combine_with(through_to_child)
fn join_all_columns() -> Self::JoinAllColumns {
(<$parent_table as $crate::query_source::Table>::all_columns(),
<$child_table as $crate::query_source::Table>::all_columns())
}
}
}
}


/// Takes a query QueryFragment expression as an argument and returns a string
/// of SQL with placeholders for the dynamic values.
///
Expand Down
25 changes: 17 additions & 8 deletions diesel/src/query_builder/select_statement/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use std::marker::PhantomData;
use backend::Backend;
use expression::*;
use query_source::*;
use query_source::joins::{InnerJoinable, LeftJoinable};
use result::QueryResult;
use super::distinct_clause::NoDistinctClause;
use super::group_by_clause::NoGroupByClause;
Expand Down Expand Up @@ -65,14 +66,18 @@ impl<ST, S, F, D, W, O, L, Of, G> SelectStatement<ST, S, F, D, W, O, L, Of, G> {
}
}

pub fn inner_join<T>(self, other: T)
-> SelectStatement<ST, S, InnerJoinSource<F, T>, D, W, O, L, Of, G> where
pub fn inner_join<T, FK>(self, other: T, by: FK)
-> SelectStatement<ST, S, InnerJoinSource<F, T, FK>, D, W, O, L, Of, G> where
T: Table,
F: Table + JoinTo<T, joins::Inner>,
F: Table + JoinTo<T, joins::Inner, FK> + InnerJoinable,
FK: Column,
F::JoinAllColumns: SelectableExpression<InnerJoinSource<F, T,FK>,
<F as JoinTo<T, joins::Inner, FK>>::JoinSqlType>

{
SelectStatement::new(
self.select,
self.from.inner_join(other),
self.from.inner_join(other, by),
self.distinct,
self.where_clause,
self.order,
Expand All @@ -82,14 +87,18 @@ impl<ST, S, F, D, W, O, L, Of, G> SelectStatement<ST, S, F, D, W, O, L, Of, G> {
)
}

pub fn left_outer_join<T>(self, other: T)
-> SelectStatement<ST, S, LeftOuterJoinSource<F, T>, D, W, O, L, Of, G> where
pub fn left_outer_join<T, FK>(self, other: T, by: FK)
-> SelectStatement<ST, S, LeftOuterJoinSource<F, T, FK>, D, W, O, L, Of, G> where
F: JoinTo<T, joins::LeftOuter, FK> + LeftJoinable,
T: Table,
F: Table + JoinTo<T, joins::LeftOuter>,
FK: Column,
T::SqlType: ::types::IntoNullable,
F::JoinAllColumns: SelectableExpression<LeftOuterJoinSource<F, T, FK>,
<F as JoinTo<T, joins::LeftOuter, FK>>::JoinSqlType>
{
SelectStatement::new(
self.select,
self.from.left_outer_join(other),
self.from.left_outer_join(other, by),
self.distinct,
self.where_clause,
self.order,
Expand Down
2 changes: 1 addition & 1 deletion diesel/src/query_dsl/belonging_to_dsl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use query_builder::AsQuery;

pub trait BelongingToDsl<T: ?Sized> {
pub trait BelongingToDsl<T: ?Sized, FK> {
type Output: AsQuery;

fn belonging_to(other: &T) -> Self::Output;
Expand Down
4 changes: 2 additions & 2 deletions diesel/src/query_dsl/filter_dsl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ impl<T, Predicate, ST> FilterDsl<Predicate> for T where
}

impl<T: Table> NotFiltered for T {}
impl<Left, Right> NotFiltered for InnerJoinSource<Left, Right> {}
impl<Left, Right> NotFiltered for LeftOuterJoinSource<Left, Right> {}
impl<Left, Right, FK> NotFiltered for InnerJoinSource<Left, Right, FK> {}
impl<Left, Right, FK> NotFiltered for LeftOuterJoinSource<Left, Right, FK> {}

use expression::AsExpression;
use expression::expression_methods::*;
Expand Down
Loading