Implicitly group when joining searches (#1759)

* Implicitly group when joining searches

* Allow joining search types directly

* Test search joining

* Add comment for future selves (dae)

* Add one more assert that shows nested grouping (dae)

* Join user searches without grouping again

* Flatten a few clauses in custom study (dae)

rslib/src/backend/search/mod.rs

@@ -13,7 +13,7 @@ use crate::{
     backend_proto::sort_order::Value as SortOrderProto,
     browser_table::Column,
     prelude::*,
-    search::{replace_search_node, Node, SortMode},
+    search::{replace_search_node, JoinSearches, Node, SortMode},
 };
 
 impl SearchService for Backend {
@@ -45,12 +45,11 @@ impl SearchService for Backend {
     fn join_search_nodes(&self, input: pb::JoinSearchNodesRequest) -> Result<pb::String> {
         let existing_node: Node = input.existing_node.unwrap_or_default().try_into()?;
         let additional_node: Node = input.additional_node.unwrap_or_default().try_into()?;
-        let search = SearchBuilder::from_root(existing_node);
 
         Ok(
             match pb::search_node::group::Joiner::from_i32(input.joiner).unwrap_or_default() {
-                pb::search_node::group::Joiner::And => search.and(additional_node),
+                pb::search_node::group::Joiner::And => existing_node.and_flat(additional_node),
-                pb::search_node::group::Joiner::Or => search.or(additional_node),
+                pb::search_node::group::Joiner::Or => existing_node.or_flat(additional_node),
             }
             .write()
             .into(),

rslib/src/notetype/mod.rs

@@ -44,7 +44,7 @@ use crate::{
     define_newtype,
     error::{CardTypeError, CardTypeErrorDetails},
     prelude::*,
-    search::{Node, SearchNode},
+    search::{JoinSearches, Node, SearchNode},
     storage::comma_separated_ids,
     template::{FieldRequirements, ParsedTemplate},
     text::ensure_string_in_nfc,
@@ -222,7 +222,7 @@ impl Collection {
             .ok_or(AnkiError::NotFound)?;
 
         if self
-            .search_notes_unordered(SearchBuilder::from(note1.notetype_id).and(nids_node))?
+            .search_notes_unordered(note1.notetype_id.and(nids_node))?
             .len()
             != note_ids.len()
         {

rslib/src/notetype/notetypechange.rs

@@ -8,7 +8,7 @@ use std::collections::{HashMap, HashSet};
 use super::{CardGenContext, Notetype, NotetypeKind};
 use crate::{
     prelude::*,
-    search::{Node, SearchNode, SortMode, TemplateKind},
+    search::{JoinSearches, Node, SearchNode, SortMode, TemplateKind},
     storage::comma_separated_ids,
 };
 
@@ -294,10 +294,7 @@ impl Collection {
         if !map.removed.is_empty() {
             let ords =
                 SearchBuilder::any(map.removed.iter().map(|o| TemplateKind::Ordinal(*o as u16)));
-            self.search_cards_into_table(
+            self.search_cards_into_table(nids.and(ords), SortMode::NoOrder)?;
-                SearchBuilder::from(nids).and(ords.group()),
-                SortMode::NoOrder,
-            )?;
             for card in self.storage.all_searched_cards()? {
                 self.remove_card_and_add_grave_undoable(card, usn)?;
             }
@@ -319,10 +316,7 @@ impl Collection {
                     .keys()
                     .map(|o| TemplateKind::Ordinal(*o as u16)),
             );
-            self.search_cards_into_table(
+            self.search_cards_into_table(nids.and(ords), SortMode::NoOrder)?;
-                SearchBuilder::from(nids).and(ords.group()),
-                SortMode::NoOrder,
-            )?;
             for mut card in self.storage.all_searched_cards()? {
                 let original = card.clone();
                 card.template_idx =

rslib/src/notetype/schemachange.rs

@@ -8,7 +8,7 @@ use std::collections::HashMap;
 use super::{CardGenContext, Notetype};
 use crate::{
     prelude::*,
-    search::{SortMode, TemplateKind},
+    search::{JoinSearches, SortMode, TemplateKind},
 };
 
 /// True if any ordinals added, removed or reordered.
@@ -144,10 +144,7 @@ impl Collection {
         // remove any cards where the template was deleted
         if !changes.removed.is_empty() {
             let ords = SearchBuilder::any(changes.removed.into_iter().map(TemplateKind::Ordinal));
-            self.search_cards_into_table(
+            self.search_cards_into_table(nt.id.and(ords), SortMode::NoOrder)?;
-                SearchBuilder::from(nt.id).and(ords.group()),
-                SortMode::NoOrder,
-            )?;
             for card in self.storage.all_searched_cards()? {
                 self.remove_card_and_add_grave_undoable(card, usn)?;
             }
@@ -157,10 +154,7 @@ impl Collection {
         // update ordinals for cards with a repositioned template
         if !changes.moved.is_empty() {
             let ords = SearchBuilder::any(changes.moved.keys().cloned().map(TemplateKind::Ordinal));
-            self.search_cards_into_table(
+            self.search_cards_into_table(nt.id.and(ords), SortMode::NoOrder)?;
-                SearchBuilder::from(nt.id).and(ords.group()),
-                SortMode::NoOrder,
-            )?;
             for mut card in self.storage.all_searched_cards()? {
                 let original = card.clone();
                 card.template_idx = *changes.moved.get(&card.template_idx).unwrap();

rslib/src/scheduler/bury_and_suspend.rs

@@ -10,7 +10,7 @@ use crate::{
     card::CardQueue,
     config::SchedulerVersion,
     prelude::*,
-    search::{SearchNode, SortMode, StateKind},
+    search::{JoinSearches, SearchNode, SortMode, StateKind},
 };
 
 impl Card {
@@ -79,7 +79,7 @@ impl Collection {
         };
         self.transact(Op::UnburyUnsuspend, |col| {
             col.search_cards_into_table(
-                SearchBuilder::from(SearchNode::DeckIdWithChildren(deck_id)).and(state),
+                SearchNode::DeckIdWithChildren(deck_id).and(state),
                 SortMode::NoOrder,
             )?;
             col.unsuspend_or_unbury_searched_cards()

rslib/src/scheduler/filtered/custom_study.rs

@@ -13,7 +13,7 @@ use crate::{
     decks::{FilteredDeck, FilteredSearchOrder, FilteredSearchTerm},
     error::{CustomStudyError, FilteredDeckError},
     prelude::*,
-    search::{Negated, PropertyKind, RatingKind, SearchNode, StateKind},
+    search::{JoinSearches, Negated, PropertyKind, RatingKind, SearchNode, StateKind},
 };
 
 impl Collection {
@@ -184,29 +184,29 @@ fn custom_study_config(
 }
 
 fn forgot_config(deck_name: String, days: u32) -> FilteredDeck {
-    let search = SearchBuilder::from(SearchNode::Rated {
+    let search = SearchNode::Rated {
         days,
         ease: RatingKind::AnswerButton(1),
-    })
+    }
     .and(SearchNode::from_deck_name(&deck_name))
     .write();
     custom_study_config(false, search, FilteredSearchOrder::Random, None)
 }
 
 fn ahead_config(deck_name: String, days: u32) -> FilteredDeck {
-    let search = SearchBuilder::from(SearchNode::Property {
+    let search = SearchNode::Property {
         operator: "<=".to_string(),
         kind: PropertyKind::Due(days as i32),
-    })
+    }
     .and(SearchNode::from_deck_name(&deck_name))
     .write();
     custom_study_config(true, search, FilteredSearchOrder::Due, None)
 }
 
 fn preview_config(deck_name: String, days: u32) -> FilteredDeck {
-    let search = SearchBuilder::from(StateKind::New)
+    let search = StateKind::New
-        .and(SearchNode::AddedInDays(days))
+        .and_flat(SearchNode::AddedInDays(days))
-        .and(SearchNode::from_deck_name(&deck_name))
+        .and_flat(SearchNode::from_deck_name(&deck_name))
         .write();
     custom_study_config(
         false,
@@ -237,8 +237,8 @@ fn cram_config(deck_name: String, cram: &Cram) -> Result<FilteredDeck> {
     };
 
     let search = nodes
-        .and(tags_to_nodes(&cram.tags_to_include, &cram.tags_to_exclude))
         .and(SearchNode::from_deck_name(&deck_name))
+        .and_flat(tags_to_nodes(&cram.tags_to_include, &cram.tags_to_exclude))
         .write();
 
     Ok(custom_study_config(
@@ -261,7 +261,7 @@ fn tags_to_nodes(tags_to_include: &[String], tags_to_exclude: &[String]) -> Sear
             .map(|tag| SearchNode::from_tag_name(tag).negated()),
     );
 
-    include_nodes.group().and(exclude_nodes)
+    include_nodes.and(exclude_nodes)
 }
 
 #[cfg(test)]
@@ -357,4 +357,23 @@ mod test {
 
         Ok(())
     }
+
+    #[test]
+    fn sql_grouping() -> Result<()> {
+        let mut deck = preview_config("d".into(), 1);
+        assert_eq!(&deck.search_terms[0].search, "is:new added:1 deck:d");
+
+        let cram = Cram {
+            tags_to_include: vec!["1".into(), "2".into()],
+            tags_to_exclude: vec!["3".into(), "4".into()],
+            ..Default::default()
+        };
+        deck = cram_config("d".into(), &cram)?;
+        assert_eq!(
+            &deck.search_terms[0].search,
+            "is:due deck:d (tag:1 OR tag:2) (-tag:3 -tag:4)"
+        );
+
+        Ok(())
+    }
 }

rslib/src/scheduler/new.rs

@@ -14,7 +14,7 @@ use crate::{
     config::{BoolKey, SchedulerVersion},
     deckconfig::NewCardInsertOrder,
     prelude::*,
-    search::{SearchNode, SortMode, StateKind},
+    search::{JoinSearches, SearchNode, SortMode, StateKind},
 };
 
 impl Card {
@@ -267,7 +267,7 @@ impl Collection {
         usn: Usn,
     ) -> Result<usize> {
         let cids = self.search_cards(
-            SearchBuilder::from(SearchNode::DeckIdWithoutChildren(deck)).and(StateKind::New),
+            SearchNode::DeckIdWithoutChildren(deck).and(StateKind::New),
             SortMode::NoOrder,
         )?;
         self.sort_cards_inner(&cids, 1, 1, order.into(), false, usn)

rslib/src/search/builder.rs

@@ -12,6 +12,19 @@ pub trait Negated {
     fn negated(self) -> Node;
 }
 
+pub trait JoinSearches {
+    /// Concatenates two sets of [Node]s, inserting [Node::And], and grouping, if appropriate.
+    fn and(self, other: impl Into<SearchBuilder>) -> SearchBuilder;
+    /// Concatenates two sets of [Node]s, inserting [Node::Or], and grouping, if appropriate.
+    fn or(self, other: impl Into<SearchBuilder>) -> SearchBuilder;
+    /// Concatenates two sets of [Node]s, inserting [Node::And] if appropriate,
+    /// but without grouping either set.
+    fn and_flat(self, other: impl Into<SearchBuilder>) -> SearchBuilder;
+    /// Concatenates two sets of [Node]s, inserting [Node::Or] if appropriate,
+    /// but without grouping either set.
+    fn or_flat(self, other: impl Into<SearchBuilder>) -> SearchBuilder;
+}
+
 impl<T: Into<Node>> Negated for T {
     fn negated(self) -> Node {
         let node: Node = self.into();
@@ -23,6 +36,24 @@ impl<T: Into<Node>> Negated for T {
     }
 }
 
+impl<T: Into<SearchBuilder>> JoinSearches for T {
+    fn and(self, other: impl Into<SearchBuilder>) -> SearchBuilder {
+        self.into().join_other(other.into(), Node::And, true)
+    }
+
+    fn or(self, other: impl Into<SearchBuilder>) -> SearchBuilder {
+        self.into().join_other(other.into(), Node::Or, true)
+    }
+
+    fn and_flat(self, other: impl Into<SearchBuilder>) -> SearchBuilder {
+        self.into().join_other(other.into(), Node::And, false)
+    }
+
+    fn or_flat(self, other: impl Into<SearchBuilder>) -> SearchBuilder {
+        self.into().join_other(other.into(), Node::Or, false)
+    }
+}
+
 /// Helper to programmatically build searches.
 #[derive(Debug, PartialEq, Clone)]
 pub struct SearchBuilder(Vec<Node>);
@@ -59,19 +90,11 @@ impl SearchBuilder {
         self.0.len()
     }
 
-    /// Concatenates the two sets of [Node]s, inserting [Node::And] if appropriate.
+    fn join_other(mut self, mut other: Self, joiner: Node, group: bool) -> Self {
-    /// No implicit grouping is done.
+        if group {
-    pub fn and(self, other: impl Into<SearchBuilder>) -> Self {
+            self = self.group();
-        self.join_other(other.into(), Node::And)
+            other = other.group();
-    }
+        }
-
-    /// Concatenates the two sets of [Node]s, inserting [Node::Or] if appropriate.
-    /// No implicit grouping is done.
-    pub fn or(self, other: impl Into<SearchBuilder>) -> Self {
-        self.join_other(other.into(), Node::Or)
-    }
-
-    fn join_other(mut self, mut other: Self, joiner: Node) -> Self {
         if !(self.is_empty() || other.is_empty()) {
             self.0.push(joiner);
         }
@@ -185,4 +208,38 @@ mod test {
             Node::Not(Box::new(Node::Search(SearchNode::State(StateKind::Due))))
         )
     }
+
+    #[test]
+    fn joining() {
+        assert_eq!(
+            StateKind::Due
+                .or(StateKind::New)
+                .and(SearchBuilder::any((1..4).map(SearchNode::Flag)))
+                .write(),
+            "(is:due OR is:new) (flag:1 OR flag:2 OR flag:3)"
+        );
+        assert_eq!(
+            StateKind::Due
+                .or(StateKind::New)
+                .and_flat(SearchBuilder::any((1..4).map(SearchNode::Flag)))
+                .write(),
+            "is:due OR is:new flag:1 OR flag:2 OR flag:3"
+        );
+        assert_eq!(
+            StateKind::Due
+                .or(StateKind::New)
+                .or(StateKind::Learning)
+                .or(StateKind::Review)
+                .write(),
+            "((is:due OR is:new) OR is:learn) OR is:review"
+        );
+        assert_eq!(
+            StateKind::Due
+                .or_flat(StateKind::New)
+                .or_flat(StateKind::Learning)
+                .or_flat(StateKind::Review)
+                .write(),
+            "is:due OR is:new OR is:learn OR is:review"
+        );
+    }
 }

rslib/src/search/mod.rs

@@ -8,7 +8,7 @@ pub(crate) mod writer;
 
 use std::borrow::Cow;
 
-pub use builder::{Negated, SearchBuilder};
+pub use builder::{JoinSearches, Negated, SearchBuilder};
 pub use parser::{
     parse as parse_search, Node, PropertyKind, RatingKind, SearchNode, StateKind, TemplateKind,
 };

rslib/src/search/sqlwriter.rs

@@ -113,6 +113,9 @@ impl SqlWriter<'_> {
         }
     }
 
+    // NOTE: when adding any new nodes in the future, make sure that they are either a single
+    // search term, or they wrap multiple terms in parentheses, as can be seen in the sql() unit
+    // test at the bottom of the file.
     fn write_search_node_to_sql(&mut self, node: &SearchNode) -> Result<()> {
         use normalize_to_nfc as norm;
         match node {