A less boilerplaty way of specifying a boxed `ORDER BY` with multiple clauses?

[blakepettersson]

Problem Description

I have a use case where I want to dynamically set an order by clause via some request parameters. I have an example entity below, Todo:

infer_schema!("dotenv:POSTGRES_URL");

#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, Queryable, Insertable, AsChangeset)]
#[table_name = "todos"]
#[changeset_options(treat_none_as_null = "true")]
pub struct Todo {
    id: Uuid,
    title: String,
    body: Option<String>,
    published: bool
}

Doing a boxed query works fine:

use std::env;
use dotenv::dotenv;
use diesel::pg::PgConnection;
use self::todos::dsl::todos as table;
use self::todos as column;

pub enum MyErr {
    Diesel(diesel::result::Error),
    TooManySortColumns(String),
    InvalidParam(String)
}

pub fn establish_connection() -> PgConnection {
    dotenv().ok();

    let database_url = env::var("POSTGRES_URL").expect("POSTGRES_URL must be set");
    PgConnection::establish(&database_url)
        .expect(&format!("Error connecting to {}", database_url))
}

fn test(order_by: Vec<&str>) -> Result<Vec<Todo>, MyErr> {
    let mut query = table.into_boxed();
    let mut order_columns: Vec<Box<BoxableExpression<table, Pg, SqlType = ()>>> = Vec::new();

    for order in &order_by {
        match *order {
            "title" => order_columns.push(Box::new(column::title.asc())),
            "body" => order_columns.push(Box::new(column::body.asc())),
            "published" => order_columns.push(Box::new(column::published.asc())),
            other => return Err(MyErr::InvalidParam(other.to_string()))
        }
    }

    match order_columns.len() {
        1 => query = query.order(order_columns.remove(0)),
        2 => query = query.order((order_columns.remove(0), order_columns.remove(0))),
        3 => {
            query = query.order((
                order_columns.remove(0),
                order_columns.remove(0),
                order_columns.remove(0)
            ))
        }
        _ => {
            return Err(MyErr::TooManySortColumns("too many sort columns".to_string()))
        }
    }

    query.load::<Todo>(&establish_connection()).map_err(|e| MyErr::Diesel(e))
}

What are you trying to accomplish?

While this works great, I was wondering if there's a simpler/shorter way of accomplishing the same thing. According to the documentation, an order clause will override any other order clause. Ideally I'd like an API that would work something like this, where any order clauses would be appended instead of replacing any previous order clauses:

    for order in &order_by {
        match *order {
            "title" => query = query.order(Box::new(column::title.asc())),
            "body" => query = query.order(Box::new(column::body.asc())),
            "published" => query = query.order(Box::new(column::published.asc())),
            other => return Err(MyErr::InvalidParam(other.to_string()))
        }
    }

Is there an API that already does this with Diesel, and/or would it be possible to achieve (with current Rust stable)? If it is hypothetically possible but not currently available, I'd be happy to look into this if someone can point me in the right way :)

Checklist

• I have already looked over the issue tracker for similar issues.

[sgrif]

Short answer to answer the question: No, I don't think there is a less boilerplate-ey way to do this in 0.16/0.99

Longer answer for people looking to take a crack at this:

Ultimately the issue here comes from the fact that they are trying to have an unknown number of columns in their order clause. Unlike some other ORMs, Diesel has chosen to override the previous value for all query builder operations besides filter. (For example, .order(:foo).order(:bar) in Ruby on Rails will generate ORDER BY foo, bar).

The reasoning behind this is that it's unclear whether the order given last by the user should be considered more important or less important. Most members of the Rails team consider the behavior of order to be a mistake. The only reason filter is given special treatment here is that it is the only query builder method where we can combine its arguments in an associative fashion.

While I don't want to discount this use case, it is definitely one that is less common, which is why it was de-prioritized in favor of the tuple-based API that is causing pain here. I don't have any ideas on what this API should look like. Perhaps it'd make sense to have an API that takes a Vec, and applies it in the manner that this PR is attempting to do (I would actually love to have a vec passed to order behave in exactly this fashion)

[sgrif]

I'm moving this to the 1.0 milestone. I'm pretty sure we can make passing a Vec to order_by "just work", but it requires changing the signature of the trait, which is technically a major breaking change under RFC #1105

[sgrif]

Never mind, this will require a separate method...

[blakepettersson]

Thanks for the explanation! I presume that adding this method only really makes sense on a boxed query, and hence should only be implemented for BoxedSelectStatement?

[sgrif]

No, it makes sense for unboxed queries as well.

[sgrif]

I think I am going to restrict this to boxed queries, and go with the name .then_order_by (mostly because I can't come up with a good method name for "like order_by but it takes a vec!". I also think the API is more useable if we don't force the use of a vec for this. So the original code becomes:

let mut query = table.into_boxed();

for order in &order_by {
    match *order {
        "title" => query = query.then_order_by(column::title.asc()),
        "body" => query = query.then_order_by(column::body.asc()),
        "published" => query = query.then_order_by(column::published.asc()),
        other => return Err(MyErr::InvalidParam(other.to_string()))
    }
}

I'm open to alternative APIs if anyone has ideas.

[blakepettersson]

Hmm, maybe then_order_by should instead be called append_order_by?