diff --git a/components/places/examples/places-utils.rs b/components/places/examples/places-utils.rs index 00d3ad2f2b..81e6f4cbae 100644 --- a/components/places/examples/places-utils.rs +++ b/components/places/examples/places-utils.rs @@ -9,8 +9,8 @@ use cli_support::fxa_creds::{get_cli_fxa, get_default_fxa_config}; use places::bookmark_sync::store::BookmarksStore; use places::history_sync::store::HistoryStore; use places::storage::bookmarks::{ - fetch_tree, insert_tree, BookmarkNode, BookmarkRootGuid, BookmarkTreeNode, FolderNode, - SeparatorNode, + fetch_tree, insert_tree, BookmarkNode, BookmarkRootGuid, BookmarkTreeNode, FetchDepth, + FolderNode, SeparatorNode, }; use places::types::{BookmarkType, SyncGuid, Timestamp}; use places::{ConnectionType, PlacesApi, PlacesDb}; @@ -161,7 +161,7 @@ fn run_native_export(db: &PlacesDb, filename: String) -> Result<()> { let file = File::create(filename)?; let writer = BufWriter::new(file); - let tree = fetch_tree(db, &BookmarkRootGuid::Root.into())?.unwrap(); + let tree = fetch_tree(db, &BookmarkRootGuid::Root.into(), &FetchDepth::Deepest)?.unwrap(); serde_json::to_writer_pretty(writer, &tree)?; Ok(()) } diff --git a/components/places/src/storage/bookmarks.rs b/components/places/src/storage/bookmarks.rs index b3f1879568..8b4138cadd 100644 --- a/components/places/src/storage/bookmarks.rs +++ b/components/places/src/storage/bookmarks.rs @@ -76,6 +76,11 @@ pub enum BookmarkPosition { Append, } +pub enum FetchDepth { + Specific(usize), + Deepest, +} + /// Helpers to deal with managing the position correctly. /// Updates the position of existing items so that the insertion of a child in @@ -1122,12 +1127,13 @@ fn inflate( } } -/// Fetch the tree starting at the specified folder guid. -/// Returns a `BookmarkTreeNode::Folder(_)`, its parent's guid (if any), and +/// Fetch the tree starting at the specified guid. +/// Returns a `BookmarkTreeNode`, its parent's guid (if any), and /// position inside its parent. pub fn fetch_tree( db: &PlacesDb, item_guid: &SyncGuid, + target_depth: &FetchDepth, ) -> Result, u32)>> { // XXX - this needs additional work for tags - unlike desktop, there's no // "tags" folder, but instead a couple of tables to join on. @@ -1185,18 +1191,39 @@ pub fn fetch_tree( let row = result?; parent_guid = row.parent_guid.clone(); position = row.position; - FolderNode { - guid: Some(row.guid.clone()), - date_added: Some(row.date_added), - last_modified: Some(row.last_modified), - title: row.title.clone(), - children: Vec::new(), + match row.node_type { + BookmarkType::Folder => FolderNode { + guid: Some(row.guid.clone()), + date_added: Some(row.date_added), + last_modified: Some(row.last_modified), + title: row.title.clone(), + children: Vec::new(), + } + .into(), + BookmarkType::Bookmark => BookmarkNode { + guid: Some(row.guid.clone()), + date_added: Some(row.date_added), + last_modified: Some(row.last_modified), + title: row.title.clone(), + url: Url::parse(row.url.clone().unwrap().as_str())?, + } + .into(), + BookmarkType::Separator => SeparatorNode { + guid: Some(row.guid.clone()), + date_added: Some(row.date_added), + last_modified: Some(row.last_modified), + } + .into(), } - .into() } None => return Ok(None), }; + // Skip the rest and return if root is not a folder + if let BookmarkTreeNode::Bookmark(_) | BookmarkTreeNode::Separator(_) = root { + return Ok(Some((root, parent_guid, position))); + } + scope.err_if_interrupted()?; // For all remaining rows, build a pseudo-tree that maps parent GUIDs to // ordered children. We need this intermediate step because SQLite returns @@ -1206,6 +1233,12 @@ pub fn fetch_tree( for result in results { let row = result?; scope.err_if_interrupted()?; + // Check if we have done fetching the asked depth + if let FetchDepth::Specific(d) = *target_depth { + if row.level as usize > d + 1 { + break; + } + } let node = match row.node_type { BookmarkType::Bookmark => match &row.url { Some(url_str) => match Url::parse(&url_str) { @@ -1354,23 +1387,13 @@ fn get_raw_bookmarks_for_url(db: &PlacesDb, url: &Url) -> Result Result> { - Ok(db.query_rows_into_cached( - &format!( - "{} WHERE b.parent = :parent ORDER BY b.position ASC", - RAW_BOOKMARK_SQL - ), - &[(":parent", &parent)], - RawBookmark::from_row, - )?) -} #[cfg(test)] mod tests { use super::*; use crate::api::places_api::test::new_mem_connection; use crate::db::PlacesDb; - use crate::tests::{assert_json_tree, insert_json_tree}; + use crate::tests::{assert_json_tree, assert_json_tree_with_depth, insert_json_tree}; use pretty_assertions::assert_eq; use rusqlite::NO_PARAMS; use serde_json::Value; @@ -2078,7 +2101,8 @@ mod tests { let conn = new_mem_connection(); // Fetch the root - let (t, _, _) = fetch_tree(&conn, &BookmarkRootGuid::Root.into())?.unwrap(); + let (t, _, _) = + fetch_tree(&conn, &BookmarkRootGuid::Root.into(), &FetchDepth::Deepest)?.unwrap(); let f = match t { BookmarkTreeNode::Folder(ref f) => f, _ => panic!("tree root must be a folder"), @@ -2089,7 +2113,7 @@ mod tests { } #[test] - fn test_insert_tree() -> Result<()> { + fn test_insert_tree_and_fetch_level() -> Result<()> { let _ = env_logger::try_init(); let conn = new_mem_connection(); @@ -2140,8 +2164,45 @@ mod tests { }; insert_tree(&conn, &tree)?; - // check it. - assert_json_tree( + let expected = json!({ + "guid": &BookmarkRootGuid::Unfiled.as_guid(), + "children": [ + { + "title": "the bookmark", + "url": "https://www.example.com/" + }, + { + "title": "A folder", + "children": [ + { + "title": "bookmark 1 in A folder", + "url": "https://www.example2.com/" + }, + { + "title": "bookmark 2 in A folder", + "url": "https://www.example3.com/" + } + ], + }, + { + "title": "another bookmark", + "url": "https://www.example4.com/", + } + ] + }); + // check it with deepest fetching level. + assert_json_tree(&conn, &BookmarkRootGuid::Unfiled.into(), expected.clone()); + + // check it with one level deep, which should be the same as the previous + assert_json_tree_with_depth( + &conn, + &BookmarkRootGuid::Unfiled.into(), + expected.clone(), + &FetchDepth::Specific(1), + ); + + // check it with zero level deep, which should return root and its children only + assert_json_tree_with_depth( &conn, &BookmarkRootGuid::Unfiled.into(), json!({ @@ -2153,16 +2214,7 @@ mod tests { }, { "title": "A folder", - "children": [ - { - "title": "bookmark 1 in A folder", - "url": "https://www.example2.com/" - }, - { - "title": "bookmark 2 in A folder", - "url": "https://www.example3.com/" - } - ], + "children": [], }, { "title": "another bookmark", @@ -2170,7 +2222,10 @@ mod tests { } ] }), + &FetchDepth::Specific(0), ); + Ok(()) } + } diff --git a/components/places/src/storage/bookmarks/conversions.rs b/components/places/src/storage/bookmarks/conversions.rs index 8f3b8a9c8f..cc89069ebb 100644 --- a/components/places/src/storage/bookmarks/conversions.rs +++ b/components/places/src/storage/bookmarks/conversions.rs @@ -108,35 +108,6 @@ impl From for PublicNode { } } -impl PublicNode { - pub(crate) fn with_children( - mut self, - guids: Option>, - nodes: Option>, - ) -> Self { - if guids.is_some() || nodes.is_some() { - debug_assert_eq!( - self.node_type, - BookmarkType::Folder, - "Trying to set children on non-folder" - ); - debug_assert!( - guids.is_some() || nodes.is_some(), - "Only one of guids or nodes should be provided for folders" - ); - } else { - debug_assert_ne!( - self.node_type, - BookmarkType::Folder, - "Should provide children or guids to folder!" - ); - } - self.child_nodes = nodes; - self.child_guids = guids; - self - } -} - impl From> for msg_types::BookmarkNodeList { fn from(ns: Vec) -> Self { Self { diff --git a/components/places/src/storage/bookmarks/public_node.rs b/components/places/src/storage/bookmarks/public_node.rs index 5737bf707b..cebdeeaaf7 100644 --- a/components/places/src/storage/bookmarks/public_node.rs +++ b/components/places/src/storage/bookmarks/public_node.rs @@ -2,9 +2,9 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +use super::super::bookmarks::FetchDepth; use super::*; use crate::msg_types::BookmarkNode as ProtoBookmark; -use sql_support::SqlInterruptScope; /// This type basically exists to become a msg_types::BookmarkNode, but is /// slightly less of a pain to deal with in rust. @@ -93,105 +93,30 @@ pub fn fetch_bookmark( item_guid: &SyncGuid, get_direct_children: bool, ) -> Result> { - let _tx = db.begin_transaction()?; - let scope = db.begin_interrupt_scope(); - let bookmark = fetch_bookmark_in_tx(db, item_guid, get_direct_children, &scope)?; - // Note: We let _tx drop (which means it does a rollback) since it doesn't - // matter, we just are using a transaction to ensure things don't change out - // from under us, since this executes more than one query. - Ok(bookmark) -} - -fn get_child_guids(db: &PlacesDb, parent: RowId) -> Result> { - Ok(db.query_rows_into( - "SELECT guid FROM moz_bookmarks - WHERE parent = :parent - ORDER BY position ASC", - &[(":parent", &parent)], - |row| row.get(0), - )?) -} - -enum ChildInfo { - NoChildren, - Guids(Vec), - Nodes(Vec), -} - -impl ChildInfo { - fn guids_nodes(self) -> (Option>, Option>) { - match self { - ChildInfo::NoChildren => (None, None), - ChildInfo::Guids(g) => (Some(g), None), - ChildInfo::Nodes(n) => (None, Some(n)), - } - } -} - -fn fetch_bookmark_child_info( - db: &PlacesDb, - parent: &RawBookmark, - get_direct_children: bool, - scope: &SqlInterruptScope, -) -> Result { - if parent.bookmark_type != BookmarkType::Folder { - return Ok(ChildInfo::NoChildren); - } - // If we already know that we have no children, don't - // bother querying for them. - if parent.child_count == 0 { - return Ok(if get_direct_children { - ChildInfo::Nodes(vec![]) - } else { - ChildInfo::Guids(vec![]) - }); - } - if !get_direct_children { - // Just get the guids. - return Ok(ChildInfo::Guids(get_child_guids(db, parent.row_id)?)); - } - // Fetch children. the future this should probably be done by allowing a - // depth parameter to be passed into fetch_tree. - let child_nodes = get_raw_bookmarks_with_parent(db, parent.row_id)? - .into_iter() - .map(|kid| { - let child_guids = if kid.bookmark_type == BookmarkType::Folder { - if kid.child_count == 0 { - Some(vec![]) - } else { - Some(get_child_guids(db, kid.row_id)?) - } - } else { - None - }; - scope.err_if_interrupted()?; - Ok(PublicNode::from(kid).with_children(child_guids, None)) - }) - .collect::>>()?; - Ok(ChildInfo::Nodes(child_nodes)) -} - -// Implementation of fetch_bookmark -fn fetch_bookmark_in_tx( - db: &PlacesDb, - item_guid: &SyncGuid, - get_direct_children: bool, - scope: &SqlInterruptScope, -) -> Result> { - let rb = if let Some(raw) = get_raw_bookmark(db, item_guid)? { - raw + let depth = if get_direct_children { + FetchDepth::Specific(1) } else { - return Ok(None); + FetchDepth::Specific(0) }; + let mut bookmark = fetch_public_tree_with_depth(db, item_guid, &depth)?.unwrap(); + + if get_direct_children { + if let Some(child_nodes) = bookmark.child_nodes.as_mut() { + for node in child_nodes { + node.child_guids = node + .child_nodes + .take() + .map(|children| children.into_iter().map(|child| child.guid).collect()); + } + } + } else { + bookmark.child_guids = bookmark + .child_nodes + .take() + .map(|children| children.into_iter().map(|child| child.guid).collect()); + } - scope.err_if_interrupted()?; - // If we're a folder that has children, fetch child guids or children. - let (child_guids, child_nodes) = - fetch_bookmark_child_info(db, &rb, get_direct_children, scope)?.guids_nodes(); - - Ok(Some( - PublicNode::from(rb).with_children(child_guids, child_nodes), - )) + Ok(Some(bookmark)) } pub fn update_bookmark_from_message(db: &PlacesDb, msg: ProtoBookmark) -> Result<()> { @@ -207,13 +132,23 @@ pub fn update_bookmark_from_message(db: &PlacesDb, msg: ProtoBookmark) -> Result Ok(()) } -/// Call fetch_tree, convert the result to a ProtoBookmark, and ensure the -/// requested item's position and parent info are provided as well. This is the -/// function called by the FFI when requesting the tree. +/// Call fetch_public_tree_with_depth with FetchDepth::Deepest. +/// This is the function called by the FFI when requesting the tree. pub fn fetch_public_tree(db: &PlacesDb, item_guid: &SyncGuid) -> Result> { + fetch_public_tree_with_depth(db, item_guid, &FetchDepth::Deepest) +} + +/// Call fetch_tree with a depth parameter and convert the result +/// to a ProtoBookmark, and ensure the requested item's position +/// and parent info are provided as well. +pub fn fetch_public_tree_with_depth( + db: &PlacesDb, + item_guid: &SyncGuid, + target_depth: &FetchDepth, +) -> Result> { let _tx = db.begin_transaction()?; let (tree, parent_guid, position) = - if let Some((tree, parent_guid, position)) = fetch_tree(db, item_guid)? { + if let Some((tree, parent_guid, position)) = fetch_tree(db, item_guid, target_depth)? { (tree, parent_guid, position) } else { return Ok(None); diff --git a/components/places/src/tests.rs b/components/places/src/tests.rs index 754c3bdad2..4a064336cc 100644 --- a/components/places/src/tests.rs +++ b/components/places/src/tests.rs @@ -7,7 +7,7 @@ use serde_json::Value; use crate::{ db::PlacesDb, - storage::bookmarks::{fetch_tree, insert_tree, BookmarkTreeNode}, + storage::bookmarks::{fetch_tree, insert_tree, BookmarkTreeNode, FetchDepth}, types::SyncGuid, }; @@ -23,7 +23,16 @@ pub fn insert_json_tree(conn: &PlacesDb, jtree: Value) { } pub fn assert_json_tree(conn: &PlacesDb, folder: &SyncGuid, expected: Value) { - let (fetched, _, _) = fetch_tree(conn, folder) + assert_json_tree_with_depth(conn, folder, expected, &FetchDepth::Deepest) +} + +pub fn assert_json_tree_with_depth( + conn: &PlacesDb, + folder: &SyncGuid, + expected: Value, + target_depth: &FetchDepth, +) { + let (fetched, _, _) = fetch_tree(conn, folder, target_depth) .expect("error fetching tree") .unwrap(); let deser_tree: BookmarkTreeNode = serde_json::from_value(expected).unwrap();