Merge pull request #2251 from diesel-rs/sg-fix-non-aggregate

Replace `NonAggregate` with something less broken

.travis.yml

@@ -19,9 +19,9 @@ before_install:
   - |
     if [[ "$BACKEND" == sqlite ]]; then
       (sudo apt-get update) &&
-      (wget --quiet -c https://www.sqlite.org/2018/sqlite-autoconf-3240000.tar.gz) &&
-      (tar zxf sqlite-autoconf-3240000.tar.gz;) &&
-      (cd sqlite-autoconf-3240000; \
+      (wget --quiet -c https://sqlite.org/2020/sqlite-autoconf-3310100.tar.gz) &&
+      (tar zxf sqlite-autoconf-3310100.tar.gz;) &&
+      (cd sqlite-autoconf-3310100; \
        CFLAGS="$CFLAGS -O2 -fno-strict-aliasing \
                    -DSQLITE_DEFAULT_FOREIGN_KEYS=1 \
                    -DSQLITE_SECURE_DELETE \
@@ -48,7 +48,7 @@ before_install:
                    --enable-dynamic-extensions \
                    --libdir=/usr/lib/x86_64-linux-gnu \
                    --libexecdir=/usr/lib/x86_64-linux-gnu/sqlite3) &&
-      (cd sqlite-autoconf-3240000; sudo make; sudo make install)
+      (cd sqlite-autoconf-3310100; sudo make; sudo make install)
     fi
 before_script:
   - pip install 'travis-cargo<0.2' --user
@@ -100,7 +100,7 @@ matrix:
     script:
     - (cd diesel_cli && cargo test --no-default-features --features "sqlite-bundled")
   - rust: 1.40.0
-      name: "Minimal supported rust version == 1.40.0"
+    name: "Minimal supported rust version == 1.40.0"
     script:
     - cargo check --all
 

CHANGELOG.md

@@ -8,8 +8,6 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
 
 ### Added
 
-* `NonAggregate` can now be derived for simple cases.
-
 * `Connection` and `SimpleConnection` traits are implemented for a broader range
   of `r2d2::PooledConnection<M>` types when the `r2d2` feature is enabled.
 
@@ -30,10 +28,18 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
 * Added support for SQLite's `UPSERT`.
   You can use this feature above SQLite version 3.24.0.
 
+* Multiple aggregate expressions can now appear together in the same select
+  clause. See [the upgrade notes](#2-0-0-upgrade-non-aggregate) for details.
+
+* `ValidGrouping` has been added to represent whether an expression is valid for
+  a given group by clause, and whether or not it's aggregate. It replaces the
+  functionality of `NonAggregate`. See [the upgrade
+  notes](#2-0-0-upgrade-non-aggregate) for details.
+
 ### Removed
 
 * All previously deprecated items have been removed.
-* Support for uuid version < 0.7.0 has been removed
+* Support for uuid version < 0.7.0 has been removed.
 
 ### Changed
 
@@ -63,6 +69,20 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
 
 * Boxed queries (constructed from `.into_boxed()`) are now `Send`.
 
+* The handling of mixed aggregate values is more robust. Invalid queries such as
+  `.select(max(id) + other_column)` are now correctly rejected, and valid
+  queries such as `.select((count_star(), max(other_column)))` are now correctly
+  accepted. For more details, see [the upgrade notes](#2-0-0-upgrade-non-aggregate).
+
+* `NonAggregate` is now a trait alias for `ValidGrouping<()>` for expressions
+  that are not aggregate. On stable this is a normal trait with a blanket impl,
+  but it should never be implemented directly. With the `unstable` feature, it
+  will use trait aliases which prevent manual implementations.
+
+  Due to language limitations, we cannot make the new trait alias by itself
+  represent everything it used to, so in some rare cases code changes may be
+  required. See [the upgrade notes](#2-0-0-upgrade-non-aggregate) for details.
+
 ### Fixed
 
 * Many types were incorrectly considered non-aggregate when they should not
@@ -93,6 +113,38 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
   Please use `diesel::upsert` instead.
 
 
+### Upgrade Notes
+
+#### Replacement of `NonAggregate` with `ValidGrouping`
+<a name="2-0-0-upgrade-non-aggregate"></a>
+
+FIXME: This should probably be on the website, but I wanted to document it in
+the PR adding the changes.
+
+Key points:
+
+- Rules for aggregation are now correctly enforced. They match the semantics of
+  PG or MySQL with `ONLY_FULL_GROUP_BY` enabled.
+  - As before, `sql` is the escape hatch if needed.
+  - MySQL users can use `ANY_VALUE`, PG users can use `DISTINCT ON`. Also
+    consider using max/min/etc to get deterministic values.
+- Any `impl NonAggregate` must be replaced with `impl ValidGrouping`
+- For most code, `T: NonAggregate` should continue to work. Unless you're
+  getting a compiler error, you most likely don't need to change it.
+- The full equivalent of what `T: NonAggregate` used to mean is:
+
+      where
+          T: ValidGrouping<()>,
+          T::IsAggregate: MixedGrouping<is_aggregate::No, Output = is_aggregate::No>,
+          is_aggreagte::No: MixedGrouping<T::IsAggregate, Output = is_aggreagte::No>,
+
+- With `feature = "unstable"`, `T: NonAggregate` implies the first two bounds,
+  but not the third. On stable only the first bound is implied. This is a
+  language limitation.
+- `T: NonAggregate` can still be passed everywhere it could before, but `T:
+  NonAggregate` no longer implies `(OtherType, T): NonAggregate`.
+  - With `feature = "unstable"`, `(T, OtherType): NonAggregate` is still implied.
+
 [2-0-migration]: FIXME write a migration guide
 
 ## [1.4.4] - 2020-03-22

diesel/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "diesel"
-version = "1.4.4"
+version = "2.0.0"
 authors = ["Sean Griffin <[email protected]>"]
 license = "MIT OR Apache-2.0"
 description = "A safe, extensible ORM and Query Builder for PostgreSQL, SQLite, and MySQL"
@@ -34,7 +34,7 @@ bitflags = { version = "1.0", optional = true }
 r2d2 = { version = ">= 0.8, < 0.9", optional = true }
 
 [dependencies.diesel_derives]
-version = "~1.4.0"
+version = "~2.0.0"
 path = "../diesel_derives"
 
 [dev-dependencies]

diesel/src/expression/array_comparison.rs

@@ -5,13 +5,13 @@ use crate::query_builder::*;
 use crate::result::QueryResult;
 use crate::sql_types::Bool;
 
-#[derive(Debug, Copy, Clone, QueryId, NonAggregate)]
+#[derive(Debug, Copy, Clone, QueryId, ValidGrouping)]
 pub struct In<T, U> {
     left: T,
     values: U,
 }
 
-#[derive(Debug, Copy, Clone, QueryId, NonAggregate)]
+#[derive(Debug, Copy, Clone, QueryId, ValidGrouping)]
 pub struct NotIn<T, U> {
     left: T,
     values: U,
@@ -140,7 +140,7 @@ where
     }
 }
 
-#[derive(Debug, Clone, NonAggregate)]
+#[derive(Debug, Clone, ValidGrouping)]
 pub struct Many<T>(Vec<T>);
 
 impl<T: Expression> Expression for Many<T> {

diesel/src/expression/bound.rs

@@ -5,7 +5,7 @@ use crate::backend::Backend;
 use crate::query_builder::*;
 use crate::result::QueryResult;
 use crate::serialize::ToSql;
-use crate::sql_types::HasSqlType;
+use crate::sql_types::{DieselNumericOps, HasSqlType};
 
 #[derive(Debug, Clone, Copy, DieselNumericOps)]
 pub struct Bound<T, U> {
@@ -47,4 +47,6 @@ impl<T, U, QS> SelectableExpression<QS> for Bound<T, U> where Bound<T, U>: Appea
 
 impl<T, U, QS> AppearsOnTable<QS> for Bound<T, U> where Bound<T, U>: Expression {}
 
-impl<T, U> NonAggregate for Bound<T, U> {}
+impl<T, U, GB> ValidGrouping<GB> for Bound<T, U> {
+    type IsAggregate = is_aggregate::Never;
+}

diesel/src/expression/coerce.rs

@@ -4,6 +4,7 @@ use crate::backend::Backend;
 use crate::expression::*;
 use crate::query_builder::*;
 use crate::result::QueryResult;
+use crate::sql_types::DieselNumericOps;
 
 #[derive(Debug, Copy, Clone, QueryId, DieselNumericOps)]
 #[doc(hidden)]
@@ -53,4 +54,9 @@ where
     }
 }
 
-impl<T, ST> NonAggregate for Coerce<T, ST> where T: NonAggregate {}
+impl<T, ST, GB> ValidGrouping<GB> for Coerce<T, ST>
+where
+    T: ValidGrouping<GB>,
+{
+    type IsAggregate = T::IsAggregate;
+}

diesel/src/expression/count.rs

@@ -1,9 +1,9 @@
 use super::functions::sql_function;
-use super::Expression;
+use super::{Expression, ValidGrouping};
 use crate::backend::Backend;
 use crate::query_builder::*;
 use crate::result::QueryResult;
-use crate::sql_types::BigInt;
+use crate::sql_types::{BigInt, DieselNumericOps};
 
 sql_function! {
     /// Creates a SQL `COUNT` expression
@@ -54,7 +54,8 @@ pub fn count_star() -> CountStar {
     CountStar
 }
 
-#[derive(Debug, Clone, Copy, QueryId, DieselNumericOps)]
+#[derive(Debug, Clone, Copy, QueryId, DieselNumericOps, ValidGrouping)]
+#[diesel(aggregate)]
 #[doc(hidden)]
 pub struct CountStar;
 

diesel/src/expression/exists.rs

@@ -1,6 +1,6 @@
 use crate::backend::Backend;
 use crate::expression::subselect::Subselect;
-use crate::expression::{AppearsOnTable, Expression, NonAggregate, SelectableExpression};
+use crate::expression::{AppearsOnTable, Expression, SelectableExpression, ValidGrouping};
 use crate::query_builder::*;
 use crate::result::QueryResult;
 use crate::sql_types::Bool;
@@ -42,7 +42,12 @@ where
     type SqlType = Bool;
 }
 
-impl<T> NonAggregate for Exists<T> where Subselect<T, ()>: NonAggregate {}
+impl<T, GB> ValidGrouping<GB> for Exists<T>
+where
+    Subselect<T, ()>: ValidGrouping<GB>,
+{
+    type IsAggregate = <Subselect<T, ()> as ValidGrouping<GB>>::IsAggregate;
+}
 
 #[cfg(not(feature = "unstable"))]
 impl<T, DB> QueryFragment<DB> for Exists<T>

diesel/src/expression/functions/date_and_time.rs

@@ -1,15 +1,15 @@
 use crate::backend::Backend;
 use crate::expression::coerce::Coerce;
 use crate::expression::functions::sql_function;
-use crate::expression::{AsExpression, Expression};
+use crate::expression::{AsExpression, Expression, ValidGrouping};
 use crate::query_builder::*;
 use crate::result::QueryResult;
 use crate::sql_types::*;
 
 /// Represents the SQL `CURRENT_TIMESTAMP` constant. This is equivalent to the
 /// `NOW()` function on backends that support it.
 #[allow(non_camel_case_types)]
-#[derive(Debug, Copy, Clone, QueryId, NonAggregate)]
+#[derive(Debug, Copy, Clone, QueryId, ValidGrouping)]
 pub struct now;
 
 impl Expression for now {

diesel/src/expression/functions/mod.rs

@@ -9,7 +9,13 @@ macro_rules! no_arg_sql_function_body_except_to_sql {
     ($type_name:ident, $return_type:ty, $docs:expr) => {
         #[allow(non_camel_case_types)]
         #[doc=$docs]
-        #[derive(Debug, Clone, Copy, $crate::query_builder::QueryId, $crate::expression::NonAggregate)]
+        #[derive(
+            Debug,
+            Clone,
+            Copy,
+            $crate::query_builder::QueryId,
+            $crate::expression::ValidGrouping
+        )]
         pub struct $type_name;
 
         impl $crate::expression::Expression for $type_name {

diesel/src/expression/grouped.rs

@@ -1,9 +1,10 @@
 use crate::backend::Backend;
-use crate::expression::Expression;
+use crate::expression::{Expression, ValidGrouping};
 use crate::query_builder::*;
 use crate::result::QueryResult;
+use crate::sql_types::DieselNumericOps;
 
-#[derive(Debug, Copy, Clone, QueryId, Default, DieselNumericOps, NonAggregate)]
+#[derive(Debug, Copy, Clone, QueryId, Default, DieselNumericOps, ValidGrouping)]
 pub struct Grouped<T>(pub T);
 
 impl<T: Expression> Expression for Grouped<T> {