From 8aa85ff798e8f8abddd37d6e66a26b478baf4e15 Mon Sep 17 00:00:00 2001 From: Sammy Khamis Date: Mon, 30 Oct 2023 15:14:15 -1000 Subject: [PATCH] remove old history delete methods --- CHANGELOG.md | 5 ++++ .../appservices/places/PlacesConnection.kt | 27 ------------------- components/places/ios/Places/Places.swift | 7 ----- components/places/src/ffi.rs | 20 ++------------ components/places/src/places.udl | 10 ------- components/places/src/storage/history.rs | 19 ++----------- .../places/src/storage/history_metadata.rs | 8 +++--- 7 files changed, 13 insertions(+), 83 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d74392167d..d7d0187c21 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt b/components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt index 07d06e8c8e..b9c06c6d1d 100644 --- a/components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt +++ b/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. * diff --git a/components/places/ios/Places/Places.swift b/components/places/ios/Places/Places.swift index 749ee1a0d3..6b3c1af9b2 100644 --- a/components/places/ios/Places/Places.swift +++ b/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() diff --git a/components/places/src/ffi.rs b/components/places/src/ffi.rs index 6fef4f49ae..9729a786dd 100644 --- a/components/places/src/ffi.rs +++ b/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 { self.with_conn(|conn| storage::run_maintenance_prune(conn, db_size_limit)) diff --git a/components/places/src/places.udl b/components/places/src/places.udl index ea20afb676..6592628872 100644 --- a/components/places/src/places.udl +++ b/components/places/src/places.udl @@ -117,21 +117,11 @@ interface PlacesConnection { [Throws=PlacesApiError] sequence 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 diff --git a/components/places/src/storage/history.rs b/components/places/src/storage/history.rs index ece297ebb4..82895ee265 100644 --- a/components/places/src/storage/history.rs +++ b/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( diff --git a/components/places/src/storage/history_metadata.rs b/components/places/src/storage/history_metadata.rs index a7beb1bc7f..9b65ddcb9b 100644 --- a/components/places/src/storage/history_metadata.rs +++ b/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);