Address diesel-rs#3173

This is a tricky one. It seems like the behaviour described in that
issue should work out of the box, but it doesn't. I've spend some time
to investigate various solutions to make this work, but I came to the
conclusion that the current behaviour is the correct one.

The underlying issue here is that such an query promotes the inner
`Nullable<_>` of the field onto the outer `Queryable` wrapper. Without
`Selectable` that would require a select clause like
`.select((table::column.assume_not_null(),).nullable())`. This is
technically a safe pattern, but requires the usage of the "advanced"
`assume_not_null()` method to forcibly convert types to their not null representation.

Possible solutions tried to make the enable constructs shown in that
issue:

* I've tried to make the `impl Selectable for Option<T>` return the
following `SelectExpression`:
`dsl::Nullable<dsl::AssumeNotNull<T::SelectExpression>>`
where `AssumeNotNull` converts each tuple element to the corresponding
not nullable expression, while `Nullable` wraps the tuple itself into a
nullable type wrapper.
* I've tried to apply a similar approach like that one above, but only
for derived impls by manipulating the generated code for a optional
field with `#[diesel(embed)]`

Both solutions require changes to our sql type system, as for example
allowing to load a non nullable value into a `Option<T>` to enable their
usage in a more general scope as the presented example case.
(See the added test cases for this). That by itself would be fine in my
opinion, as this is always a safe operation. Unfortunately the
`AssumeNotNull` transformation would be applied recursively for all
sub-tuples, which in turn would cause trouble with nested joins (again
see the examples). We would be able to workaround this issue by allowing
the `FromSql<ST, DB> for Option<T>` impl for non-nullable types to catch
null values, which in turn really feels like a bad hack. (You would like
to get an error there instead, but nested nullable information are
gone.)
All in all this lead me to the conclusion that the current behaviour is
correct.

This PR adds a few additional tests (an adjusted version of the test
from the bug report + more tests around nested joins) and does move
around some code bits that I noticed while working on this.

I think the official answer for the bug report would be: Either wrap the
inner type also in an `Option` or provide a manual `Selectable` impl
that does the "right" thing there.

Author: weiznich

diesel/src/expression/nullable.rs

@@ -48,19 +48,6 @@ impl<T: QueryId> QueryId for Nullable<T> {
     const HAS_STATIC_QUERY_ID: bool = T::HAS_STATIC_QUERY_ID;
 }
 
-impl<T, DB> Selectable<DB> for Option<T>
-where
-    DB: Backend,
-    T: Selectable<DB>,
-    Nullable<T::SelectExpression>: Expression,
-{
-    type SelectExpression = Nullable<T::SelectExpression>;
-
-    fn construct_selection() -> Self::SelectExpression {
-        Nullable::new(T::construct_selection())
-    }
-}
-
 impl<T, QS> SelectableExpression<QS> for Nullable<T>
 where
     Self: AppearsOnTable<QS>,

diesel/src/query_dsl/load_dsl.rs

@@ -195,8 +195,8 @@ mod private {
     }
 
     #[cfg_attr(
-        feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes",
-        cfg(feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes")
+        doc_cfg,
+        doc(cfg(feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"))
     )]
     pub trait CompatibleType<U, DB> {
         type SqlType;

diesel/src/type_impls/option.rs

@@ -5,6 +5,7 @@ use crate::expression::*;
 use crate::query_builder::QueryId;
 use crate::serialize::{self, IsNull, Output, ToSql};
 use crate::sql_types::{is_nullable, HasSqlType, Nullable, SingleValue, SqlType};
+use crate::NullableExpressionMethods;
 
 impl<T, DB> HasSqlType<Nullable<T>> for DB
 where
@@ -109,15 +110,25 @@ where
     }
 }
 
-#[cfg(all(test, feature = "postgres"))]
-use crate::pg::Pg;
-#[cfg(all(test, feature = "postgres"))]
-use crate::sql_types;
+impl<T, DB> Selectable<DB> for Option<T>
+where
+    DB: Backend,
+    T: Selectable<DB>,
+    crate::dsl::Nullable<T::SelectExpression>: Expression,
+{
+    type SelectExpression = crate::dsl::Nullable<T::SelectExpression>;
+
+    fn construct_selection() -> Self::SelectExpression {
+        T::construct_selection().nullable()
+    }
+}
 
 #[test]
 #[cfg(feature = "postgres")]
 fn option_to_sql() {
+    use crate::pg::Pg;
     use crate::query_builder::bind_collector::ByteWrapper;
+    use crate::sql_types;
 
     type Type = sql_types::Nullable<sql_types::VarChar>;
 

diesel_derives/tests/as_changeset.rs

@@ -1,3 +1,5 @@
+use diesel::deserialize::FromSqlRow;
+use diesel::expression::AsExpression;
 use diesel::*;
 use helpers::*;
 use schema::*;

diesel_derives/tests/selectable.rs

@@ -86,3 +86,31 @@ fn embedded_option() {
     let data = my_structs::table.select(A::as_select()).get_result(conn);
     assert!(data.is_err());
 }
+
+#[test]
+fn embedded_option_with_nullable_field() {
+    table! {
+        my_structs (foo) {
+            foo -> Integer,
+            bar -> Nullable<Integer>,
+        }
+    }
+
+    #[derive(Debug, Clone, Copy, PartialEq, Eq, Queryable, Selectable)]
+    #[diesel(table_name = my_structs)]
+    struct A {
+        foo: i32,
+        #[diesel(embed)]
+        b: Option<B>,
+    }
+
+    #[derive(Debug, Clone, Copy, PartialEq, Eq, Queryable, Selectable)]
+    #[diesel(table_name = my_structs)]
+    struct B {
+        bar: Option<i32>,
+    }
+
+    let conn = &mut connection();
+    let data = my_structs::table.select(A::as_select()).get_result(conn);
+    assert!(data.is_err());
+}

diesel_derives/tests/tests.rs

@@ -1,5 +1,3 @@
-#![deny(warnings)]
-
 #[macro_use]
 extern crate cfg_if;
 #[macro_use]

diesel_tests/tests/joins.rs

@@ -701,7 +701,7 @@ fn selecting_crazy_nested_joins() {
     assert_eq!(Ok(expected), data);
 }
 
-fn connection_with_fixture_data_for_multitable_joins() -> (TestConnection, TestData) {
+pub(crate) fn connection_with_fixture_data_for_multitable_joins() -> (TestConnection, TestData) {
     let mut connection = connection_with_sean_and_tess_in_users_table();
 
     let sean = find_user_by_name("Sean", &mut connection);
@@ -775,11 +775,11 @@ fn connection_with_fixture_data_for_multitable_joins() -> (TestConnection, TestD
     (connection, test_data)
 }
 
-struct TestData {
-    sean: User,
-    tess: User,
-    posts: Vec<Post>,
-    comments: Vec<Comment>,
-    likes: Vec<Like>,
-    followings: Vec<Following>,
+pub struct TestData {
+    pub sean: User,
+    pub tess: User,
+    pub posts: Vec<Post>,
+    pub comments: Vec<Comment>,
+    pub likes: Vec<Like>,
+    pub followings: Vec<Following>,
 }

diesel_tests/tests/schema/backend_specifics.rs

@@ -1,6 +1,8 @@
 use super::*;
 
-#[derive(PartialEq, Eq, Debug, Clone, Queryable, Identifiable, Associations, QueryableByName)]
+#[derive(
+    PartialEq, Eq, Debug, Clone, Queryable, Identifiable, Associations, QueryableByName, Selectable,
+)]
 #[diesel(belongs_to(User))]
 #[diesel(table_name = posts)]
 pub struct Post {

diesel_tests/tests/schema/mod.rs

@@ -61,8 +61,8 @@ impl UserName {
     }
 }
 
-#[derive(PartialEq, Eq, Debug, Clone, Queryable, Identifiable, Associations)]
-#[diesel(belongs_to(Post))]
+#[derive(PartialEq, Eq, Debug, Clone, Queryable, Identifiable, Associations, Selectable)]
+#[diesel(belongs_to(Post), table_name = comments)]
 pub struct Comment {
     id: i32,
     post_id: i32,
@@ -79,7 +79,9 @@ impl Comment {
     }
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Queryable, Insertable, Associations, Identifiable)]
+#[derive(
+    Debug, Clone, Copy, PartialEq, Eq, Queryable, Insertable, Associations, Identifiable, Selectable,
+)]
 #[diesel(belongs_to(User))]
 #[diesel(belongs_to(Post))]
 #[diesel(table_name = followings)]
@@ -177,14 +179,16 @@ pub struct NullableColumn {
     value: Option<i32>,
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Queryable, Insertable, Identifiable, Associations)]
+#[derive(
+    Debug, Clone, Copy, PartialEq, Eq, Queryable, Insertable, Identifiable, Associations, Selectable,
+)]
 #[diesel(table_name = likes)]
-#[diesel(primary_key(user_id, comment_id))]
+#[diesel(primary_key(comment_id, user_id))]
 #[diesel(belongs_to(User))]
 #[diesel(belongs_to(Comment))]
 pub struct Like {
-    pub user_id: i32,
     pub comment_id: i32,
+    pub user_id: i32,
 }
 
 #[cfg(feature = "postgres")]

diesel_tests/tests/schema/postgres_specific_schema.rs

@@ -1,6 +1,8 @@
 use super::{posts, User};
 
-#[derive(PartialEq, Eq, Debug, Clone, Queryable, Identifiable, Associations, QueryableByName)]
+#[derive(
+    PartialEq, Eq, Debug, Clone, Queryable, Identifiable, Associations, QueryableByName, Selectable,
+)]
 #[diesel(table_name = posts)]
 #[diesel(belongs_to(User))]
 pub struct Post {