From 97f2ebebcc0ae757ece0f3c56c10e408d29e7733 Mon Sep 17 00:00:00 2001 From: Tarik Eshaq Date: Fri, 15 Jul 2022 08:34:41 -0400 Subject: [PATCH] delete_visits_between now deletes history metadata too --- CHANGES_UNRELEASED.md | 4 + components/places/src/storage/history.rs | 5 +- .../places/src/storage/history_metadata.rs | 121 ++++++++++++++++-- 3 files changed, 120 insertions(+), 10 deletions(-) diff --git a/CHANGES_UNRELEASED.md b/CHANGES_UNRELEASED.md index 78d2bf0a4e..e85e33f6d3 100644 --- a/CHANGES_UNRELEASED.md +++ b/CHANGES_UNRELEASED.md @@ -23,3 +23,7 @@ Use the template below to make assigning a version number during the release cut ### What's New - Added initial work to support component FML files into an app FML. ([#4999](https://github.com/mozilla/application-services/pull/4999)). Please upgrade your nimbus-fml tooling at the same time as upgrading the megazord. + +## Places +### What's changed + - The `delete_visits_between` API now also deletes history metadata ([#5046](https://github.com/mozilla/application-services/pull/5046)) diff --git a/components/places/src/storage/history.rs b/components/places/src/storage/history.rs index be6e597d84..9f625a78ea 100644 --- a/components/places/src/storage/history.rs +++ b/components/places/src/storage/history.rs @@ -259,7 +259,7 @@ fn delete_visits_for_in_tx(db: &PlacesDb, guid: &SyncGuid) -> Result<()> { )?; // Note that history metadata has an `ON DELETE CASCADE` for the place ID - so if we // call `delete_page` here, we assume history metadata dies too. Otherwise we - // explicitly delete the metadata after we delete the visits themself. + // explicitly delete the metadata after we delete the visits themselves. match to_clean { Some(PageToClean { id, @@ -615,6 +615,9 @@ pub fn delete_visits_between_in_tx(db: &PlacesDb, start: Timestamp, end: Timesta cleanup_pages(db, &pages) }, )?; + + // Clean up history metadata between start and end + history_metadata::delete_between(db, start.as_millis_i64(), end.as_millis_i64())?; delete_pending_temp_tables(db)?; Ok(()) } diff --git a/components/places/src/storage/history_metadata.rs b/components/places/src/storage/history_metadata.rs index 1ced390737..bccda1a6d5 100644 --- a/components/places/src/storage/history_metadata.rs +++ b/components/places/src/storage/history_metadata.rs @@ -487,6 +487,15 @@ pub fn delete_older_than(db: &PlacesDb, older_than: i64) -> Result<()> { Ok(()) } +pub fn delete_between(db: &PlacesDb, start: i64, end: i64) -> Result<()> { + db.execute_cached( + "DELETE FROM moz_places_metadata + WHERE updated_at > :start and updated_at < :end", + &[(":start", &start), (":end", &end)], + )?; + Ok(()) +} + /// Delete all metadata for the specified place id. pub fn delete_all_metadata_for_page(db: &PlacesDb, place_id: RowId) -> Result<()> { db.execute_cached( @@ -1653,7 +1662,7 @@ mod tests { delete_older_than(&conn, beginning).expect("delete worked"); assert_eq!(3, get_since(&conn, beginning).expect("get worked").len()); - // boundary condition, should only delete the last one. + // boundary condition, should only delete the first one. delete_older_than(&conn, after_meta1).expect("delete worked"); assert_eq!(2, get_since(&conn, beginning).expect("get worked").len()); assert_eq!( @@ -1667,6 +1676,56 @@ mod tests { assert_eq!(0, get_since(&conn, beginning).expect("get worked").len()); } + #[test] + fn test_delete_between() { + let conn = PlacesDb::open_in_memory(ConnectionType::ReadWrite).expect("memory db"); + + let beginning = Timestamp::now().as_millis() as i64; + thread::sleep(time::Duration::from_millis(10)); + + note_observation!(&conn, + url "http://mozilla.com/1", + view_time Some(20000), + search_term None, + document_type Some(DocumentType::Regular), + referrer_url None, + title None + ); + + thread::sleep(time::Duration::from_millis(10)); + + note_observation!(&conn, + url "http://mozilla.com/2", + view_time Some(20000), + search_term None, + document_type Some(DocumentType::Regular), + referrer_url None, + title None + ); + let after_meta2 = Timestamp::now().as_millis() as i64; + + thread::sleep(time::Duration::from_millis(10)); + + note_observation!(&conn, + url "http://mozilla.com/3", + view_time Some(20000), + search_term None, + document_type Some(DocumentType::Regular), + referrer_url None, + title None + ); + let after_meta3 = Timestamp::now().as_millis() as i64; + + // deleting meta 3 + delete_between(&conn, after_meta2, after_meta3).expect("delete worked"); + assert_eq!(2, get_since(&conn, beginning).expect("get worked").len()); + assert_eq!( + None, + get_latest_for_url(&conn, &Url::parse("http://mozilla.com/3").expect("url")) + .expect("get") + ); + } + #[test] fn test_metadata_deletes_do_not_affect_places() { let conn = PlacesDb::open_in_memory(ConnectionType::ReadWrite).expect("memory db"); @@ -1817,6 +1876,50 @@ mod tests { .is_none()); } + #[test] + fn test_delete_between_also_deletes_metadata() -> Result<()> { + let conn = PlacesDb::open_in_memory(ConnectionType::ReadWrite).expect("memory db"); + + let now = Timestamp::now(); + let url = Url::parse("https://www.mozilla.org/").unwrap(); + let other_url = + Url::parse("https://www.google.com/search?client=firefox-b-d&q=mozilla+firefox") + .unwrap(); + let start_timestamp = Timestamp(now.as_millis() - 1000_u64); + let end_timestamp = Timestamp(now.as_millis() + 1000_u64); + let observation1 = VisitObservation::new(url.clone()) + .with_at(start_timestamp) + .with_title(Some(String::from("Test page 0"))) + .with_is_remote(false) + .with_visit_type(VisitTransition::Link); + + let observation2 = VisitObservation::new(other_url) + .with_at(end_timestamp) + .with_title(Some(String::from("Test page 1"))) + .with_is_remote(false) + .with_visit_type(VisitTransition::Link); + + apply_observation(&conn, observation1).expect("Should apply visit"); + apply_observation(&conn, observation2).expect("Should apply visit"); + + note_observation!( + &conn, + url "https://www.mozilla.org/", + view_time Some(20000), + search_term Some("mozilla firefox"), + document_type Some(DocumentType::Regular), + referrer_url Some("https://www.google.com/search?client=firefox-b-d&q=mozilla+firefox"), + title None + ); + assert_eq!( + "https://www.mozilla.org/", + get_latest_for_url(&conn, &url)?.unwrap().url + ); + delete_visits_between(&conn, start_timestamp, end_timestamp)?; + assert_eq!(None, get_latest_for_url(&conn, &url)?); + Ok(()) + } + #[test] fn test_places_delete_triggers_with_bookmarks() { // The cleanup functionality lives as a TRIGGER in `create_shared_triggers`. @@ -2006,12 +2109,12 @@ mod tests { .expect("get worked"); assert!(meta1.is_none(), "expected metadata to have been deleted"); - assert!( - meta2.is_some(), - "expected metadata to not have been deleted" - ); + // Verify that if a history metadata entry was entered **after** the visit + // then we delete the range of the metadata, and not the visit. The metadata + // is still deleted + assert!(meta2.is_none(), "expected metadata to been deleted"); - // still have a 'mozilla' search query entry, since one meta entry points to it. + // The 'mozilla' search query entry is deleted since the delete cascades. assert!( conn.try_query_one::( "SELECT id FROM moz_places_metadata_search_queries WHERE term = :term", @@ -2019,11 +2122,11 @@ mod tests { true ) .expect("select works") - .is_some(), - "search_query records with related metadata should not have been deleted" + .is_none(), + "search_query records with related metadata should have been deleted" ); - // don't have the 'firefox' search query entry anymore, nothing points to it. + // don't have the 'firefox' search query entry either, nothing points to it. assert!( conn.try_query_one::( "SELECT id FROM moz_places_metadata_search_queries WHERE term = :term",