Merge pull request diesel-rs#2234 from diesel-rs/sg-fix-nested-joins

Correctly enforce nullability with nested joins

Author: weiznich

CHANGELOG.md

@@ -61,6 +61,12 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
   have been. All types in Diesel are now correctly only considered
   non-aggregate if their parts are.
 
+* Nullability requirements are now properly enforced for nested joins.
+  Previously, only the rules for the outer-most join were considered. For
+  example, `users.left_join(posts).left_join(comments)` would allow selecting
+  any columns from `posts`. That will now fail to compile, and any selections
+  from `posts` will need to be made explicitly nullable.
+
 ### Deprecated
 
 * `diesel_(prefix|postfix|infix)_operator!` have been deprecated. These macros

diesel/src/expression/nullable.rs

@@ -1,7 +1,7 @@
 use crate::backend::Backend;
 use crate::expression::*;
 use crate::query_builder::*;
-use crate::query_source::Table;
+use crate::query_source::joins::ToInnerJoin;
 use crate::result::QueryResult;
 use crate::sql_types::IntoNullable;
 
@@ -50,7 +50,7 @@ impl<T: QueryId> QueryId for Nullable<T> {
 impl<T, QS> SelectableExpression<QS> for Nullable<T>
 where
     Self: AppearsOnTable<QS>,
-    T: SelectableExpression<QS>,
-    QS: Table,
+    QS: ToInnerJoin,
+    T: SelectableExpression<QS::InnerJoin>,
 {
 }

diesel/src/lib.rs

@@ -95,8 +95,8 @@
 
 #![cfg_attr(feature = "unstable", feature(specialization))]
 // Built-in Lints
-#![deny(
-    warnings,
+#![deny(warnings)]
+#![warn(
     missing_debug_implementations,
     missing_copy_implementations,
     missing_docs

diesel/src/macros/mod.rs

@@ -44,7 +44,9 @@ macro_rules! __diesel_column {
             Join<Left, Right, LeftOuter>,
         > for $column_name where
             $column_name: AppearsOnTable<Join<Left, Right, LeftOuter>>,
-            Left: AppearsInFromClause<$table, Count=Once>,
+            Self: SelectableExpression<Left>,
+            // If our table is on the right side of this join, only
+            // `Nullable<Self>` can be selected
             Right: AppearsInFromClause<$table, Count=Never>,
         {
         }
@@ -53,7 +55,12 @@ macro_rules! __diesel_column {
             Join<Left, Right, Inner>,
         > for $column_name where
             $column_name: AppearsOnTable<Join<Left, Right, Inner>>,
-            Join<Left, Right, Inner>: AppearsInFromClause<$table, Count=Once>,
+            Left: AppearsInFromClause<$table>,
+            Right: AppearsInFromClause<$table>,
+            (Left::Count, Right::Count): Pick<Left, Right>,
+            Self: SelectableExpression<
+                <(Left::Count, Right::Count) as Pick<Left, Right>>::Selection,
+            >,
         {
         }
 
@@ -791,7 +798,7 @@ macro_rules! __diesel_table_impl {
                 use $crate::backend::Backend;
                 use $crate::query_builder::{QueryFragment, AstPass, SelectStatement};
                 use $crate::query_source::joins::{Join, JoinOn, Inner, LeftOuter};
-                use $crate::query_source::{AppearsInFromClause, Once, Never};
+                use $crate::query_source::{AppearsInFromClause, Once, Never, Pick};
                 use $crate::result::QueryResult;
                 $($imports)*
 

diesel/src/query_source/joins.rs

@@ -121,35 +121,6 @@ where
     }
 }
 
-impl<Left, Right, Kind, T> SelectableExpression<Join<Left, Right, Kind>> for Nullable<T>
-where
-    T: SelectableExpression<Join<Left, Right, Inner>>,
-    Nullable<T>: AppearsOnTable<Join<Left, Right, Kind>>,
-{
-}
-
-// FIXME: Remove this when overlapping marker traits are stable
-impl<Join, On, T> SelectableExpression<JoinOn<Join, On>> for Nullable<T>
-where
-    Nullable<T>: SelectableExpression<Join>,
-    Nullable<T>: AppearsOnTable<JoinOn<Join, On>>,
-{
-}
-
-// FIXME: Remove this when overlapping marker traits are stable
-impl<From, T> SelectableExpression<SelectStatement<From>> for Nullable<T>
-where
-    Nullable<T>: SelectableExpression<From>,
-    Nullable<T>: AppearsOnTable<SelectStatement<From>>,
-{
-}
-
-// FIXME: We want these blanket impls when overlapping marker traits are stable
-// impl<T, Join, On> SelectableExpression<JoinOn<Join, On>> for T where
-//     T: SelectableExpression<Join> + AppearsOnTable<JoinOn<Join, On>>,
-// {
-// }
-
 /// Indicates that two tables can be joined without an explicit `ON` clause.
 ///
 /// Implementations of this trait are generated by invoking [`joinable!`].
@@ -300,3 +271,41 @@ where
         (rhs.source, rhs.on)
     }
 }
+
+#[doc(hidden)]
+/// Convert any joins in a `FROM` clause into an inner join.
+///
+/// This trait is used to determine whether
+/// `Nullable<T>: SelectableExpression<SomeJoin>`. We consider it to be
+/// selectable if `T: SelectableExpression<InnerJoin>`. Since `SomeJoin`
+/// may be deeply nested, we need to recursively change any appearances of
+/// `LeftOuter` to `Inner` in order to perform this check.
+pub trait ToInnerJoin {
+    type InnerJoin;
+}
+
+impl<Left, Right, Kind> ToInnerJoin for Join<Left, Right, Kind>
+where
+    Left: ToInnerJoin,
+    Right: ToInnerJoin,
+{
+    type InnerJoin = Join<Left::InnerJoin, Right::InnerJoin, Inner>;
+}
+
+impl<Join, On> ToInnerJoin for JoinOn<Join, On>
+where
+    Join: ToInnerJoin,
+{
+    type InnerJoin = JoinOn<Join::InnerJoin, On>;
+}
+
+impl<From> ToInnerJoin for SelectStatement<From>
+where
+    From: ToInnerJoin,
+{
+    type InnerJoin = SelectStatement<From::InnerJoin>;
+}
+
+impl<T: Table> ToInnerJoin for T {
+    type InnerJoin = T;
+}

diesel/src/query_source/mod.rs

@@ -76,3 +76,83 @@ pub trait AppearsInFromClause<QS> {
     /// How many times does `Self` appear in `QS`?
     type Count;
 }
+
+#[doc(hidden)]
+/// Used to determine which of two from clauses contains a given table.
+///
+/// This trait can be used to emulate "or" conditions in where clauses when
+/// we want a trait to be implemented with one of two type parameters.
+///
+/// For example, if we wanted to write:
+///
+/// ```rust,ignore
+/// where
+///     T: SelectableExpression<Left> | SelectableExpression<Right>,
+/// ```
+///
+/// we can emulate this by writing:
+///
+/// ```rust,ignore
+/// where
+///     Left: AppearsInFromClause<T::Table>,
+///     Right: AppearsInFromClause<T::Table>,
+///     (Left::Count, Right::Count): Pick<Left, Right>,
+///     T: SelectableExpression<
+///         <(Left::Count, Right::Count) as Pick<Left, Right>>::Selection,
+///     >,
+/// ```
+///
+/// In order to aquire the counts in the first place, we must already know
+/// the table we're searching for.
+pub trait Pick<Left, Right> {
+    /// The selected type.
+    ///
+    /// For `(Once, Never)` this type will be `Left`. For `(Never, Once)`, this type will be
+    /// `Right`
+    type Selection;
+}
+
+impl<Left, Right> Pick<Left, Right> for (Once, Never) {
+    type Selection = Left;
+}
+
+impl<Left, Right> Pick<Left, Right> for (Never, Once) {
+    type Selection = Right;
+}
+
+#[doc(hidden)]
+#[allow(
+    non_camel_case_types,
+    missing_debug_implementations,
+    missing_copy_implementations
+)]
+/// Everything in this module is here to give something more helpful than:
+///
+/// > (Never, Never): Pick<table1, table2> is not satisifed
+///
+/// Any of these impls can be deleted if they are getting in the way of
+/// other functionality. Any code which is using these impls is already
+/// failing to compile.
+mod impls_which_are_only_here_to_improve_error_messages {
+    use super::*;
+
+    pub struct this_table_doesnt_appear_in_the_from_clause_of_your_query;
+
+    impl<Left, Right> Pick<Left, Right> for (Never, Never) {
+        type Selection = this_table_doesnt_appear_in_the_from_clause_of_your_query;
+    }
+
+    pub struct this_table_appears_in_your_query_more_than_once_and_must_be_aliased;
+
+    impl<Left, Right, OtherCount> Pick<Left, Right> for (MoreThanOnce, OtherCount) {
+        type Selection = this_table_appears_in_your_query_more_than_once_and_must_be_aliased;
+    }
+
+    impl<Left, Right> Pick<Left, Right> for (Never, MoreThanOnce) {
+        type Selection = this_table_appears_in_your_query_more_than_once_and_must_be_aliased;
+    }
+
+    impl<Left, Right> Pick<Left, Right> for (Once, MoreThanOnce) {
+        type Selection = this_table_appears_in_your_query_more_than_once_and_must_be_aliased;
+    }
+}

diesel_compile_tests/tests/compile-fail/right_side_of_left_join_requires_nullable.rs

@@ -18,26 +18,122 @@ table! {
     }
 }
 
+table! {
+    pets {
+        id -> Integer,
+        user_id -> Integer,
+        name -> Text,
+    }
+}
+
 joinable!(posts -> users (user_id));
-allow_tables_to_appear_in_same_query!(posts, users);
+joinable!(pets -> users (user_id));
+allow_tables_to_appear_in_same_query!(posts, users, pets);
 sql_function!(fn lower(x: Text) -> Text);
 
 fn main() {
-    let conn = PgConnection::establish("some url").unwrap();
+}
+
+fn direct_joins() {
     let join = users::table.left_outer_join(posts::table);
 
     // Invalid, only Nullable<title> is selectable
     let _ = join.select(posts::title);
     //~^ ERROR E0271
+    //~| ERROR E0277
+    // Valid
+    let _ = join.select(posts::title.nullable());
+    // Valid -- NULL to a function will return null
+    let _ = join.select(lower(posts::title).nullable());
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(lower(posts::title));
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Invalid, Nullable<title> is selectable, but lower expects not-null
+    let _ = join.select(lower(posts::title.nullable()));
+    //~^ ERROR E0271
     //~| ERROR E0271
+}
+
+fn nested_outer_joins_left_associative() {
+
+    let join = users::table.left_outer_join(posts::table).left_outer_join(pets::table);
+
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(posts::title);
+    //~^ ERROR E0271
+    //~| ERROR E0277
     // Valid
     let _ = join.select(posts::title.nullable());
     // Valid -- NULL to a function will return null
     let _ = join.select(lower(posts::title).nullable());
     // Invalid, only Nullable<title> is selectable
     let _ = join.select(lower(posts::title));
     //~^ ERROR E0271
+    //~| ERROR E0277
+    // Invalid, Nullable<title> is selectable, but lower expects not-null
+    let _ = join.select(lower(posts::title.nullable()));
+    //~^ ERROR E0271
     //~| ERROR E0271
+}
+
+fn nested_mixed_joins_left_associative() {
+    let join = users::table.left_outer_join(posts::table).inner_join(pets::table);
+
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(posts::title);
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Valid
+    let _ = join.select(posts::title.nullable());
+    // Valid -- NULL to a function will return null
+    let _ = join.select(lower(posts::title).nullable());
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(lower(posts::title));
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Invalid, Nullable<title> is selectable, but lower expects not-null
+    let _ = join.select(lower(posts::title.nullable()));
+    //~^ ERROR E0271
+    //~| ERROR E0271
+}
+
+fn nested_outer_joins_right_associative() {
+    let join = pets::table.left_outer_join(users::table.left_outer_join(posts::table));
+
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(posts::title);
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Valid
+    let _ = join.select(posts::title.nullable());
+    // Valid -- NULL to a function will return null
+    let _ = join.select(lower(posts::title).nullable());
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(lower(posts::title));
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Invalid, Nullable<title> is selectable, but lower expects not-null
+    let _ = join.select(lower(posts::title.nullable()));
+    //~^ ERROR E0271
+    //~| ERROR E0271
+}
+
+fn nested_mixed_joins_right_associative() {
+    let join = pets::table.inner_join(users::table.left_outer_join(posts::table));
+
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(posts::title);
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Valid
+    let _ = join.select(posts::title.nullable());
+    // Valid -- NULL to a function will return null
+    let _ = join.select(lower(posts::title).nullable());
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(lower(posts::title));
+    //~^ ERROR E0271
+    //~| ERROR E0277
     // Invalid, Nullable<title> is selectable, but lower expects not-null
     let _ = join.select(lower(posts::title.nullable()));
     //~^ ERROR E0271