Skip to content

Commit ee05eb1

Browse files
sgrifsgrif
committed
Split SelectableExpression into two traits
The change in #709 had the side effect of re-introducing #104. With the design that we have right now, nullability isn't propagating upwards. This puts the issue of "expressions aren't validating that the type of its arguments haven't become nullable, and thus nulls are slipping in where they shouldn't be" at odds with "we can't use complex expressions in filters for joins because the SQL type changed". This semi-resolves the issue by restricting when we care about nullability. Ultimately the only time it really matters is when we're selecting data, as we need to enforce that the result goes into an `Option`. For places where we don't see the bytes in Rust (filter, order, etc), `NULL` is effectively `false`. This change goes back to fully fixing #104, but brings back a small piece of #621. I've changed everything that is a composite expression to only be selectable if the SQL type hasn't changed. This means that you won't be able to do things like `users.left_outer_join(posts).select(posts::id + 1)`, but you will be able to use whatever you want in `filter`. This change is also to support what I think will fix the root of all these issues. The design of "Here's the SQL type on this query source" is just fundamentally not what we need. There is only one case where the type changes, and that is to become null when it is on the right side of a left join, the left side of a right join, or either side of a full join. One of the changes that #709 made was to require that you explicitly call `.nullable()` on a tuple if you wanted to get `Option<(i32, String)>` instead of `(Option<i32>, Option<String>)`. This has worked out fine, and isn't a major ergonomic pain. The common case is just to use the default select clause anyway. So I want to go further down this path. The longer term plan is to remove `SqlTypeForSelect` entirely, and *not* implement `SelectableExpression` for columns on the nullable side of a join. We will then provide these two blanket impls: ```rust impl<Left, Right, T> SelectableExpression<LeftOuterJoin<Left, Right>> for Nullable<T> where T: SelectableExpression<Right>, {} impl<Left, Right, Head, Tail> SelectableExpression<LeftOuterJoin<Left, Right>> for Nullable<Cons<Head, Tail>> where Nullable<Head>: SelectableExpression<LeftOuterJoin<Left, Right>>, Nullable<Tail>: SelectableExpression<LeftOuterJoin<Left, Right>>, {} ``` (Note: Those impls overlap. Providing them as blanket impls would require rust-lang/rust#40097. Providing them as non-blanket impls would require us to mark `Nullable` and possibly `Cons` as `#[fundamental]`) The end result will be that nullability naturally propagates as we want it to. Given `sql_function!(lower, lower_t, (x: Text) -> Text)`, doing `select(lower(posts::name).nullable())` will work. `lower(posts::name)` will fail because `posts::name` doesn't impl `SelectableExpression`. `lower(posts::name.nullable())` will fail because while `SelectableExpression` will be met, the SQL type of the argument isn't what's expected. Putting `.nullable` at the very top level naturally follows SQL's semantics here.
1 parent d51de03 commit ee05eb1

30 files changed

+222
-154
lines changed

‎diesel/src/expression/array_comparison.rs

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -52,22 +52,6 @@ impl<T, U> Expression for NotIn<T, U> where
5252
type SqlType = Bool;
5353
}
5454

55-
impl<T, U, QS> SelectableExpression<QS> for In<T, U> where
56-
In<T, U>: Expression,
57-
T: SelectableExpression<QS>,
58-
U: SelectableExpression<QS>,
59-
{
60-
type SqlTypeForSelect = Self::SqlType;
61-
}
62-
63-
impl<T, U, QS> SelectableExpression<QS> for NotIn<T, U> where
64-
NotIn<T, U>: Expression,
65-
T: SelectableExpression<QS>,
66-
U: SelectableExpression<QS>,
67-
{
68-
type SqlTypeForSelect = Self::SqlType;
69-
}
70-
7155
impl<T, U> NonAggregate for In<T, U> where
7256
In<T, U>: Expression,
7357
{
@@ -138,6 +122,8 @@ impl<T, U, DB> QueryFragment<DB> for NotIn<T, U> where
138122

139123
impl_query_id!(In<T, U>);
140124
impl_query_id!(NotIn<T, U>);
125+
impl_selectable_expression!(In<T, U>);
126+
impl_selectable_expression!(NotIn<T, U>);
141127

142128
use std::marker::PhantomData;
143129
use query_builder::{SelectStatement, BoxedSelectStatement};
@@ -202,12 +188,18 @@ impl<T> MaybeEmpty for Many<T> {
202188
}
203189

204190
impl<T, QS> SelectableExpression<QS> for Many<T> where
205-
Many<T>: Expression,
191+
Many<T>: AppearsOnTable<QS>,
206192
T: SelectableExpression<QS>,
207193
{
208194
type SqlTypeForSelect = T::SqlTypeForSelect;
209195
}
210196

197+
impl<T, QS> AppearsOnTable<QS> for Many<T> where
198+
Many<T>: Expression,
199+
T: AppearsOnTable<QS>,
200+
{
201+
}
202+
211203
impl<T, DB> QueryFragment<DB> for Many<T> where
212204
DB: Backend,
213205
T: QueryFragment<DB>,
@@ -252,12 +244,18 @@ impl<T, ST> MaybeEmpty for Subselect<T, ST> {
252244
}
253245

254246
impl<T, ST, QS> SelectableExpression<QS> for Subselect<T, ST> where
255-
Subselect<T, ST>: Expression,
247+
Subselect<T, ST>: AppearsOnTable<QS>,
256248
T: Query,
257249
{
258250
type SqlTypeForSelect = ST;
259251
}
260252

253+
impl<T, ST, QS> AppearsOnTable<QS> for Subselect<T, ST> where
254+
Subselect<T, ST>: Expression,
255+
T: Query,
256+
{
257+
}
258+
261259
impl<T, ST, DB> QueryFragment<DB> for Subselect<T, ST> where
262260
DB: Backend,
263261
T: QueryFragment<DB>,

‎diesel/src/expression/bound.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use backend::Backend;
44
use query_builder::*;
55
use result::Error::SerializationError;
66
use result::QueryResult;
7-
use super::{Expression, SelectableExpression, NonAggregate};
7+
use super::*;
88
use types::{HasSqlType, ToSql, IsNull};
99

1010
#[derive(Debug, Clone, Copy)]
@@ -64,11 +64,16 @@ impl<T: QueryId, U> QueryId for Bound<T, U> {
6464
}
6565

6666
impl<T, U, QS> SelectableExpression<QS> for Bound<T, U> where
67-
Bound<T, U>: Expression,
67+
Bound<T, U>: AppearsOnTable<QS>,
6868
{
6969
type SqlTypeForSelect = T;
7070
}
7171

72+
impl<T, U, QS> AppearsOnTable<QS> for Bound<T, U> where
73+
Bound<T, U>: Expression,
74+
{
75+
}
76+
7277
impl<T, U> NonAggregate for Bound<T, U> where
7378
Bound<T, U>: Expression,
7479
{

‎diesel/src/expression/coerce.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use std::marker::PhantomData;
22

33
use backend::Backend;
4-
use expression::{Expression, SelectableExpression, NonAggregate};
4+
use expression::*;
55
use query_builder::*;
66
use result::QueryResult;
77

@@ -44,6 +44,11 @@ impl<T, ST, QS> SelectableExpression<QS> for Coerce<T, ST> where
4444
type SqlTypeForSelect = Self::SqlType;
4545
}
4646

47+
impl<T, ST, QS> AppearsOnTable<QS> for Coerce<T, ST> where
48+
T: AppearsOnTable<QS>,
49+
{
50+
}
51+
4752
impl<T, ST, DB> QueryFragment<DB> for Coerce<T, ST> where
4853
T: QueryFragment<DB>,
4954
DB: Backend,

‎diesel/src/expression/count.rs

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use backend::Backend;
22
use query_builder::*;
33
use result::QueryResult;
4-
use super::{Expression, SelectableExpression};
4+
use super::Expression;
55
use types::BigInt;
66

77
/// Creates a SQL `COUNT` expression
@@ -78,10 +78,7 @@ impl<T: QueryFragment<DB>, DB: Backend> QueryFragment<DB> for Count<T> {
7878
}
7979

8080
impl_query_id!(Count<T>);
81-
82-
impl<T: Expression, QS> SelectableExpression<QS> for Count<T> {
83-
type SqlTypeForSelect = BigInt;
84-
}
81+
impl_selectable_expression!(Count<T>);
8582

8683
#[derive(Debug, Clone, Copy)]
8784
#[doc(hidden)]
@@ -106,8 +103,5 @@ impl<DB: Backend> QueryFragment<DB> for CountStar {
106103
}
107104
}
108105

109-
impl<QS> SelectableExpression<QS> for CountStar {
110-
type SqlTypeForSelect = BigInt;
111-
}
112-
113106
impl_query_id!(CountStar);
107+
impl_selectable_expression!(CountStar);

‎diesel/src/expression/exists.rs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use backend::Backend;
2-
use expression::{Expression, SelectableExpression, NonAggregate};
2+
use expression::{Expression, NonAggregate};
33
use query_builder::*;
44
use result::QueryResult;
55
use types::Bool;
@@ -49,12 +49,6 @@ impl<T> Expression for Exists<T> where
4949
type SqlType = Bool;
5050
}
5151

52-
impl<T, QS> SelectableExpression<QS> for Exists<T> where
53-
Exists<T>: Expression,
54-
{
55-
type SqlTypeForSelect = Bool;
56-
}
57-
5852
impl<T> NonAggregate for Exists<T> {
5953
}
6054

@@ -80,3 +74,4 @@ impl<T, DB> QueryFragment<DB> for Exists<T> where
8074
}
8175

8276
impl_query_id!(Exists<T>);
77+
impl_selectable_expression!(Exists<T>);

‎diesel/src/expression/functions/aggregate_folding.rs

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use backend::Backend;
2-
use expression::{Expression, SelectableExpression};
2+
use expression::Expression;
33
use query_builder::*;
44
use result::QueryResult;
55
use types::{Foldable, HasSqlType};
@@ -50,13 +50,7 @@ macro_rules! fold_function {
5050
}
5151

5252
impl_query_id!($type_name<T>);
53-
54-
impl<T, QS> SelectableExpression<QS> for $type_name<T> where
55-
$type_name<T>: Expression,
56-
T: SelectableExpression<QS>,
57-
{
58-
type SqlTypeForSelect = Self::SqlType;
59-
}
53+
impl_selectable_expression!($type_name<T>);
6054
}
6155
}
6256

‎diesel/src/expression/functions/aggregate_ordering.rs

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use backend::Backend;
2-
use expression::{Expression, SelectableExpression};
2+
use expression::Expression;
33
use query_builder::*;
44
use result::QueryResult;
55
use types::{SqlOrd, HasSqlType, IntoNullable};
@@ -49,13 +49,7 @@ macro_rules! ord_function {
4949
}
5050

5151
impl_query_id!($type_name<T>);
52-
53-
impl<T, QS> SelectableExpression<QS> for $type_name<T> where
54-
$type_name<T>: Expression,
55-
T: SelectableExpression<QS>,
56-
{
57-
type SqlTypeForSelect = Self::SqlType;
58-
}
52+
impl_selectable_expression!($type_name<T>);
5953
}
6054
}
6155

‎diesel/src/expression/functions/date_and_time.rs

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use backend::Backend;
2-
use expression::{Expression, SelectableExpression, NonAggregate};
2+
use expression::{Expression, NonAggregate};
33
use query_builder::*;
44
use result::QueryResult;
55
use types::*;
@@ -14,10 +14,6 @@ impl Expression for now {
1414
type SqlType = Timestamp;
1515
}
1616

17-
impl<QS> SelectableExpression<QS> for now {
18-
type SqlTypeForSelect = Timestamp;
19-
}
20-
2117
impl NonAggregate for now {
2218
}
2319

@@ -37,6 +33,7 @@ impl<DB: Backend> QueryFragment<DB> for now {
3733
}
3834

3935
impl_query_id!(now);
36+
impl_selectable_expression!(now);
4037

4138
operator_allowed!(now, Add, add);
4239
operator_allowed!(now, Sub, sub);

‎diesel/src/expression/functions/mod.rs

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,22 @@ macro_rules! sql_function_body {
6262

6363
#[allow(non_camel_case_types)]
6464
impl<$($arg_name),*, QS> $crate::expression::SelectableExpression<QS> for $struct_name<$($arg_name),*> where
65-
$($arg_name: $crate::expression::SelectableExpression<QS>,)*
66-
$struct_name<$($arg_name),*>: $crate::expression::Expression,
65+
$($arg_name: $crate::expression::SelectableExpression<
66+
QS,
67+
SqlTypeForSelect = <$arg_name as $crate::expression::Expression>::SqlType,
68+
>,)*
69+
$struct_name<$($arg_name),*>: $crate::expression::AppearsOnTable<QS>,
6770
{
6871
type SqlTypeForSelect = Self::SqlType;
6972
}
7073

74+
#[allow(non_camel_case_types)]
75+
impl<$($arg_name),*, QS> $crate::expression::AppearsOnTable<QS> for $struct_name<$($arg_name),*> where
76+
$($arg_name: $crate::expression::AppearsOnTable<QS>,)*
77+
$struct_name<$($arg_name),*>: $crate::expression::Expression,
78+
{
79+
}
80+
7181
#[allow(non_camel_case_types)]
7282
impl<$($arg_name),*> $crate::expression::NonAggregate for $struct_name<$($arg_name),*> where
7383
$($arg_name: $crate::expression::NonAggregate,)*
@@ -136,6 +146,9 @@ macro_rules! no_arg_sql_function_body_except_to_sql {
136146
type SqlTypeForSelect = $return_type;
137147
}
138148

149+
impl<QS> $crate::expression::AppearsOnTable<QS> for $type_name {
150+
}
151+
139152
impl $crate::expression::NonAggregate for $type_name {
140153
}
141154

‎diesel/src/expression/grouped.rs

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use backend::Backend;
2-
use expression::{Expression, SelectableExpression, NonAggregate};
2+
use expression::{Expression, NonAggregate};
33
use query_builder::*;
44
use result::QueryResult;
55

@@ -29,13 +29,7 @@ impl<T: QueryFragment<DB>, DB: Backend> QueryFragment<DB> for Grouped<T> {
2929
}
3030

3131
impl_query_id!(Grouped<T>);
32-
33-
impl<T, QS> SelectableExpression<QS> for Grouped<T> where
34-
T: SelectableExpression<QS>,
35-
Grouped<T>: Expression,
36-
{
37-
type SqlTypeForSelect = T::SqlTypeForSelect;
38-
}
32+
impl_selectable_expression!(Grouped<T>);
3933

4034
impl<T: NonAggregate> NonAggregate for Grouped<T> where
4135
Grouped<T>: Expression,

0 commit comments

Comments
 (0)