[DISCO-3211] Suggest: move to new remote settings API #6598

tiftran · 2025-02-19T07:46:03Z

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes, or
  - There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

tiftran · 2025-02-19T07:47:36Z

components/suggest/src/rs.rs

@@ -181,11 +157,23 @@ impl Record {
    }
 }

+impl From<Record> for RemoteSettingsRecord {


cargo clippy said impl From is preferred over Into

Makes sense, Rust will automatically generate the Into if you define a From.

tiftran · 2025-02-19T07:49:14Z

components/suggest/src/rs.rs

@@ -214,6 +202,15 @@ pub(crate) enum SuggestRecord {
    Geonames,
 }

+impl SuggestRecord {


This feels a little goofy just so that we can use Record to get_attachment, but setting fields to Map::new( ) also felt weird

Yes, slightly goofy but this makes sense to me.

bendk

This looks great. I had some minor suggestions, but the only real issue is how we land this. I'm marking this Request Changes just because I think we should land that preparatory PR I mentioned to make dealing with the breaking changes easier.

bendk · 2025-02-19T15:35:51Z

components/suggest/src/store.rs

@@ -98,19 +98,14 @@ impl SuggestStoreBuilder {
    }

    #[handle_error(Error)]
-    pub fn build(&self) -> SuggestApiResult<Arc<SuggestStore>> {
+    pub fn build(&self, rs_service: &RemoteSettingsService) -> SuggestApiResult<Arc<SuggestStore>> {


One thing that we haven't talked about is that this is going to be a breaking change for all applications, and it might not be so simple to fix. I'm not sure they all have started using the new remote settings API and have a RemoteSettingsService ready to give us.

To make the transition easy, could you open a second PR that changes this signature to something like this:

#[uniffi::method(default(rs_service=None))] pub fn build(&self, rs_service: Option<Arc<RemoteSettingsService>>)

That PR doesn't need to do actually do anything with rs_service. The point is to give us a decent way to merge all the changes:

Merge the above, non-breaking, change.

Get all consumers to start passing in the rs_service

Merge your actual PR, which technically will be a breaking change since rs_service is no longer optional, but it shouldn't be much work to fix the consumer apps. Maybe they won't need any changes at all.

remote_settings_server and remote_settings_bucket_name can be removed from SuggestStoreBuilder too.

Actually why not make rs_service a method on SuggestStoreBuilder instead of an arg to build?

@bdk added a remote_settings_service() method in #6608 yesterday so we should use that to set the service instead of adding a new param to build().

bendk · 2025-02-19T15:37:59Z

components/suggest/src/benchmarks/mod.rs

    std::fs::copy(starter_db_path, &db_path).expect("Error copying starter DB file");
-    SuggestStore::new(&db_path.to_string_lossy(), None).expect("Error building store")
+    SuggestStore::new(&db_path.to_string_lossy(), remote_settings_service)
+        .expect("Error building store")


Nice! Thanks for keeping this code up to date.

bendk · 2025-02-19T15:39:34Z

components/suggest/src/rs.rs

        })
    }

-    fn client_for_collection(&self, collection: Collection) -> &remote_settings::RemoteSettings {
+    fn client_for_collection(&self, collection: Collection) -> &Arc<RemoteSettingsClient> {


Nit: this could just be a &RemoteSettingsClient. The Arc makes me think it might be cloned to create a new reference, but I don't think you ever do that.

bendk · 2025-02-19T15:40:40Z

components/suggest/src/rs.rs

-        };
-        if last_modified != response.last_modified {
-            dao.write_cached_rs_data(collection.name(), &response);
+        let _ = client.sync();


This should be client.sync()?, that way errors get propagated up.

bendk · 2025-02-19T15:41:48Z

components/suggest/src/rs.rs

@@ -181,11 +157,23 @@ impl Record {
    }
 }

+impl From<Record> for RemoteSettingsRecord {


Makes sense, Rust will automatically generate the Into if you define a From.

bendk · 2025-02-19T15:42:53Z

components/suggest/src/rs.rs

@@ -214,6 +202,15 @@ pub(crate) enum SuggestRecord {
    Geonames,
 }

+impl SuggestRecord {


Yes, slightly goofy but this makes sense to me.

bendk · 2025-02-19T15:43:40Z

components/suggest/src/rs.rs

+    fn to_json_map(&self) -> Map<String, Value> {
+        match serde_json::to_value(self) {
+            Ok(Value::Object(map)) => map,
+            _ => Map::new(),


Maybe this branch could be unreachable!(). I think we can safely say that serde will always be able to to deserialize to Value::Object.

tiftran · 2025-02-19T19:25:22Z

I see that there are a couple of PRs that will slightly change this one, I will wait and see when those merge and change accordingly

tiftran · 2025-02-26T05:41:12Z

I see that the other PRs are merged, so i rebased and made changes 🙂

0c0w3

IIUC we no longer need our RS cache in suggest, right? And that's why this removes the RS-cache-related methods from SuggestDao. It looks like rs_cache is still referenced in schema.rs, so we'll also need to:

remove those references
add a schema migration that removes the rs_cache table

I think we need to do this before vendoring, so either in this PR or in a fast follow-up.

0c0w3 · 2025-02-26T16:59:44Z

components/suggest/src/rs.rs

        // For now, handle the cache manually.  Once 6328 is merged, we should be able to delegate
        // this to remote_settings.


This comment can be removed.

components/suggest/src/rs.rs

0c0w3 · 2025-02-26T17:25:02Z

components/suggest/src/rs.rs

        match &record.attachment {
-            Some(a) => Ok(self
+            Some(_a) => Ok(self


Nit: Some(_)

0c0w3 · 2025-02-26T17:37:01Z

components/suggest/src/store.rs

@@ -98,19 +98,14 @@ impl SuggestStoreBuilder {
    }

    #[handle_error(Error)]
-    pub fn build(&self) -> SuggestApiResult<Arc<SuggestStore>> {
+    pub fn build(&self, rs_service: &RemoteSettingsService) -> SuggestApiResult<Arc<SuggestStore>> {


@bdk added a remote_settings_service() method in #6608 yesterday so we should use that to set the service instead of adding a new param to build().

0c0w3 · 2025-02-26T17:57:26Z

components/suggest/src/rs.rs

    }

-    fn download_attachment(&self, record: &Record) -> Result<Vec<u8>> {
+    fn download_attachment(&self, record: Record) -> Result<Vec<u8>> {


This can remain &Record AFAICT, no need to pass by value

oh i ran into trouble yesterday and this is my rust newbie solution. get_attachment expects a &RemoteSettingRecord and .into() wasn't helping. Since we're suggesting removing the struct below in a follow up, this would resolve itself?

If you add back the & in both places in rs.rs and then fix the errors in the other files, it should work (it does for me). I do think it's worth fixing so we're not unnecessarily creating clones.

let me try that again

0c0w3 · 2025-02-26T17:59:18Z

components/suggest/src/rs.rs

@@ -181,11 +157,23 @@ impl Record {
    }
 }

+impl From<Record> for RemoteSettingsRecord {
+    fn from(record: Record) -> Self {


Could we get a bug/ticket on file for removing our Record struct?

makes sense it's more or less the same as RemoteSettingsRecord

The key difference is the payload field. RemoteSettingsRecord stores a JSON value there, since it doesn't know the specifics of how it's going to be used. suggest::rs::Record stores the deserialized SuggestRecord there. So, if a function inputs suggest::rs::Record it can access the fields more conveniently and it doesn't have to deal with deserialization errors.

It's still a good idea and I'm pretty sure there's a way to remove some code duplication. Maybe RemoteSettingsRecord could be generic over its payload field. It's just not going to be a trivial change.

created a jira in our maintenance epic for this

tiftran · 2025-02-26T20:50:42Z

components/suggest/src/store.rs

+            }))
+        } else {
+            Err(Error::RemoteSettings(RemoteSettingsError::Other {
+                reason: "Unable to create client".to_string(),


Im unsure what's the best way to handle this

Yeah this makes sense to me and it's similar to how we return an error if data_path is none, right above. You could use that as an example to rewrite this without an if statement:

let rs_service = inner.remote_settings_service.clone().ok_or_else(|| { Error::RemoteSettings(RemoteSettingsError::Other { reason: "Unable to create client".to_string(), }) })?; Ok(Arc::new(SuggestStore { inner: SuggestStoreInner::new( data_path, extensions_to_load, SuggestRemoteSettingsClient::new(&rs_service)?, ), }))

I'll need to defer to @bendk if all this is kosher or not w/r/t Arc. SuggestRemoteSettingsClient should probably take an Arc<RemoteSettingsService> instead of &RemoteSettingsService, right? Like how SuggestStore does.

The error message should be something like "remote_settings_service not specified" (similar to the data_path error message)

Agreed that that error message would be better. I think everything else is fine.

It's okay that SuggestRemoteStetingsClient::new. inputs &RemoteSettingsService, since it only uses that that to call make_client(), then stores the returned Arc<RemoteSettingsClient> instances. In general, I think Rust is going to keep us honest here and throw up compile errors if we do something wrong.

tiftran · 2025-02-26T20:51:06Z

okie doke, i think i've addressed the latest feedback

0c0w3 · 2025-02-26T21:45:26Z

components/suggest/src/schema.rs

@@ -616,6 +611,12 @@ impl ConnectionInitializer for SuggestConnectionInitializer<'_> {
                )?;
                Ok(())
            }
+            32 => {
+                // Drop rs_cache since it's no longer needed.
+                clear_database(tx)?;


No need to clear the DB I don't think?

0c0w3 · 2025-02-26T21:49:35Z

components/suggest/src/schema.rs

@@ -819,11 +815,6 @@ PRAGMA user_version=16;
            0,
            "ingested_records should be empty"
        );
-        assert_eq!(


A few code comments in this function (including the function's doc comment) talk about rs_cache so could you update those too please?

0c0w3 · 2025-02-26T21:50:54Z

components/suggest/src/store.rs

        // When #6607 lands, this will set the remote settings service.
        // For now, it just exists so we can move consumers over to the new API ahead of time.


This comment can be removed but I'll defer to @bendk on if/how this patch breaks mobile consumers.

Yeah, this comment can be removed. Assuming we get mobile consumers using the new API from #6607 ahead of time, then there's no breakage.

0c0w3 · 2025-02-26T22:10:54Z

components/suggest/src/store.rs

+            }))
+        } else {
+            Err(Error::RemoteSettings(RemoteSettingsError::Other {
+                reason: "Unable to create client".to_string(),


Yeah this makes sense to me and it's similar to how we return an error if data_path is none, right above. You could use that as an example to rewrite this without an if statement:

let rs_service = inner.remote_settings_service.clone().ok_or_else(|| { Error::RemoteSettings(RemoteSettingsError::Other { reason: "Unable to create client".to_string(), }) })?; Ok(Arc::new(SuggestStore { inner: SuggestStoreInner::new( data_path, extensions_to_load, SuggestRemoteSettingsClient::new(&rs_service)?, ), }))

I'll need to defer to @bendk if all this is kosher or not w/r/t Arc. SuggestRemoteSettingsClient should probably take an Arc<RemoteSettingsService> instead of &RemoteSettingsService, right? Like how SuggestStore does.

0c0w3

Thanks Tif!

0c0w3 · 2025-02-27T19:03:22Z

components/suggest/src/store.rs

@@ -578,8 +569,7 @@ where
        // For each collection, fetch all records
        for (collection, record_types) in record_types_by_collection {
            breadcrumb!("Ingesting collection {}", collection.name());
-            let records =
-                write_scope.write(|dao| self.settings_client.get_records(collection, dao))?;
+            let records = write_scope.write(|_| self.settings_client.get_records(collection))?;


let records = self.settings_client.get_records(collection)?;

No need for the write scope anymore since we don't need to write to the DB when getting records. (Sorry for not catching that before)

0c0w3 · 2025-02-27T19:11:30Z

components/suggest/src/store.rs

        let records = writer
-            .write(|dao| {
+            .write(|_| {
                self.settings_client
-                    .get_records(ingest_record_type.collection(), dao)
+                    .get_records(ingest_record_type.collection())
            })
            .unwrap();


Here too:

let records = self .settings_client .get_records(ingest_record_type.collection()) .unwrap();

tiftran requested a review from a team February 19, 2025 07:46

tiftran commented Feb 19, 2025

View reviewed changes

bendk requested changes Feb 19, 2025

View reviewed changes

tiftran force-pushed the rs-new-client branch from 93bd3d2 to 4e99448 Compare February 19, 2025 19:02

bendk mentioned this pull request Feb 19, 2025

[DISCO-3212] Switch Relevancy to new remote settings API #6601

Open

5 tasks

[DISCO-3211] Suggest: move to new remote settings API

be662ea

tiftran force-pushed the rs-new-client branch from 4e99448 to 5ddb547 Compare February 26, 2025 04:48

rebase changes

f584af2

tiftran force-pushed the rs-new-client branch from 5ddb547 to f584af2 Compare February 26, 2025 05:10

tiftran requested review from bendk and 0c0w3 February 26, 2025 05:40

0c0w3 reviewed Feb 26, 2025

View reviewed changes

components/suggest/src/rs.rs Show resolved Hide resolved

0c0w3 reviewed Feb 26, 2025

View reviewed changes

tiftran added 2 commits February 26, 2025 11:45

feedback changes

24ded47

clippy fix

16a78e4

tiftran commented Feb 26, 2025

View reviewed changes

tiftran requested a review from 0c0w3 February 26, 2025 20:51

0c0w3 reviewed Feb 26, 2025

View reviewed changes

feedback changes

fd74e93

tiftran requested a review from 0c0w3 February 27, 2025 18:47

0c0w3 approved these changes Feb 27, 2025

View reviewed changes

feedback changes

11b0606

		// For now, handle the cache manually. Once 6328 is merged, we should be able to delegate
		// this to remote_settings.

		// When #6607 lands, this will set the remote settings service.
		// For now, it just exists so we can move consumers over to the new API ahead of time.

[DISCO-3211] Suggest: move to new remote settings API #6598

Are you sure you want to change the base?

[DISCO-3211] Suggest: move to new remote settings API #6598

Conversation

tiftran commented Feb 19, 2025

Pull Request checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiftran commented Feb 19, 2025

tiftran commented Feb 26, 2025

0c0w3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiftran Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiftran commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0c0w3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiftran Feb 26, 2025 •

edited

Loading