Sorting and filtering data dynamically

Question

This application collects data from an external database (neo4j) on start-up and then provides one HTTP end-point where one can get a filtered and sorted response based on some JSON that's passed along in the body of the request.

main.rs:

#![feature(plugin)]
#![plugin(rocket_codegen)]

extern crate rusted_cypher;
extern crate rocket;
#[macro_use]
extern crate rocket_contrib;
#[macro_use]
extern crate serde_derive;
extern crate serde_json;
extern crate serde;
extern crate time;

use std::collections::HashMap;

use rusted_cypher::GraphClient;
use rocket_contrib::{JSON, Value};
use rocket::State;

fn get_year_of_release_date(release_date: i64) -> u16 {
    time::at_utc(time::Timespec {
                     nsec: 0,
                     sec: release_date,
                 })
            .tm_year as u16 + 1900
}

fn load_data() -> Vec<Media> {
    // TODO: get neo4j server url from environment
    let graph = GraphClient::connect("http://serveraddress:7474/db/data").unwrap();
    // query is a simple match of nodes that returns an id, rating, release date for collected nodes and sorts them by rating in descending order
    let results = graph.exec(query).unwrap();
    let mut data: Vec<Media> = Vec::new();
    for row in results.rows() {
        data.push(Media {
                      id: row.get("id").unwrap(),
                      rating: row.get("rating").unwrap(),
                      year: get_year_of_release_date(row.get("releaseDate").unwrap_or(0)),
                  });
    }

    data
}

#[derive(Debug, Serialize, Deserialize)]
struct Media {
    id: String,
    rating: u16,
    year: u16,
}

#[derive(Serialize, Deserialize, Debug, Clone)]
struct RecsysObject {
    #[serde(rename = "pageSize")]
    page_size: Option<usize>,
    page: Option<usize>,
    filters: Option<HashMap<String, SimpleYearFilter>>,
    sort: Option<Vec<HashMap<String, String>>>,
}

#[derive(Serialize, Deserialize, Debug, Clone)]
struct SimpleYearFilter {
    #[serde(rename = "$eq")]
    eq: Option<u16>,
    #[serde(rename = "$and")]
    and: Option<Vec<LogicFilter>>,
}

#[derive(Serialize, Deserialize, Debug, Clone)]
enum LogicFilter {
    #[serde(rename = "$gt")]
    GT(u16),
    #[serde(rename = "$lt")]
    LT(u16),
}

impl RecsysObject {
    fn construct_filter(self: &Self, movie: &Media) -> bool {
        match self.filters.clone() {
            None => true,
            Some(f) => {
                match f.get("year") {
                    None => true,
                    Some(yf) => {
                        match yf.eq {
                            Some(year) => movie.year == year,
                            None => {
                                match yf.and.clone() {
                                    None => true,
                                    Some(rangefilter) => {
                                        for rf in rangefilter {
                                            match rf {
                                                LogicFilter::LT(val) => {
                                                    if movie.year >= val {
                                                        return false;
                                                    }
                                                }
                                                LogicFilter::GT(val) => {
                                                    if movie.year <= val {
                                                        return false;
                                                    }
                                                }
                                            }
                                        }
                                        true
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

#[post("/", data = "<object>")]
fn one_point_of_success(allmovies: State<Vec<Media>>, object: JSON<RecsysObject>) -> JSON<Value> {
    let mut results = allmovies
                    .iter()
                    .filter(|m| object.construct_filter(m))
                    .collect::<Vec<_>>();
    match object.sort.clone() {
        None => (),
        Some(v) => {
            match v.get(0) {
                None => (),
                Some(hmap) => {
                    match hmap.get("property") {
                        None => {
                            if hmap.get("type").unwrap() == "popularity" {
                                match hmap.get("order") {
                                    Some(order) => {
                                        if order == "ascending" {
                                            results.sort_by(|a, b| a.rating.cmp(&b.rating));
                                        }
                                    }
                                    _ => {
                                        () // already sorted
                                    }
                                }
                            }
                        }
                        _ => {
                            if hmap.get("property").unwrap() == "metadata.releaseDate" {
                                match hmap.get("order") {
                                    Some(order) => {
                                        if order == "ascending" {
                                            results.sort_by(|a, b| a.year.cmp(&b.year));
                                        } else {
                                            results.sort_by(|a, b| b.year.cmp(&a.year));
                                        }
                                    }
                                    _ => {
                                        results.sort_by(|a, b| b.year.cmp(&a.year));
                                    }
                                }
                            } else {
                                ()
                            }
                        }

                    }
                }
            }
        }
    };
    JSON(json!(results.iter().skip(object.page.unwrap_or(0) * object.page_size.unwrap_or(10)) // first page is page 0
                    .take(object.page_size.unwrap_or(10)).collect::<Vec<_>>()))

}

fn main() {
    rocket::ignite()
        .manage(load_data())
        .mount("/", routes![one_point_of_success])
        .launch();
}

These types of filters/sorts are available to be sent as a JSON body into the POST request:

Single year filter

{"filters":{"year":{"$eq":1984}},"pageSize":10,"page":0}

Year range filter

{"filters":{"year":{"$and":[{"$gt":1982},{"$lt":1989}]}},"pageSize":10,"page":0}

Year range filter + year sort

{"filters":{"year":{"$and":[{"$gt":1900},{"$lt":2007}]}},"sort":[{"type":"property","property":"metadata.releaseDate","order":"descending"}],"pageSize":100,"page":310}

Year range filter + popularity sort

{"filters":{"year":{"$and":[{"$gt":1900},{"$lt":2018}]}},"sort":[{"type":"popularity","order":"descending"}],"pageSize":10,"page":0}

Popularity sort

{"sort":[{"type":"popularity","order":"descending"}],"pageSize":10,"page":0}

Year sort

{"sort":[{"type":"property","property":"metadata.releaseDate","order":"ascending"}],"pageSize":10,"page":0}

While putting everything into a vector might not be the best choice in terms of filtering and sorting, the performance is good enough. I'm primarily concerned on how ugly I've implemented the filtering and sorting, so I'm looking for feedback to improve on that, preferably with some concrete examples.

For completeness, this is built on nightly Rust with these dependencies:

rusted_cypher = "1.1.0"
rocket = { git = "https://github.com/SergioBenitez/Rocket" }
rocket_codegen = { git = "https://github.com/SergioBenitez/Rocket" }
rocket_contrib = { git = "https://github.com/SergioBenitez/Rocket" }
serde = "1.0.2"
serde_json = "1.0.1"
serde_derive = "1.0.2"
time = "0.1"

Answer 1

Instead of using the time crate, I recommend you use the chrono crate. It's not as low-level as time and it has more features. For example, with chrono, we don't need to add a magic 1900 value in get_year_of_release_date.

extern crate chrono;

use chrono::{Datelike, NaiveDateTime};

// Datelike::year() returns i32, not u16,
// so I propagated that change throughout the program.
fn get_year_of_release_date(release_date: i64) -> i32 {
    NaiveDateTime::from_timestamp(release_date, 0).year()
}

#[derive(Serialize, Deserialize, Debug, Clone)]
struct SimpleYearFilter {
    #[serde(rename = "$eq")]
    eq: Option<i32>,
    #[serde(rename = "$and")]
    and: Option<Vec<LogicFilter>>,
}

#[derive(Serialize, Deserialize, Debug, Clone)]
enum LogicFilter {
    #[serde(rename = "$gt")]
    GT(i32),
    #[serde(rename = "$lt")]
    LT(i32),
}

Next, you've got some deeply nested sets of matches. You're also performing some unnecessary clones in those matches. Let's address this first. When you match on a lvalue that you can't move, you can obtain a reference to an interior value by adding ref before the binding. This means that the bound variable will not contain the value directly, only a reference to the value. For example:

impl RecsysObject {
    fn construct_filter(self: &Self, movie: &Media) -> bool {
        match self.filters {
            None => true,
            Some(ref f) => {
                match f.get("year") {
                    None => true,
                    Some(yf) => {
                        match yf.eq {
                            Some(year) => movie.year == year,
                            None => {
                                match yf.and {
                                    None => true,
                                    Some(ref rangefilter) => {
                                        for rf in rangefilter {
                                            match *rf {
                                                LogicFilter::LT(val) => {
                                                    if movie.year >= val {
                                                        return false;
                                                    }
                                                }
                                                LogicFilter::GT(val) => {
                                                    if movie.year <= val {
                                                        return false;
                                                    }
                                                }
                                            }
                                        }
                                        true
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

On the first match, when self.filters is a Some, f will contain a reference to the HashMap.

When we get to the for rf in rangefilter loop, rangefilter is now a reference too. Generally, when we iterate on a reference to a collection, the iterator emits references too. Indeed, rf is now a reference to a LogicFilter. Here, I wrote match *rf to match the dereferenced value (note that this doesn't necessarily copy or clone the whole LogicFilter) because we bind i32 values, which are cheap to copy, so we bind them by value, not by reference.

Finally, let's simplify these nested matches. Let's look at RecsysObject::construct_filter first:

impl RecsysObject {
    fn construct_filter(&self, movie: &Media) -> bool {
        self.filters.as_ref().map_or(true, |f| {
            f.get("year").map_or(true, |yf| {
                if let Some(year) = yf.eq {
                    movie.year == year
                } else {
                    yf.and.as_ref().map_or(true,
                        |range_filter| range_filter.iter().all(|rf| rf.matches(movie.year)))
                }
            })
        })
    }
}

impl LogicFilter {
    fn matches(&self, actual: i32) -> bool {
        match *self {
            LogicFilter::LT(expected) => actual < expected,
            LogicFilter::GT(expected) => actual > expected,
        }
    }
}

Here, we're merely computing a value, so functional style is appropriate here. I'm using map_or (map_or(d, f) is a shorthand for map(f).unwrap_or(d)) to simplify the matches where None returns true. I need to use as_ref a couple times to turn a &Option<T> into an Option<&T>, because map_or takes self by value but we don't own either Option<T> in that function.

I've replaced the for loop with all, which takes care of exiting early when a false value is encountered. (any does the opposite.)

We could also use and_then to remove one level of nesting.

impl RecsysObject {
    fn construct_filter(&self, movie: &Media) -> bool {
        self.filters.as_ref()
            .and_then(|f| f.get("year"))
            .map_or(true, |yf| {
                if let Some(year) = yf.eq {
                    movie.year == year
                } else {
                    yf.and.as_ref().map_or(true,
                        |range_filter| range_filter.iter().all(|rf| rf.matches(movie.year)))
                }
            })
    }
}

Now for one_point_of_success:

#[post("/", data = "<object>")]
fn one_point_of_success(all_movies: State<Vec<Media>>, object: JSON<RecsysObject>) -> JSON<Value> {
    let mut results = all_movies
        .iter()
        .filter(|m| object.construct_filter(m))
        .collect::<Vec<_>>();

    if let Some(ref v) = object.sort {
        if let Some(hmap) = v.get(0) {
            match hmap.get("property").map(AsRef::as_ref) {
                Some("metadata.releaseDate") => {
                    if hmap.get("order").map_or("ascending", AsRef::as_ref) == "ascending" {
                        results.sort_by_key(|result| result.year);
                    } else {
                        results.sort_by_key(|result| RevOrd(result.year));
                    }
                }
                None => {
                    if hmap.get("type").expect("Expected either a `property` or a `type` attribute") == "popularity" {
                        if let Some("ascending") = hmap.get("order").map(AsRef::as_ref) {
                            results.sort_by_key(|result| result.rating);
                        } else {
                            // already sorted
                        }
                    }
                }
                _ => {}
            }
        }
    }

    JSON(json!(results.iter().skip(object.page.unwrap_or(0) * object.page_size.unwrap_or(10)) // first page is page 0
                      .take(object.page_size.unwrap_or(10)).collect::<Vec<_>>()))
}

The sorting part doesn't produce a value, it merely mutates an existing value (the results vector), so imperative style fits best. I'm using if let to simplify the matches where we only do something on one branch.

Other details:

• Write &self instead of self: &Self.
• Use sort_by_key instead of sort_by when you apply the same transformation on both values. To sort in descending (reverse) order, wrap the value in a RevOrd from the revord crate. If you ever need to sort on multiple values (sort by X then by Y...), just wrap them in a tuple (tuples implement Ord for up to 12 elements; if you need more, you can nest tuples as a workaround).
• You can match on string slices on string literals. If you have an Option<&String>, you need to convert it to an Option<&str>, which is what I'm doing with .map(AsRef::as_ref).
• Use expect instead of unwrap in order to provide an error message that gives some context when an unexpected condition does occur.

Answer 2

1. Use map and collect instead of iterating over something and pushing to a Vec. This avoids the need for a mutable variable and provides more efficient memory allocation.

2. Use expect instead of unwrap. expect has some useful error text presented to the end user when it fails.

3. Option<HashMap> is a little surprising. You may be able to just get away with a HashMap because it can hold zero values.

4. Idiomatic Rust naming is not to have consecutive uppercase letters in CamelCase names. That means it should be Lt; not LT.

5. Don't use self: &Self; that's idiomatically &self.

6. construct_filter seems like a poor name; it doesn't construct anything. It applies the filters, so perhaps that might be better.

7. There's no reason to clone the filters. Instead match on references (e.g. Some(ref foo)) or use methods like Option:as_ref.

8. Instead of iterating over things yourself, use iterator adapters like Iterator::any and Iterator::all.

9. The use of negative logic ("does this filter not match this element") is really confusing. Flip it around.

10. Extract methods to the structs and enums where they are most relevant. This also allows you to decrease nesting.

11. The query structure doesn't make sense; you can have both equality and less-than filters concurrently, but you only apply equality when both exist. Perhaps different datatypes might express your domain restrictions better.

12. You can flatten the equal / and match into one level by matching against a tuple.

13. Review all of Option's methods; you are going to be using them a lot. For example, map, as_ref, and_then are super common.

14. Don't unwrap property - just match on the Some() instead of _.

15. A missing else is already a (), no need to explicitly write it out.

16. Flatten out the sorting match too. Doing so shows it's really strange to apply the sort on three keys, instead of just two ("column" and "direction")...

17. Consider validating the sort parameters and converting them into two enums (column, direction). This splits up the concerns about which strings are valid, which are used for sorting, and would allow you to simply the sorting match.

18. Use if let when you only care about one variant of an enum.

19. Temporary variables like page and page_size are your friend. Avoids repeating the default of 10, for example.

20. Seems like it'd more efficient to take a slice of results instead of collecting into a vec.

#![feature(plugin)]
#![plugin(rocket_codegen)]

extern crate rusted_cypher;
extern crate rocket;
#[macro_use]
extern crate rocket_contrib;
#[macro_use]
extern crate serde_derive;
extern crate serde_json;
extern crate serde;
extern crate time;

use std::collections::HashMap;

use rusted_cypher::GraphClient;
use rocket_contrib::{JSON, Value};
use rocket::State;

fn get_year_of_release_date(release_date: i64) -> u16 {
    time::at_utc(time::Timespec {
                     nsec: 0,
                     sec: release_date,
                 })
            .tm_year as u16 + 1900
}

fn load_data() -> Vec<Media> {
    // TODO: get neo4j server url from environment
    let graph = GraphClient::connect("http://serveraddress:7474/db/data").unwrap();
    // query is a simple match of nodes that returns an id, rating, release date for collected nodes and sorts them by rating in descending order
    let query = "I didn't provide a value so this is just something";
    let results = graph.exec(query).unwrap();

    results.rows().map(|row| {
        Media {
            id: row.get("id").unwrap(),
            rating: row.get("rating").unwrap(),
            year: get_year_of_release_date(row.get("releaseDate").unwrap_or(0)),
        }
    }).collect();
}

#[derive(Debug, Serialize, Deserialize)]
struct Media {
    id: String,
    rating: u16,
    year: u16,
}

#[derive(Serialize, Deserialize, Debug, Clone)]
struct RecsysObject {
    #[serde(rename = "pageSize")]
    page_size: Option<usize>,
    page: Option<usize>,
    filters: Option<HashMap<String, SimpleYearFilter>>,
    sort: Option<Vec<HashMap<String, String>>>,
}

#[derive(Serialize, Deserialize, Debug, Clone)]
struct SimpleYearFilter {
    #[serde(rename = "$eq")]
    eq: Option<u16>,
    #[serde(rename = "$and")]
    and: Option<Vec<LogicFilter>>,
}

impl SimpleYearFilter {
    fn passes(&self, value: u16) -> bool {
        match (self.eq, &self.and) {
            (Some(my_val), _) => {
                value == my_val
            }
            (None, &Some(ref range_filters)) => {
                range_filters.iter().all(|filter| filter.passes(value))
            }
            _ => true,
        }
    }
}

#[derive(Serialize, Deserialize, Debug, Clone)]
enum LogicFilter {
    #[serde(rename = "$gt")]
    Gt(u16),
    #[serde(rename = "$lt")]
    Lt(u16),
}

impl LogicFilter {
    fn passes(&self, value: u16) -> bool {
        match *self {
            LogicFilter::Lt(my_val) => value < my_val,
            LogicFilter::Gt(my_val) => value > my_val,
        }
    }
}

impl RecsysObject {
    fn construct_filter(&self, movie: &Media) -> bool {
        self.filters.as_ref().and_then(|f| {
            f.get("year").map(|yf| yf.passes(movie.year))
        }).unwrap_or(true)
    }
}

#[post("/", data = "<object>")]
fn one_point_of_success(allmovies: State<Vec<Media>>, object: JSON<RecsysObject>) -> JSON<Value> {
    let mut results = allmovies
                    .iter()
                    .filter(|m| object.construct_filter(m))
                    .collect::<Vec<_>>();

    if let Some(hmap) = object.sort.as_ref().and_then(|v| v.get(0)) {
        let property = hmap.get("property").map(String::as_str);
        let typ = hmap.get("type").map(String::as_str);
        let order = hmap.get("order").map(String::as_str);

        match (property, typ, order) {
            (None, Some("popularity"), Some("ascending")) =>
                results.sort_by(|a, b| a.rating.cmp(&b.rating)),
            (Some("metadata.releaseDate"), _, Some("ascending")) =>
                results.sort_by(|a, b| a.year.cmp(&b.year)),
            (Some("metadata.releaseDate"), _, _) =>
                results.sort_by(|a, b| b.year.cmp(&a.year)),
            _ => {
                // Do nothing - unknown sort or default sort
            }
        }
    };

    let page = object.page.unwrap_or(0);
    let page_size = object.page_size.unwrap_or(10);

    let foo = results.iter().skip(page * page_size) // first page is page 0
        .take(page_size).collect::<Vec<_>>();
    JSON(json!(foo))
}

fn main() {
    rocket::ignite()
        .manage(load_data())
        .mount("/", routes![one_point_of_success])
        .launch();
}