delete_visits_between now deletes history metadata too

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))

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(())
 }

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,24 +2109,24 @@ 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::<i64, _>(
                 "SELECT id FROM moz_places_metadata_search_queries WHERE term = :term",
                 rusqlite::named_params! { ":term": "mozilla" },
                 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::<i64, _>(
                 "SELECT id FROM moz_places_metadata_search_queries WHERE term = :term",