Remote Settings: use changes endpoint

Hit this endpoint in `RemoteSettingsService::sync` and use it to
determine which collections actually need syncing.

Author: bendk

CHANGELOG.md

@@ -7,6 +7,12 @@
 
 #### BREAKING CHANGE
 - Removed `Megazord.kt` and moved the contents to the new `RustComponentsInitializer.kt`.
+- 
+## 🦊 What's Changed 🦊
+
+### Remote Settings
+- `RemoteSettingsService::sync` is now more efficient.  It checks the remote settings changes
+  endpoint and only syncs collections that have been modified since the last sync.
 
 ## 🔧 What's Fixed 🔧
 

components/remote_settings/src/client.rs

@@ -294,6 +294,12 @@ impl<C: ApiClient> RemoteSettingsClient<C> {
         })
     }
 
+    pub fn get_last_modified_timestamp(&self) -> Result<Option<u64>> {
+        let mut inner = self.inner.lock();
+        let collection_url = inner.api_client.collection_url();
+        inner.storage.get_last_modified_timestamp(&collection_url)
+    }
+
     /// Synchronizes the local collection with the remote server by performing the following steps:
     /// 1. Fetches the last modified timestamp of the collection from local storage.
     /// 2. Fetches the changeset from the remote server based on the last modified timestamp.
@@ -950,7 +956,7 @@ impl Default for RemoteState {
 }
 
 impl RemoteState {
-    fn handle_backoff_hint(&mut self, response: &Response) -> Result<()> {
+    pub fn handle_backoff_hint(&mut self, response: &Response) -> Result<()> {
         let extract_backoff_header = |header| -> Result<u64> {
             Ok(response
                 .headers
@@ -973,7 +979,7 @@ impl RemoteState {
         Ok(())
     }
 
-    fn ensure_no_backoff(&mut self) -> Result<()> {
+    pub fn ensure_no_backoff(&mut self) -> Result<()> {
         if let BackoffState::Backoff {
             observed_at,
             duration,

components/remote_settings/src/config.rs

@@ -165,4 +165,8 @@ impl BaseUrl {
         // error for cannot-be-a-base URLs.
         self.url.path_segments_mut().unwrap()
     }
+
+    pub fn query_pairs_mut(&mut self) -> url::form_urlencoded::Serializer<'_, url::UrlQuery<'_>> {
+        self.url.query_pairs_mut()
+    }
 }

components/remote_settings/src/service.rs

@@ -3,16 +3,18 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use std::{
-    collections::HashSet,
+    collections::{HashMap, HashSet},
     sync::{Arc, Weak},
 };
 
 use camino::Utf8PathBuf;
 use parking_lot::Mutex;
+use serde::Deserialize;
+use viaduct::Request;
 
 use crate::{
-    config::BaseUrl, storage::Storage, RemoteSettingsClient, RemoteSettingsConfig2,
-    RemoteSettingsContext, RemoteSettingsServer, Result,
+    client::RemoteState, config::BaseUrl, error::Error, storage::Storage, RemoteSettingsClient,
+    RemoteSettingsConfig2, RemoteSettingsContext, RemoteSettingsServer, Result,
 };
 
 /// Internal Remote settings service API
@@ -25,6 +27,7 @@ struct RemoteSettingsServiceInner {
     base_url: BaseUrl,
     bucket_name: String,
     app_context: Option<RemoteSettingsContext>,
+    remote_state: RemoteState,
     /// Weakrefs for all clients that we've created.  Note: this stores the
     /// top-level/public `RemoteSettingsClient` structs rather than `client::RemoteSettingsClient`.
     /// The reason for this is that we return Arcs to the public struct to the foreign code, so we
@@ -51,6 +54,7 @@ impl RemoteSettingsService {
                 base_url,
                 bucket_name,
                 app_context: config.app_context,
+                remote_state: RemoteState::default(),
                 clients: vec![],
             }),
         }
@@ -81,13 +85,30 @@ impl RemoteSettingsService {
         // Make sure we only sync each collection once, even if there are multiple clients
         let mut synced_collections = HashSet::new();
 
-        // TODO: poll the server using `/buckets/monitor/collections/changes/changeset` to fetch
-        // the current timestamp for all collections.  That way we can avoid fetching collections
-        // we know haven't changed and also pass the `?_expected{ts}` param to the server.
+        let mut inner = self.inner.lock();
+        let changes = inner.fetch_changes()?;
+        let change_map: HashMap<_, _> = changes
+            .changes
+            .iter()
+            .map(|c| ((c.collection.as_str(), &c.bucket), c.last_modified))
+            .collect();
+        let bucket_name = inner.bucket_name.clone();
 
-        for client in self.inner.lock().active_clients() {
-            if synced_collections.insert(client.collection_name()) {
-                client.internal.sync()?;
+        for client in inner.active_clients() {
+            let client = &client.internal;
+            let collection_name = client.collection_name();
+            if let Some(client_last_modified) = client.get_last_modified_timestamp()? {
+                if let Some(server_last_modified) = change_map.get(&(collection_name, &bucket_name))
+                {
+                    if client_last_modified != *server_last_modified {
+                        log::trace!("skipping up-to-date collection: {collection_name}");
+                        continue;
+                    }
+                }
+            }
+            if synced_collections.insert(collection_name.to_string()) {
+                log::trace!("syncing collection: {collection_name}");
+                client.sync()?;
             }
         }
         Ok(synced_collections.into_iter().collect())
@@ -134,4 +155,52 @@ impl RemoteSettingsServiceInner {
         });
         active_clients
     }
+
+    fn fetch_changes(&mut self) -> Result<Changes> {
+        let mut url = self.base_url.clone();
+        url.path_segments_mut()
+            .push("buckets")
+            .push("monitor")
+            .push("collections")
+            .push("changes")
+            .push("changeset");
+        // For now, always use `0` for the expected value.  This means we'll get updates based on
+        // the default TTL of 1 hour.
+        //
+        // Eventually, we should add support for push notifications and use the timestamp from the
+        // notification.
+        url.query_pairs_mut().append_pair("_expected", "0");
+        let url = url.into_inner();
+        log::trace!("make_request: {url}");
+        self.remote_state.ensure_no_backoff()?;
+
+        let req = Request::get(url);
+        let resp = req.send()?;
+
+        self.remote_state.handle_backoff_hint(&resp)?;
+
+        if resp.is_success() {
+            Ok(resp.json()?)
+        } else {
+            Err(Error::ResponseError(format!(
+                "status code: {}",
+                resp.status
+            )))
+        }
+    }
+}
+
+/// Data from the changes endpoint
+///
+/// https://remote-settings.readthedocs.io/en/latest/client-specifications.html#endpoints
+#[derive(Debug, Deserialize)]
+struct Changes {
+    changes: Vec<ChangesCollection>,
+}
+
+#[derive(Debug, Deserialize)]
+struct ChangesCollection {
+    collection: String,
+    bucket: String,
+    last_modified: u64,
 }

components/search/src/selector.rs

@@ -1919,6 +1919,18 @@ mod tests {
         selector
     }
 
+    fn mock_changes_endpoint() -> mockito::Mock {
+        mock(
+            "GET",
+            "/v1/buckets/monitor/collections/changes/changeset?_expected=0",
+        )
+        .with_body(response_body_changes())
+        .with_status(200)
+        .with_header("content-type", "application/json")
+        .with_header("etag", "\"1000\"")
+        .create()
+    }
+
     fn response_body() -> String {
         json!({
           "metadata": {
@@ -2021,6 +2033,20 @@ mod tests {
         .to_string()
     }
 
+    fn response_body_changes() -> String {
+        json!({
+          "timestamp": 1000,
+          "changes": [
+            {
+              "collection": "search-config-v2",
+              "bucket": "main",
+              "last_modified": 1000,
+            }
+        ],
+        })
+        .to_string()
+    }
+
     fn response_body_locales() -> String {
         json!({
           "metadata": {
@@ -2129,6 +2155,7 @@ mod tests {
 
     #[test]
     fn test_remote_settings_empty_search_config_records_throws_error() {
+        let changes_mock = mock_changes_endpoint();
         let m = mock(
             "GET",
             "/v1/buckets/main/collections/search-config-v2/changeset?_expected=0",
@@ -2168,11 +2195,13 @@ mod tests {
             .unwrap_err()
             .to_string()
             .contains("No search config v2 records received from remote settings"));
+        changes_mock.expect(1).assert();
         m.expect(1).assert();
     }
 
     #[test]
     fn test_remote_settings_search_config_records_is_none_throws_error() {
+        let changes_mock = mock_changes_endpoint();
         let m1 = mock(
             "GET",
             "/v1/buckets/main/collections/search-config-v2/changeset?_expected=0",
@@ -2197,11 +2226,13 @@ mod tests {
             .unwrap_err()
             .to_string()
             .contains("No search config v2 records received from remote settings"));
+        changes_mock.expect(1).assert();
         m1.expect(1).assert();
     }
 
     #[test]
     fn test_remote_settings_empty_search_config_overrides_filtered_without_error() {
+        let changes_mock = mock_changes_endpoint();
         let m1 = mock(
             "GET",
             "/v1/buckets/main/collections/search-config-v2/changeset?_expected=0",
@@ -2248,12 +2279,14 @@ mod tests {
             "Should have filtered the configuration using an empty search config overrides without causing an error. {:?}",
             result
         );
+        changes_mock.expect(1).assert();
         m1.expect(1).assert();
         m2.expect(1).assert();
     }
 
     #[test]
     fn test_remote_settings_search_config_overrides_records_is_none_throws_error() {
+        let changes_mock = mock_changes_endpoint();
         let m1 = mock(
             "GET",
             "/v1/buckets/main/collections/search-config-v2/changeset?_expected=0",
@@ -2288,12 +2321,14 @@ mod tests {
             .unwrap_err()
             .to_string()
             .contains("No search config overrides v2 records received from remote settings"));
+        changes_mock.expect(1).assert();
         m1.expect(1).assert();
         m2.expect(1).assert();
     }
 
     #[test]
     fn test_filter_with_remote_settings_overrides() {
+        let changes_mock = mock_changes_endpoint();
         let m1 = mock(
             "GET",
             "/v1/buckets/main/collections/search-config-v2/changeset?_expected=0",
@@ -2355,12 +2390,15 @@ mod tests {
             test_engine.clone(),
             "Should have applied the overrides to the matching engine"
         );
+        changes_mock.expect(1).assert();
         m1.expect(1).assert();
         m2.expect(1).assert();
     }
 
     #[test]
     fn test_filter_with_remote_settings() {
+        let changes_mock = mock_changes_endpoint();
+
         let m = mock(
             "GET",
             "/v1/buckets/main/collections/search-config-v2/changeset?_expected=0",
@@ -2468,11 +2506,13 @@ mod tests {
             },
             "Should have selected the private default engine for the matching specific default"
         );
+        changes_mock.expect(1).assert();
         m.expect(1).assert();
     }
 
     #[test]
     fn test_filter_with_remote_settings_negotiate_locales() {
+        let changes_mock = mock_changes_endpoint();
         let m = mock(
             "GET",
             "/v1/buckets/main/collections/search-config-v2/changeset?_expected=0",
@@ -2551,6 +2591,7 @@ mod tests {
             },
             "Should have selected the en-us engine when given another english locale we don't support"
         );
+        changes_mock.expect(1).assert();
         m.expect(1).assert();
     }