remove old history delete methods

CHANGELOG.md

@@ -10,6 +10,11 @@
   - This was originally thought to be of use, but after running the system for sometime, we have found that this isn't needed.
   - In the spirit of reducing unique identifiers in telemetry and in the spirit of Lean Data, we have removed `enrollment_id` (and the code that generates it).
 
+## Places
+
+### ⚠️ Breaking Changes ⚠️
+  - `wipe_local` and `prune_destructively` have been removed from history API. `delete_everything` or `run_maintenance_*` methods should be used instead.
+
 # v120.0 (_2023-10-23_)
 
 ## FxA-Client

components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt

@@ -341,10 +341,6 @@ class PlacesWriterConnection internal constructor(conn: UniffiPlacesConnection,
         }
     }
 
-    override fun wipeLocal() {
-        this.conn.wipeLocalHistory()
-    }
-
     override fun runMaintenance(dbSizeLimit: UInt) {
         val pruneMetrics = PlacesManagerMetrics.runMaintenanceTime.measure {
             val pruneMetrics = PlacesManagerMetrics.runMaintenancePruneTime.measure {
@@ -367,10 +363,6 @@ class PlacesWriterConnection internal constructor(conn: UniffiPlacesConnection,
         PlacesManagerMetrics.dbSizeAfterMaintenance.accumulateSamples(listOf(pruneMetrics.dbSizeAfter.toLong() / 1024))
     }
 
-    override fun pruneDestructively() {
-        this.conn.pruneDestructively()
-    }
-
     override fun deleteEverything() {
         return writeQueryCounters.measure {
             this.conn.deleteEverythingHistory()
@@ -793,14 +785,6 @@ interface WritableHistoryConnection : ReadableHistoryConnection {
      */
     fun noteObservation(data: VisitObservation)
 
-    /**
-     * Deletes all history visits, without recording tombstones.
-     *
-     * That is, these deletions will not be synced. Any changes which were
-     * pending upload on the next sync are discarded and will be lost.
-     */
-    fun wipeLocal()
-
     /**
      * Run periodic database maintenance. This might include, but is not limited
      * to:
@@ -824,17 +808,6 @@ interface WritableHistoryConnection : ReadableHistoryConnection {
      */
     fun runMaintenance(dbSizeLimit: UInt = 0U)
 
-    /**
-     * Aggressively prune history visits. These deletions are not intended
-     * to be synced, however due to the way history sync works, this can
-     * still cause data loss.
-     *
-     * As a result, this should only be called if a low disk space
-     * notification is received from the OS, and things like the network
-     * cache have already been cleared.
-     */
-    fun pruneDestructively()
-
     /**
      * Delete everything locally.
      *

components/places/ios/Places/Places.swift

@@ -785,13 +785,6 @@ public class PlacesWriteConnection: PlacesReadConnection {
         }
     }
 
-    open func pruneDestructively() throws {
-        try queue.sync {
-            try self.checkApi()
-            try self.conn.pruneDestructively()
-        }
-    }
-
     open func acceptResult(searchString: String, url: String) throws {
         return try queue.sync {
             try self.checkApi()

components/places/src/ffi.rs

@@ -411,29 +411,13 @@ impl PlacesConnection {
             )
         })
     }
-
-    // XXX - We probably need to document/name this a little better as it's specifically for
-    // history and NOT bookmarks...
-    #[handle_error(crate::Error)]
-    pub fn wipe_local_history(&self) -> ApiResult<()> {
-        self.with_conn(history::wipe_local)
-    }
-
-    // Calls wipe_local_history but also updates the
-    // sync metadata to only sync after most recent visit to prevent
-    // further syncing of older data
+    // deletes all history and updates the sync metadata to only sync after
+    // most recent visit to prevent further syncing of older data
     #[handle_error(crate::Error)]
     pub fn delete_everything_history(&self) -> ApiResult<()> {
         history::delete_everything(&self.db.lock())
     }
 
-    // XXX - This just calls wipe_local under the hood...
-    // should probably have this go away?
-    #[handle_error(crate::Error)]
-    pub fn prune_destructively(&self) -> ApiResult<()> {
-        self.with_conn(history::prune_destructively)
-    }
-
     #[handle_error(crate::Error)]
     pub fn run_maintenance_prune(&self, db_size_limit: u32) -> ApiResult<RunMaintenanceMetrics> {
         self.with_conn(|conn| storage::run_maintenance_prune(conn, db_size_limit))

components/places/src/places.udl

@@ -117,21 +117,11 @@ interface PlacesConnection {
     [Throws=PlacesApiError]
     sequence<TopFrecentSiteInfo> get_top_frecent_site_infos(i32 num_items, FrecencyThresholdOption threshold_option);
 
-    // These three methods below are not actively being used by the consumers, we should investigate further
-    // and remove if so https://github.com/mozilla/application-services/issues/4719
-    [Throws=PlacesApiError]
-    void wipe_local_history();
-
     //From a-c: will not remove any history from remote devices, but it will prevent deleted
     // history from returning.
     [Throws=PlacesApiError]
     void delete_everything_history();
 
-    // Exactly the same as wipe_local_history
-    [Throws=PlacesApiError]
-    void prune_destructively();
-
-
     /// Run maintenance on the places DB (prune step)
     ///
     /// The `run_maintenance_*()` functions are intended to be run during idle time and will take steps

components/places/src/storage/history.rs

@@ -401,11 +401,6 @@ pub fn delete_place_visit_at_time_by_href(
     Ok(())
 }
 
-pub fn prune_destructively(db: &PlacesDb) -> Result<()> {
-    // For now, just fall back to wipe_local until we decide how this should work.
-    wipe_local(db)
-}
-
 pub fn prune_older_visits(db: &PlacesDb) -> Result<()> {
     let tx = db.begin_transaction()?;
     // Prune 6 items at a time, which matches desktops "small limit" value
@@ -494,15 +489,6 @@ fn find_exotic_visits_to_prune(
     )
 }
 
-pub fn wipe_local(db: &PlacesDb) -> Result<()> {
-    let tx = db.begin_transaction()?;
-    wipe_local_in_tx(db)?;
-    tx.commit()?;
-    // Note: SQLite cannot VACUUM within a transaction.
-    db.execute_batch("VACUUM")?;
-    Ok(())
-}
-
 fn wipe_local_in_tx(db: &PlacesDb) -> Result<()> {
     use crate::frecency::DEFAULT_FRECENCY_SETTINGS;
     db.execute_all(&[
@@ -540,7 +526,6 @@ fn wipe_local_in_tx(db: &PlacesDb) -> Result<()> {
     Ok(())
 }
 
-#[allow(unreachable_code)]
 pub fn delete_everything(db: &PlacesDb) -> Result<()> {
     let tx = db.begin_transaction()?;
 
@@ -2557,7 +2542,7 @@ mod tests {
     }
 
     #[test]
-    fn test_wipe_local() {
+    fn test_delete_local() {
         use crate::frecency::DEFAULT_FRECENCY_SETTINGS;
         use crate::storage::bookmarks::{
             self, BookmarkPosition, BookmarkRootGuid, InsertableBookmark, InsertableItem,
@@ -2641,7 +2626,7 @@ mod tests {
 
         assert!(bookmarks::delete_bookmark(&conn, &b0.0).unwrap());
 
-        wipe_local(&conn).unwrap();
+        delete_everything(&conn).unwrap();
 
         let places = conn
             .query_rows_and_then(

components/places/src/storage/history_metadata.rs

@@ -751,8 +751,8 @@ mod tests {
     };
     use crate::storage::fetch_page_info;
     use crate::storage::history::{
-        apply_observation, delete_visits_between, delete_visits_for, get_visit_count, url_to_guid,
-        wipe_local,
+        apply_observation, delete_everything, delete_visits_between, delete_visits_for,
+        get_visit_count, url_to_guid,
     };
     use crate::types::VisitType;
     use crate::VisitTransitionSet;
@@ -2006,7 +2006,7 @@ mod tests {
         assert_table_size!(&conn, "moz_origins", 2);
 
         // this somehow deletes 1 origin record, and our metadata
-        wipe_local(&conn).expect("places wipe succeeds");
+        delete_everything(&conn).expect("places wipe succeeds");
 
         assert_table_size!(&conn, "moz_places_metadata", 0);
         assert_table_size!(&conn, "moz_places_metadata_search_queries", 0);
@@ -2147,7 +2147,7 @@ mod tests {
         );
 
         // now, let's wipe places, and make sure none of the metadata stuff remains.
-        wipe_local(&conn).expect("places wipe succeeds");
+        delete_everything(&conn).expect("places wipe succeeds");
 
         assert_table_size!(&conn, "moz_places_metadata", 0);
         assert_table_size!(&conn, "moz_places_metadata_search_queries", 0);