Skip to content

Commit e19afd5

Browse files
weiznichweiznich
authored
Merge pull request #5012 from ayarotsky/fix-aggregate-expressions-and-order-by
Disallow mixing aggregate and non-aggregate expressions in ORDER BY
2 parents 9320d82 + bc8cb1d commit e19afd5

File tree

5 files changed

+179
-0
lines changed

5 files changed

+179
-0
lines changed

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@ Increasing the minimal supported Rust version will always be coupled at least wi
1717
* Diesel-Migrations now contains a migration source that allows you to combine migrations from several different sources
1818
* Added `SqliteConnection::with_raw_connection` to provide safe, callback-based access to the raw `*mut sqlite3` handle for advanced SQLite C APIs (session extension, hooks, etc.)
1919

20+
### Fixed
21+
22+
* Raise a compile-time error when mixing aggregate and non-aggregate expressions in an `ORDER BY` clause without a `GROUP BY` clause
23+
2024
### Changed
2125

2226
* The minimal supported Rust version is now 1.88.0

‎diesel/src/query_builder/select_statement/dsl_impls.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,12 @@ where
274274
SelectStatement<FromClause<F>, S, D, W, OrderClause<Expr>, LOf, G, H, LC>:
275275
SelectQuery<SqlType = ST>,
276276
OrderClause<Expr>: ValidOrderingForDistinct<D>,
277+
G: ValidGroupByClause,
278+
S: SelectClauseExpression<FromClause<F>>,
279+
Expr: ValidGrouping<G::Expressions>,
280+
S::Selection: ValidGrouping<G::Expressions>,
281+
<S::Selection as ValidGrouping<G::Expressions>>::IsAggregate:
282+
MixedAggregates<<Expr as ValidGrouping<G::Expressions>>::IsAggregate>,
277283
{
278284
type Output = SelectStatement<FromClause<F>, S, D, W, OrderClause<Expr>, LOf, G, H, LC>;
279285

@@ -298,6 +304,12 @@ impl<F, S, D, W, O, LOf, G, H, LC, Expr> ThenOrderDsl<Expr>
298304
where
299305
F: QuerySource,
300306
Expr: AppearsOnTable<F>,
307+
G: ValidGroupByClause,
308+
S: SelectClauseExpression<FromClause<F>>,
309+
Expr: ValidGrouping<G::Expressions>,
310+
S::Selection: ValidGrouping<G::Expressions>,
311+
<S::Selection as ValidGrouping<G::Expressions>>::IsAggregate:
312+
MixedAggregates<<Expr as ValidGrouping<G::Expressions>>::IsAggregate>,
301313
{
302314
type Output = SelectStatement<FromClause<F>, S, D, W, OrderClause<(O, Expr)>, LOf, G, H, LC>;
303315

‎diesel_compile_tests/tests/fail/cannot_mix_aggregate_and_non_aggregate_in_order_by.rs‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
extern crate diesel;
2+
3+
use diesel::*;
4+
5+
table! {
6+
users {
7+
id -> Integer,
8+
name -> Text,
9+
}
10+
}
11+
12+
fn main() {
13+
use self::users::dsl::*;
14+
use diesel::dsl::max;
15+
16+
// aggregate SELECT + non-aggregate ORDER BY (the issue's example)
17+
let _ = users.select(max(id)).order_by(name);
18+
//~^ ERROR: mixing aggregate and not aggregate expressions is not allowed in SQL
19+
20+
// non-aggregate SELECT + aggregate ORDER BY (also invalid SQL)
21+
let _ = users.select(id).order_by(max(id));
22+
//~^ ERROR: mixing aggregate and not aggregate expressions is not allowed in SQL
23+
24+
// aggregate SELECT + non-aggregate then_order_by
25+
let _ = users
26+
.select(max(id))
27+
.order_by(max(id))
28+
.then_order_by(name);
29+
//~^ ERROR: mixing aggregate and not aggregate expressions is not allowed in SQL
30+
}

‎diesel_compile_tests/tests/fail/cannot_mix_aggregate_and_non_aggregate_in_order_by.stderr‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
error[E0277]: mixing aggregate and not aggregate expressions is not allowed in SQL
2+
--> tests/fail/cannot_mix_aggregate_and_non_aggregate_in_order_by.rs:17:44
3+
|
4+
LL | let _ = users.select(max(id)).order_by(name);
5+
| -------- ^^^^ unsatisfied trait bound
6+
| |
7+
| required by a bound introduced by this call
8+
|
9+
= help: the trait `MixedAggregates<diesel::expression::is_aggregate::No>` is not implemented for `diesel::expression::is_aggregate::Yes`
10+
= note: you tried to combine expressions that aggregate over a certain column with expressions that don't aggregate over that column
11+
= note: try to either use aggregate functions like `min`/`max`/… for this column or add the column to your `GROUP BY` clause
12+
= note: also there are clauses like `WHERE` or `RETURNING` that does not accept aggregate expressions at all
13+
help: the following other types implement trait `MixedAggregates<Other>`
14+
--> DIESEL/diesel/diesel/src/expression/mod.rs
15+
|
16+
LL | impl MixedAggregates<Yes> for Yes {
17+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `diesel::expression::is_aggregate::Yes` implements `MixedAggregates<diesel::expression::is_aggregate::Yes>`
18+
...
19+
LL | impl MixedAggregates<Never> for Yes {
20+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `diesel::expression::is_aggregate::Yes` implements `MixedAggregates<diesel::expression::is_aggregate::Never>`
21+
= note: required for `SelectStatement<FromClause<table>, ...>` to implement `OrderDsl<columns::name>`
22+
note: required by a bound in `order_by`
23+
--> DIESEL/diesel/diesel/src/query_dsl/mod.rs
24+
|
25+
LL | fn order_by<Expr>(self, expr: Expr) -> OrderBy<Self, Expr>
26+
| -------- required by a bound in this associated function
27+
...
28+
LL | Self: methods::OrderDsl<Expr>,
29+
| ^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `QueryDsl::order_by`
30+
31+
32+
error[E0277]: mixing aggregate and not aggregate expressions is not allowed in SQL
33+
--> tests/fail/cannot_mix_aggregate_and_non_aggregate_in_order_by.rs:21:39
34+
|
35+
LL | let _ = users.select(id).order_by(max(id));
36+
| -------- ^^^^^^^ unsatisfied trait bound
37+
| |
38+
| required by a bound introduced by this call
39+
|
40+
= help: the trait `MixedAggregates<diesel::expression::is_aggregate::Yes>` is not implemented for `diesel::expression::is_aggregate::No`
41+
= note: you tried to combine expressions that aggregate over a certain column with expressions that don't aggregate over that column
42+
= note: try to either use aggregate functions like `min`/`max`/… for this column or add the column to your `GROUP BY` clause
43+
= note: also there are clauses like `WHERE` or `RETURNING` that does not accept aggregate expressions at all
44+
help: the following other types implement trait `MixedAggregates<Other>`
45+
--> DIESEL/diesel/diesel/src/expression/mod.rs
46+
|
47+
LL | impl MixedAggregates<No> for No {
48+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `diesel::expression::is_aggregate::No` implements `MixedAggregates<diesel::expression::is_aggregate::No>`
49+
...
50+
LL | impl MixedAggregates<Never> for No {
51+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `diesel::expression::is_aggregate::No` implements `MixedAggregates<diesel::expression::is_aggregate::Never>`
52+
= note: required for `SelectStatement<FromClause<table>, ...>` to implement `OrderDsl<max<Integer, id>>`
53+
note: required by a bound in `order_by`
54+
--> DIESEL/diesel/diesel/src/query_dsl/mod.rs
55+
|
56+
LL | fn order_by<Expr>(self, expr: Expr) -> OrderBy<Self, Expr>
57+
| -------- required by a bound in this associated function
58+
...
59+
LL | Self: methods::OrderDsl<Expr>,
60+
| ^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `QueryDsl::order_by`
61+
62+
63+
error[E0277]: mixing aggregate and not aggregate expressions is not allowed in SQL
64+
--> tests/fail/cannot_mix_aggregate_and_non_aggregate_in_order_by.rs:28:24
65+
|
66+
LL | .then_order_by(name);
67+
| ------------- ^^^^ unsatisfied trait bound
68+
| |
69+
| required by a bound introduced by this call
70+
|
71+
= help: the trait `MixedAggregates<diesel::expression::is_aggregate::No>` is not implemented for `diesel::expression::is_aggregate::Yes`
72+
= note: you tried to combine expressions that aggregate over a certain column with expressions that don't aggregate over that column
73+
= note: try to either use aggregate functions like `min`/`max`/… for this column or add the column to your `GROUP BY` clause
74+
= note: also there are clauses like `WHERE` or `RETURNING` that does not accept aggregate expressions at all
75+
help: the following other types implement trait `MixedAggregates<Other>`
76+
--> DIESEL/diesel/diesel/src/expression/mod.rs
77+
|
78+
LL | impl MixedAggregates<Yes> for Yes {
79+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `diesel::expression::is_aggregate::Yes` implements `MixedAggregates<diesel::expression::is_aggregate::Yes>`
80+
...
81+
LL | impl MixedAggregates<Never> for Yes {
82+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `diesel::expression::is_aggregate::Yes` implements `MixedAggregates<diesel::expression::is_aggregate::Never>`
83+
= note: required for `SelectStatement<..., ..., ..., ..., ...>` to implement `ThenOrderDsl<columns::name>`
84+
note: required by a bound in `diesel::QueryDsl::then_order_by`
85+
--> DIESEL/diesel/diesel/src/query_dsl/mod.rs
86+
|
87+
LL | fn then_order_by<Order>(self, order: Order) -> ThenOrderBy<Self, Order>
88+
| ------------- required by a bound in this associated function
89+
LL | where
90+
LL | Self: methods::ThenOrderDsl<Order>,
91+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `QueryDsl::then_order_by`
92+
93+
For more information about this error, try `rustc --explain E0277`.

‎diesel_tests/tests/aggregate_expressions.rs‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use crate::schema::connection_with_sean_and_tess_in_users_table;
22
use crate::schema::posts;
3+
use crate::schema::users;
34
use diesel::dsl;
45
use diesel::prelude::*;
56

@@ -72,3 +73,42 @@ fn order() {
7273

7374
assert_eq!(res, 2);
7475
}
76+
77+
#[diesel_test_helper::test]
78+
fn order_by_aggregate_with_aggregate_select() {
79+
let mut conn = connection_with_sean_and_tess_in_users_table();
80+
let res = users::table
81+
.select(dsl::max(users::id))
82+
.order_by(dsl::max(users::id))
83+
.get_result::<Option<i32>>(&mut conn)
84+
.unwrap();
85+
assert_eq!(res, Some(2));
86+
}
87+
88+
#[diesel_test_helper::test]
89+
fn order_by_group_by_column_with_aggregate_select() {
90+
let mut conn = connection_with_sean_and_tess_in_users_table();
91+
let mut res = users::table
92+
.group_by(users::name)
93+
.select((users::name, dsl::max(users::id)))
94+
.order_by(users::name)
95+
.load::<(String, Option<i32>)>(&mut conn)
96+
.unwrap();
97+
res.sort();
98+
assert_eq!(
99+
res,
100+
vec![("Sean".to_string(), Some(1)), ("Tess".to_string(), Some(2)),]
101+
);
102+
}
103+
104+
#[diesel_test_helper::test]
105+
fn then_order_by_aggregate() {
106+
let mut conn = connection_with_sean_and_tess_in_users_table();
107+
let res = users::table
108+
.select(dsl::max(users::id))
109+
.order_by(dsl::max(users::id))
110+
.then_order_by(dsl::count_star())
111+
.get_result::<Option<i32>>(&mut conn)
112+
.unwrap();
113+
assert_eq!(res, Some(2));
114+
}

0 commit comments

Comments
 (0)