Centralize API for working set to a single struct

Rather than allow addressing tasks either by working set ID or uuid,
with attendant performance issues, this moves the API for the working
set to a single struct that just serves as a 1-1 mapping of indexes to
UUIDs.  It's up to the caller to use this information.

cli/src/invocation/cmd/add.rs

@@ -33,7 +33,11 @@ mod test {
         execute(&mut w, &mut replica, modification).unwrap();
 
         // check that the task appeared..
-        let task = replica.get_working_set_task(1).unwrap().unwrap();
+        let working_set = replica.working_set().unwrap();
+        let task = replica
+            .get_task(working_set.by_index(1).unwrap())
+            .unwrap()
+            .unwrap();
         assert_eq!(task.get_description(), "my description");
         assert_eq!(task.get_status(), Status::Pending);
 

cli/src/invocation/cmd/info.rs

@@ -12,6 +12,8 @@ pub(crate) fn execute<W: WriteColor>(
     filter: Filter,
     debug: bool,
 ) -> Fallible<()> {
+    let working_set = replica.working_set()?;
+
     for task in filtered_tasks(replica, &filter)? {
         let uuid = task.get_uuid();
 
@@ -24,7 +26,7 @@ pub(crate) fn execute<W: WriteColor>(
             }
         } else {
             t.add_row(row![b->"Uuid", uuid]);
-            if let Some(i) = replica.get_working_set_index(uuid)? {
+            if let Some(i) = working_set.by_uuid(uuid) {
                 t.add_row(row![b->"Id", i]);
             }
             t.add_row(row![b->"Description", task.get_description()]);

cli/src/invocation/filter.rs

@@ -2,9 +2,9 @@ use crate::argparse::{Condition, Filter, TaskId};
 use failure::Fallible;
 use std::collections::HashSet;
 use std::convert::TryInto;
-use taskchampion::{Replica, Status, Tag, Task, Uuid};
+use taskchampion::{Replica, Status, Tag, Task, Uuid, WorkingSet};
 
-fn match_task(filter: &Filter, task: &Task, uuid: Uuid, working_set_id: Option<usize>) -> bool {
+fn match_task(filter: &Filter, task: &Task, uuid: Uuid, working_set: &WorkingSet) -> bool {
     for cond in &filter.conditions {
         match cond {
             Condition::HasTag(ref tag) => {
@@ -29,6 +29,8 @@ fn match_task(filter: &Filter, task: &Task, uuid: Uuid, working_set_id: Option<u
             Condition::IdList(ids) => {
                 let uuid_str = uuid.to_string();
                 let mut found = false;
+                let working_set_id = working_set.by_uuid(uuid);
+
                 for id in ids {
                     if match id {
                         TaskId::WorkingSetId(i) => Some(*i) == working_set_id,
@@ -111,6 +113,8 @@ pub(super) fn filtered_tasks(
 
     log::debug!("Applying filter {:?}", filter);
 
+    let working_set = replica.working_set()?;
+
     // We will enumerate the universe of tasks for this filter, checking
     // each resulting task with match_task
     match universe_for_filter(filter) {
@@ -123,7 +127,11 @@ pub(super) fn filtered_tasks(
             let mut seen = HashSet::new();
             for id in ids {
                 let task = match id {
-                    TaskId::WorkingSetId(id) => replica.get_working_set_task(*id)?,
+                    TaskId::WorkingSetId(id) => working_set
+                        .by_index(*id)
+                        .map(|uuid| replica.get_task(uuid))
+                        .transpose()?
+                        .flatten(),
                     TaskId::PartialUuid(_) => unreachable!(), // not present in absolute id list
                     TaskId::Uuid(id) => replica.get_task(*id)?,
                 };
@@ -136,9 +144,7 @@ pub(super) fn filtered_tasks(
                     }
                     seen.insert(uuid);
 
-                    let working_set_id = replica.get_working_set_index(uuid)?;
+                    if match_task(filter, &task, uuid, &working_set) {
-
-                    if match_task(filter, &task, uuid, working_set_id) {
                         res.push(task);
                     }
                 }
@@ -149,19 +155,16 @@ pub(super) fn filtered_tasks(
         Universe::AllTasks => {
             log::debug!("Scanning all tasks in the task database");
             for (uuid, task) in replica.all_tasks()?.drain() {
-                // Yikes, slow! https://github.com/djmitche/taskchampion/issues/108
+                if match_task(filter, &task, uuid, &working_set) {
-                let working_set_id = replica.get_working_set_index(uuid)?;
-                if match_task(filter, &task, uuid, working_set_id) {
                     res.push(task);
                 }
             }
         }
         Universe::WorkingSet => {
             log::debug!("Scanning only the working set (pending tasks)");
-            for (i, task) in replica.working_set()?.drain(..).enumerate() {
+            for (_, uuid) in working_set.iter() {
-                if let Some(task) = task {
+                if let Some(task) = replica.get_task(uuid)? {
-                    let uuid = task.get_uuid();
+                    if match_task(filter, &task, uuid, &working_set) {
-                    if match_task(filter, &task, uuid, Some(i)) {
                         res.push(task);
                     }
                 }

cli/src/invocation/report.rs

@@ -6,35 +6,9 @@ use config::Config;
 use failure::{format_err, Fallible};
 use prettytable::{Row, Table};
 use std::cmp::Ordering;
-use taskchampion::{Replica, Task, Uuid};
+use taskchampion::{Replica, Task, WorkingSet};
 use termcolor::WriteColor;
 
-// pending #123, this is a non-fallible way of looking up a task's working set index
-struct WorkingSet(Vec<Option<Uuid>>);
-
-impl WorkingSet {
-    fn new(replica: &mut Replica) -> Fallible<Self> {
-        let working_set = replica.working_set()?;
-        Ok(Self(
-            working_set
-                .iter()
-                .map(|opt| opt.as_ref().map(|t| t.get_uuid()))
-                .collect(),
-        ))
-    }
-
-    fn index(&self, target: Uuid) -> Option<usize> {
-        for (i, uuid) in self.0.iter().enumerate() {
-            if let Some(uuid) = uuid {
-                if *uuid == target {
-                    return Some(i);
-                }
-            }
-        }
-        None
-    }
-}
-
 /// Sort tasks for the given report.
 fn sort_tasks(tasks: &mut Vec<Task>, report: &Report, working_set: &WorkingSet) {
     tasks.sort_by(|a, b| {
@@ -43,8 +17,8 @@ fn sort_tasks(tasks: &mut Vec<Task>, report: &Report, working_set: &WorkingSet)
                 SortBy::Id => {
                     let a_uuid = a.get_uuid();
                     let b_uuid = b.get_uuid();
-                    let a_id = working_set.index(a_uuid);
+                    let a_id = working_set.by_uuid(a_uuid);
-                    let b_id = working_set.index(b_uuid);
+                    let b_id = working_set.by_uuid(b_uuid);
                     println!("a_uuid {} -> a_id {:?}", a_uuid, a_id);
                     println!("b_uuid {} -> b_id {:?}", b_uuid, b_id);
                     match (a_id, b_id) {
@@ -78,7 +52,7 @@ fn task_column(task: &Task, column: &Column, working_set: &WorkingSet) -> String
         Property::Id => {
             let uuid = task.get_uuid();
             let mut id = uuid.to_string();
-            if let Some(i) = working_set.index(uuid) {
+            if let Some(i) = working_set.by_uuid(uuid) {
                 id = i.to_string();
             }
             id
@@ -111,7 +85,7 @@ pub(super) fn display_report<W: WriteColor>(
     filter: Filter,
 ) -> Fallible<()> {
     let mut t = Table::new();
-    let working_set = WorkingSet::new(replica)?;
+    let working_set = replica.working_set()?;
 
     // Get the report from settings
     let mut report = Report::from_config(settings.get(&format!("reports.{}", report_name))?)
@@ -151,7 +125,7 @@ mod test {
     use crate::invocation::test::*;
     use crate::report::Sort;
     use std::convert::TryInto;
-    use taskchampion::Status;
+    use taskchampion::{Status, Uuid};
 
     fn create_tasks(replica: &mut Replica) -> [Uuid; 3] {
         let t1 = replica.new_task(Status::Pending, s!("A")).unwrap();
@@ -172,7 +146,7 @@ mod test {
     fn sorting_by_descr() {
         let mut replica = test_replica();
         create_tasks(&mut replica);
-        let working_set = WorkingSet::new(&mut replica).unwrap();
+        let working_set = replica.working_set().unwrap();
         let mut report = Report {
             sort: vec![Sort {
                 ascending: true,
@@ -199,7 +173,7 @@ mod test {
     fn sorting_by_id() {
         let mut replica = test_replica();
         create_tasks(&mut replica);
-        let working_set = WorkingSet::new(&mut replica).unwrap();
+        let working_set = replica.working_set().unwrap();
         let mut report = Report {
             sort: vec![Sort {
                 ascending: true,
@@ -226,7 +200,7 @@ mod test {
     fn sorting_by_uuid() {
         let mut replica = test_replica();
         let uuids = create_tasks(&mut replica);
-        let working_set = WorkingSet::new(&mut replica).unwrap();
+        let working_set = replica.working_set().unwrap();
         let report = Report {
             sort: vec![Sort {
                 ascending: true,
@@ -254,7 +228,7 @@ mod test {
             .add_tag(&("second".try_into().unwrap()))
             .unwrap();
 
-        let working_set = WorkingSet::new(&mut replica).unwrap();
+        let working_set = replica.working_set().unwrap();
         let report = Report {
             sort: vec![
                 Sort {
@@ -280,9 +254,9 @@ mod test {
     fn task_column_id() {
         let mut replica = test_replica();
         let uuids = create_tasks(&mut replica);
-        let working_set = WorkingSet::new(&mut replica).unwrap();
+        let working_set = replica.working_set().unwrap();
 
-        let task = replica.get_working_set_task(1).unwrap().unwrap();
+        let task = replica.get_task(uuids[0]).unwrap().unwrap();
         let column = Column {
             label: s!(""),
             property: Property::Id,
@@ -301,10 +275,10 @@ mod test {
     #[test]
     fn task_column_uuid() {
         let mut replica = test_replica();
-        create_tasks(&mut replica);
+        let uuids = create_tasks(&mut replica);
-        let working_set = WorkingSet::new(&mut replica).unwrap();
+        let working_set = replica.working_set().unwrap();
 
-        let task = replica.get_working_set_task(1).unwrap().unwrap();
+        let task = replica.get_task(uuids[0]).unwrap().unwrap();
         let column = Column {
             label: s!(""),
             property: Property::Uuid,
@@ -319,7 +293,7 @@ mod test {
     fn task_column_active() {
         let mut replica = test_replica();
         let uuids = create_tasks(&mut replica);
-        let working_set = WorkingSet::new(&mut replica).unwrap();
+        let working_set = replica.working_set().unwrap();
 
         // make task A active
         replica
@@ -335,19 +309,19 @@ mod test {
             property: Property::Active,
         };
 
-        let task = replica.get_working_set_task(1).unwrap().unwrap();
+        let task = replica.get_task(uuids[0]).unwrap().unwrap();
         assert_eq!(task_column(&task, &column, &working_set), s!("*"));
-        let task = replica.get_working_set_task(2).unwrap().unwrap();
+        let task = replica.get_task(uuids[2]).unwrap().unwrap();
         assert_eq!(task_column(&task, &column, &working_set), s!(""));
     }
 
     #[test]
     fn task_column_description() {
         let mut replica = test_replica();
-        create_tasks(&mut replica);
+        let uuids = create_tasks(&mut replica);
-        let working_set = WorkingSet::new(&mut replica).unwrap();
+        let working_set = replica.working_set().unwrap();
 
-        let task = replica.get_working_set_task(2).unwrap().unwrap();
+        let task = replica.get_task(uuids[2]).unwrap().unwrap();
         let column = Column {
             label: s!(""),
             property: Property::Description,
@@ -359,7 +333,7 @@ mod test {
     fn task_column_tags() {
         let mut replica = test_replica();
         let uuids = create_tasks(&mut replica);
-        let working_set = WorkingSet::new(&mut replica).unwrap();
+        let working_set = replica.working_set().unwrap();
 
         // add some tags to task A
         let mut t1 = replica
@@ -375,9 +349,9 @@ mod test {
             property: Property::Tags,
         };
 
-        let task = replica.get_working_set_task(1).unwrap().unwrap();
+        let task = replica.get_task(uuids[0]).unwrap().unwrap();
         assert_eq!(task_column(&task, &column, &working_set), s!("+bar +foo"));
-        let task = replica.get_working_set_task(2).unwrap().unwrap();
+        let task = replica.get_task(uuids[2]).unwrap().unwrap();
         assert_eq!(task_column(&task, &column, &working_set), s!(""));
     }
 }

taskchampion/src/lib.rs

@@ -32,10 +32,12 @@ mod task;
 mod taskdb;
 pub mod taskstorage;
 mod utils;
+mod workingset;
 
 pub use config::{ReplicaConfig, ServerConfig};
 pub use replica::Replica;
 pub use task::{Priority, Status, Tag, Task, TaskMut};
+pub use workingset::WorkingSet;
 
 /// Re-exported type from the `uuid` crate, for ease of compatibility for consumers of this crate.
 pub use uuid::Uuid;

taskchampion/src/replica.rs

@@ -4,6 +4,7 @@ use crate::server::Server;
 use crate::task::{Status, Task};
 use crate::taskdb::TaskDB;
 use crate::taskstorage::{KVStorage, Operation, TaskMap, TaskStorage};
+use crate::workingset::WorkingSet;
 use chrono::Utc;
 use failure::Fallible;
 use log::trace;
@@ -87,21 +88,10 @@ impl Replica {
         self.taskdb.all_task_uuids()
     }
 
-    /// Get the "working set" for this replica -- the set of pending tasks, as indexed by small
+    /// Get the "working set" for this replica.  This is a snapshot of the current state,
-    /// integers
+    /// and it is up to the caller to decide how long to store this value.
-    pub fn working_set(&mut self) -> Fallible<Vec<Option<Task>>> {
+    pub fn working_set(&mut self) -> Fallible<WorkingSet> {
-        let working_set = self.taskdb.working_set()?;
+        Ok(WorkingSet::new(self.taskdb.working_set()?))
-        let mut res = Vec::with_capacity(working_set.len());
-        for item in working_set.iter() {
-            res.push(match item {
-                Some(u) => match self.taskdb.get_task(*u)? {
-                    Some(tm) => Some(Task::new(*u, tm)),
-                    None => None,
-                },
-                None => None,
-            })
-        }
-        Ok(res)
     }
 
     /// Get an existing task by its UUID
@@ -112,33 +102,6 @@ impl Replica {
             .map(move |tm| Task::new(uuid, tm)))
     }
 
-    /// Get an existing task by its working set index
-    pub fn get_working_set_task(&mut self, i: usize) -> Fallible<Option<Task>> {
-        let working_set = self.taskdb.working_set()?;
-        if (i as usize) < working_set.len() {
-            if let Some(uuid) = working_set[i as usize] {
-                return Ok(self
-                    .taskdb
-                    .get_task(uuid)?
-                    .map(move |tm| Task::new(uuid, tm)));
-            }
-        }
-        Ok(None)
-    }
-
-    /// Get the working set index for the given task uuid
-    pub fn get_working_set_index(&mut self, uuid: Uuid) -> Fallible<Option<usize>> {
-        let working_set = self.taskdb.working_set()?;
-        for (i, u) in working_set.iter().enumerate() {
-            if let Some(u) = u {
-                if *u == uuid {
-                    return Ok(Some(i));
-                }
-            }
-        }
-        Ok(None)
-    }
-
     /// Create a new task.  The task must not already exist.
     pub fn new_task(&mut self, status: Status, description: String) -> Fallible<Task> {
         let uuid = Uuid::new_v4();
@@ -258,7 +221,8 @@ mod tests {
 
         rep.rebuild_working_set(true).unwrap();
 
-        assert!(rep.get_working_set_index(t.get_uuid()).unwrap().is_none());
+        let ws = rep.working_set().unwrap();
+        assert!(ws.by_uuid(t.get_uuid()).is_none());
     }
 
     #[test]
@@ -270,16 +234,13 @@ mod tests {
             .unwrap();
         let uuid = t.get_uuid();
 
-        let t = rep.get_working_set_task(1).unwrap().unwrap();
+        let ws = rep.working_set().unwrap();
-        assert_eq!(t.get_status(), Status::Pending);
+        assert_eq!(ws.len(), 1); // only one non-none value
-        assert_eq!(t.get_description(), "to-be-pending");
+        assert!(ws.by_index(0).is_none());
+        assert_eq!(ws.by_index(1), Some(uuid));
 
         let ws = rep.working_set().unwrap();
-        assert_eq!(ws.len(), 2);
+        assert_eq!(ws.by_uuid(t.get_uuid()), Some(1));
-        assert!(ws[0].is_none());
-        assert_eq!(ws[1].as_ref().unwrap().get_uuid(), uuid);
-
-        assert_eq!(rep.get_working_set_index(t.get_uuid()).unwrap().unwrap(), 1);
     }
 
     #[test]

taskchampion/src/workingset.rs

@@ -0,0 +1,132 @@
+use std::collections::HashMap;
+use uuid::Uuid;
+
+/// A WorkingSet represents a snapshot of the working set from a replica.
+///
+/// A replica's working set is a mapping from small integers to task uuids for all pending tasks.
+/// The small integers are meant to be stable, easily-typed identifiers for users to interact with
+/// important tasks.
+///
+/// IMPORTANT: the content of the working set may change at any time that a DB transaction is not
+/// in progress, and the data in this type will not be updated automatically.  It is up to the
+/// caller to decide how long to keep this value, and how much to trust the accuracy of its
+/// contents.  In practice, the answers are usually "a few milliseconds" and treating unexpected
+/// results as non-fatal.
+pub struct WorkingSet {
+    by_index: Vec<Option<Uuid>>,
+    by_uuid: HashMap<Uuid, usize>,
+}
+
+impl WorkingSet {
+    /// Create a new WorkingSet.  Typically this is acquired via `replica.working_set()`
+    pub(crate) fn new(by_index: Vec<Option<Uuid>>) -> Self {
+        let mut by_uuid = HashMap::new();
+        for (index, uuid) in by_index.iter().enumerate() {
+            if let Some(uuid) = uuid {
+                by_uuid.insert(*uuid, index);
+            }
+        }
+        Self { by_index, by_uuid }
+    }
+
+    /// Get the "length" of the working set: the total number of uuids in the set.
+    pub fn len(&self) -> usize {
+        self.by_index.iter().filter(|e| e.is_some()).count()
+    }
+
+    /// True if the length is zero
+    pub fn is_empty(&self) -> bool {
+        self.by_index.iter().all(|e| e.is_none())
+    }
+
+    /// Get the uuid with the given index, if any exists.
+    pub fn by_index(&self, index: usize) -> Option<Uuid> {
+        if let Some(Some(uuid)) = self.by_index.get(index) {
+            Some(*uuid)
+        } else {
+            None
+        }
+    }
+
+    /// Get the index for the given uuid, if any
+    pub fn by_uuid(&self, uuid: Uuid) -> Option<usize> {
+        self.by_uuid.get(&uuid).copied()
+    }
+
+    /// Iterate over pairs (index, uuid), in order by index.
+    pub fn iter(&self) -> impl Iterator<Item = (usize, Uuid)> + '_ {
+        self.by_index
+            .iter()
+            .enumerate()
+            .filter_map(|(index, uuid)| {
+                if let Some(uuid) = uuid {
+                    Some((index, *uuid))
+                } else {
+                    None
+                }
+            })
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    fn make() -> (Uuid, Uuid, WorkingSet) {
+        let uuid1 = Uuid::new_v4();
+        let uuid2 = Uuid::new_v4();
+        (
+            uuid1,
+            uuid2,
+            WorkingSet::new(vec![None, Some(uuid1), None, Some(uuid2), None]),
+        )
+    }
+
+    #[test]
+    fn test_new() {
+        let (_, uuid2, ws) = make();
+        assert_eq!(ws.by_index[3], Some(uuid2));
+        assert_eq!(ws.by_uuid.get(&uuid2), Some(&3));
+    }
+
+    #[test]
+    fn test_len_and_is_empty() {
+        let (_, _, ws) = make();
+        assert_eq!(ws.len(), 2);
+        assert_eq!(ws.is_empty(), false);
+
+        let ws = WorkingSet::new(vec![]);
+        assert_eq!(ws.len(), 0);
+        assert_eq!(ws.is_empty(), true);
+
+        let ws = WorkingSet::new(vec![None, None, None]);
+        assert_eq!(ws.len(), 0);
+        assert_eq!(ws.is_empty(), true);
+    }
+
+    #[test]
+    fn test_by_index() {
+        let (uuid1, uuid2, ws) = make();
+        assert_eq!(ws.by_index(0), None);
+        assert_eq!(ws.by_index(1), Some(uuid1));
+        assert_eq!(ws.by_index(2), None);
+        assert_eq!(ws.by_index(3), Some(uuid2));
+        assert_eq!(ws.by_index(4), None);
+        assert_eq!(ws.by_index(100), None); // past the end of the vector
+    }
+
+    #[test]
+    fn test_by_uuid() {
+        let (uuid1, uuid2, ws) = make();
+        let nosuch = Uuid::new_v4();
+        assert_eq!(ws.by_uuid(uuid1), Some(1));
+        assert_eq!(ws.by_uuid(uuid2), Some(3));
+        assert_eq!(ws.by_uuid(nosuch), None);
+    }
+
+    #[test]
+    fn test_iter() {
+        let (uuid1, uuid2, ws) = make();
+        assert_eq!(ws.iter().collect::<Vec<_>>(), vec![(1, uuid1), (3, uuid2),]);
+    }
+}