[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?

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 🙂

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tiftran commented Feb 19, 2025

tiftran Feb 19, 2025

bendk Feb 19, 2025

tiftran Feb 19, 2025

bendk Feb 19, 2025

bendk left a comment

bendk Feb 19, 2025

0c0w3 Feb 22, 2025

0c0w3 Feb 22, 2025

bendk Feb 19, 2025

bendk Feb 19, 2025

bendk Feb 19, 2025

bendk Feb 19, 2025

bendk Feb 19, 2025

bendk Feb 19, 2025

tiftran commented Feb 19, 2025

tiftran commented Feb 26, 2025

[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

tiftran commented Feb 19, 2025

tiftran commented Feb 26, 2025