Skip to content

Commit

Permalink
Fix issues with allowing invalid queries
Browse files Browse the repository at this point in the history
  • Loading branch information
@weiznich
weiznich committed Aug 23, 2022
1 parent d88e389 commit feb6ea1
Show file tree
Hide file tree
Showing 9 changed files with 116 additions and 136 deletions.
139 changes: 77 additions & 62 deletions diesel/src/query_source/joins.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use crate::backend::DieselReserveSpecialization;
use crate::expression::grouped::Grouped;
use crate::expression::nullable::Nullable;
use crate::expression::SelectableExpression;
use crate::expression::ValidGrouping;
use crate::prelude::*;
use crate::query_builder::*;
use crate::query_dsl::InternalJoinDsl;
Expand Down Expand Up @@ -103,67 +102,12 @@ where
}
}

// TODO: figure out where to put this thing
// and make really sure that it cannot leak into the
// public api at all
#[derive(Debug, QueryId, Copy, Clone)]
pub struct SkipSelectableExpressionWrapper<T>(T);

impl<DB, T> QueryFragment<DB> for SkipSelectableExpressionWrapper<T>
where
T: QueryFragment<DB>,
DB: Backend,
{
fn walk_ast<'b>(&'b self, pass: AstPass<'_, 'b, DB>) -> QueryResult<()> {
self.0.walk_ast(pass)
}
}

impl<GB, T> ValidGrouping<GB> for SkipSelectableExpressionWrapper<T>
where
T: ValidGrouping<GB>,
{
type IsAggregate = T::IsAggregate;
}

impl<QS, T> SelectClauseExpression<QS> for SkipSelectableExpressionWrapper<T>
where
T: SelectClauseExpression<QS>,
{
type Selection = T::Selection;

type SelectClauseSqlType = T::SelectClauseSqlType;
}

impl<QS, T> SelectableExpression<QS> for SkipSelectableExpressionWrapper<T> where
Self: AppearsOnTable<QS>
{
}
impl<QS, T> AppearsOnTable<QS> for SkipSelectableExpressionWrapper<T> where Self: Expression {}
impl<T> Expression for SkipSelectableExpressionWrapper<T>
where
T: Expression,
{
type SqlType = T::SqlType;
}

impl<T, Selection> TupleAppend<Selection> for SkipSelectableExpressionWrapper<T>
where
T: TupleAppend<Selection>,
{
// We're re-wrapping after anyway
type Output = T::Output;

fn tuple_append(self, right: Selection) -> Self::Output {
self.0.tuple_append(right)
}
}

impl<Left, Right> QuerySource for Join<Left, Right, Inner>
where
Left: QuerySource + AppendSelection<Right::DefaultSelection>,
Right: QuerySource,
<Left as AppendSelection<Right::DefaultSelection>>::Output: Expression,
Left::Output: AppearsOnTable<Self>,
Self: Clone,
{
type FromClause = Self;
Expand All @@ -172,14 +116,16 @@ where
// again. These checked turned out to be quite expensive in terms of compile time
// so we use a wrapper type to just skip the check and forward other more relevant
// trait implementations to the inner type
type DefaultSelection = SkipSelectableExpressionWrapper<Left::Output>;
//
// See https://github.com/diesel-rs/diesel/issues/3223 for details
type DefaultSelection = self::private::SkipSelectableExpressionWrapper<Left::Output>;

fn from_clause(&self) -> Self::FromClause {
self.clone()
}

fn default_selection(&self) -> Self::DefaultSelection {
SkipSelectableExpressionWrapper(
self::private::SkipSelectableExpressionWrapper(
self.left
.source
.append_selection(self.right.source.default_selection()),
Expand All @@ -191,7 +137,7 @@ impl<Left, Right> QuerySource for Join<Left, Right, LeftOuter>
where
Left: QuerySource + AppendSelection<Nullable<Right::DefaultSelection>>,
Right: QuerySource,
<Left as AppendSelection<Nullable<Right::DefaultSelection>>>::Output: Expression,
Left::Output: AppearsOnTable<Self>,
Self: Clone,
{
type FromClause = Self;
Expand All @@ -200,14 +146,16 @@ where
// again. These checked turned out to be quite expensive in terms of compile time
// so we use a wrapper type to just skip the check and forward other more relevant
// trait implementations to the inner type
type DefaultSelection = SkipSelectableExpressionWrapper<Left::Output>;
//
// See https://github.com/diesel-rs/diesel/issues/3223 for details
type DefaultSelection = self::private::SkipSelectableExpressionWrapper<Left::Output>;

fn from_clause(&self) -> Self::FromClause {
self.clone()
}

fn default_selection(&self) -> Self::DefaultSelection {
SkipSelectableExpressionWrapper(
self::private::SkipSelectableExpressionWrapper(
self.left
.source
.append_selection(self.right.source.default_selection().nullable()),
Expand Down Expand Up @@ -493,3 +441,70 @@ where
impl<T: Table> ToInnerJoin for T {
type InnerJoin = T;
}

mod private {
use crate::backend::Backend;
use crate::expression::{Expression, ValidGrouping};
use crate::query_builder::{AstPass, QueryFragment, SelectClauseExpression};
use crate::{AppearsOnTable, QueryResult, SelectableExpression};

#[derive(Debug, crate::query_builder::QueryId, Copy, Clone)]
pub struct SkipSelectableExpressionWrapper<T>(pub(super) T);

impl<DB, T> QueryFragment<DB> for SkipSelectableExpressionWrapper<T>
where
T: QueryFragment<DB>,
DB: Backend,
{
fn walk_ast<'b>(&'b self, pass: AstPass<'_, 'b, DB>) -> QueryResult<()> {
self.0.walk_ast(pass)
}
}

// The default select clause is only valid for no group by clause
// anyway so we can just skip the recursive check here
impl<T> ValidGrouping<()> for SkipSelectableExpressionWrapper<T> {
type IsAggregate = crate::expression::is_aggregate::No;
}

// This needs to use the expression impl
impl<QS, T> SelectClauseExpression<QS> for SkipSelectableExpressionWrapper<T>
where
T: SelectClauseExpression<QS>,
{
type Selection = T::Selection;

type SelectClauseSqlType = T::SelectClauseSqlType;
}

// The default select clause for joins is always valid assuming that
// the default select clause of all involved query sources is
// valid too. We can skip the recursive check here.
impl<QS, T> SelectableExpression<QS> for SkipSelectableExpressionWrapper<T> where
Self: AppearsOnTable<QS>
{
}

impl<QS, T> AppearsOnTable<QS> for SkipSelectableExpressionWrapper<T> where Self: Expression {}

// Expression must recurse the whole expression
// as this is required for the return type of the query
impl<T> Expression for SkipSelectableExpressionWrapper<T>
where
T: Expression,
{
type SqlType = T::SqlType;
}

impl<T, Selection> crate::util::TupleAppend<Selection> for SkipSelectableExpressionWrapper<T>
where
T: crate::util::TupleAppend<Selection>,
{
// We're re-wrapping after anyway
type Output = T::Output;

fn tuple_append(self, right: Selection) -> Self::Output {
self.0.tuple_append(right)
}
}
}
16 changes: 8 additions & 8 deletions diesel_compile_tests/tests/fail/array_expressions_must_be_correct_type.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ error[E0277]: Cannot select `f64` from `NoFromClause`
<(T0, T1, T2, T3, T4, T5) as SelectableExpression<QS>>
<(T0, T1, T2, T3, T4, T5, T6) as SelectableExpression<QS>>
<(T0, T1, T2, T3, T4, T5, T6, T7) as SelectableExpression<QS>>
and 154 others
and 155 others
= note: required because of the requirements on the impl of `SelectableExpression<NoFromClause>` for `(f64, f64)`
= note: 1 redundant requirement hidden
= note: required because of the requirements on the impl of `SelectableExpression<NoFromClause>` for `diesel::pg::expression::array::ArrayLiteral<(f64, f64), diesel::sql_types::Integer>`
Expand Down Expand Up @@ -42,7 +42,7 @@ error[E0277]: the trait bound `f64: ValidGrouping<()>` is not satisfied
<(T0, T1, T2, T3, T4, T5) as ValidGrouping<__GroupByClause>>
<(T0, T1, T2, T3, T4, T5, T6) as ValidGrouping<__GroupByClause>>
<(T0, T1, T2, T3, T4, T5, T6, T7) as ValidGrouping<__GroupByClause>>
and 139 others
and 140 others
= note: required because of the requirements on the impl of `ValidGrouping<()>` for `(f64, f64)`
= note: 1 redundant requirement hidden
= note: required because of the requirements on the impl of `ValidGrouping<()>` for `diesel::pg::expression::array::ArrayLiteral<(f64, f64), diesel::sql_types::Integer>`
Expand Down Expand Up @@ -70,7 +70,7 @@ error[E0277]: Cannot select `f64` from `NoFromClause`
<(T0, T1, T2, T3, T4, T5) as SelectableExpression<QS>>
<(T0, T1, T2, T3, T4, T5, T6) as SelectableExpression<QS>>
<(T0, T1, T2, T3, T4, T5, T6, T7) as SelectableExpression<QS>>
and 154 others
and 155 others
= note: required because of the requirements on the impl of `SelectableExpression<NoFromClause>` for `(f64, f64)`
= note: 1 redundant requirement hidden
= note: required because of the requirements on the impl of `SelectableExpression<NoFromClause>` for `diesel::pg::expression::array::ArrayLiteral<(f64, f64), diesel::sql_types::Integer>`
Expand Down Expand Up @@ -98,7 +98,7 @@ error[E0277]: the trait bound `f64: ValidGrouping<()>` is not satisfied
<(T0, T1, T2, T3, T4, T5) as ValidGrouping<__GroupByClause>>
<(T0, T1, T2, T3, T4, T5, T6) as ValidGrouping<__GroupByClause>>
<(T0, T1, T2, T3, T4, T5, T6, T7) as ValidGrouping<__GroupByClause>>
and 139 others
and 140 others
= note: required because of the requirements on the impl of `ValidGrouping<()>` for `(f64, f64)`
= note: 1 redundant requirement hidden
= note: required because of the requirements on the impl of `ValidGrouping<()>` for `diesel::pg::expression::array::ArrayLiteral<(f64, f64), diesel::sql_types::Integer>`
Expand All @@ -125,7 +125,7 @@ error[E0277]: the trait bound `f64: QueryId` is not satisfied
(T0, T1, T2, T3, T4)
(T0, T1, T2, T3, T4, T5)
(T0, T1, T2, T3, T4, T5, T6)
and 228 others
and 229 others
= note: required because of the requirements on the impl of `QueryId` for `(f64, f64)`
= note: 3 redundant requirements hidden
= note: required because of the requirements on the impl of `QueryId` for `SelectStatement<NoFromClause, diesel::query_builder::select_clause::SelectClause<diesel::pg::expression::array::ArrayLiteral<(f64, f64), diesel::sql_types::Integer>>>`
Expand All @@ -151,7 +151,7 @@ error[E0277]: the trait bound `f64: QueryFragment<Pg>` is not satisfied
<(T0, T1, T2, T3, T4) as QueryFragment<__DB>>
<(T0, T1, T2, T3, T4, T5) as QueryFragment<__DB>>
<(T0, T1, T2, T3, T4, T5, T6) as QueryFragment<__DB>>
and 267 others
and 268 others
= note: required because of the requirements on the impl of `QueryFragment<Pg>` for `(f64, f64)`
= note: 4 redundant requirements hidden
= note: required because of the requirements on the impl of `QueryFragment<Pg>` for `SelectStatement<NoFromClause, diesel::query_builder::select_clause::SelectClause<diesel::pg::expression::array::ArrayLiteral<(f64, f64), diesel::sql_types::Integer>>>`
Expand All @@ -177,7 +177,7 @@ error[E0277]: the trait bound `f64: diesel::Expression` is not satisfied
(T0, T1, T2, T3, T4, T5)
(T0, T1, T2, T3, T4, T5, T6)
(T0, T1, T2, T3, T4, T5, T6, T7)
and 119 others
and 120 others
= note: required because of the requirements on the impl of `AsExpression<diesel::sql_types::Integer>` for `f64`
= note: required because of the requirements on the impl of `AsExpressionList<diesel::sql_types::Integer>` for `(f64, f64)`
note: required by a bound in `diesel::dsl::array`
Expand All @@ -201,5 +201,5 @@ error[E0277]: the trait bound `f64: diesel::Expression` is not satisfied
(T0, T1, T2, T3, T4, T5)
(T0, T1, T2, T3, T4, T5, T6)
(T0, T1, T2, T3, T4, T5, T6, T7)
and 119 others
and 120 others
= note: required because of the requirements on the impl of `AsExpression<diesel::sql_types::Integer>` for `f64`
Loading

0 comments on commit feb6ea1

Please sign in to comment.