Bug 1938924 - Add support for search-config-overrides-v2 to the Rust…

[mandysGit]

… based search engine selector.

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.

[mandysGit]

I updated the patch to fix the failing tests. I didn't notice them when I first push this up.

[Standard8]

Thank you, this is a good start and looks like it will fix some more of the desktop tests 🎉

A few things to consider that I've added in-line.

[Standard8]

I think we should set this up such that if an application calls use_remote_settings_server with apply_engine_overrides == true, then we will create a remote settings client (in use_remote_settings_server) and then use the client here.

It isn't much extra to do, and if we do enable this on android in future, then we won't need to do extra work for them.

[mandysGit]

If I'm understanding correctly, and I pass the search-config-overrides-v2 client to filter_engine_configuration_impl() Then I have to convert the overrides record to strings and then deserialize them again to the records we can use, similar to the code below.
I want to clarify is this what you're suggesting or am I over complicating it and there's a simpler way?

    fn filter_records(
        &self,
        user_environment: &SearchUserEnvironment,
        _overrides: Option<Vec<JSONOverridesRecord>>,
    ) -> Result<FilterRecordsResult, Error> {
        let mut engines = Vec::new();
        let mut default_engines_record = None;
        let mut engine_orders_record = None;

        for record in self {
            // TODO: Bug 1947241 - Find a way to avoid having to serialise the records
            // back to strings and then deserilise them into the records that we want.
            let stringified = serde_json::to_string(&record.fields)?;
            match record.fields.get("recordType") {
                Some(val) if *val == "engine" => {
                    let engine_config: Option<JSONEngineRecord> =
                        serde_json::from_str(&stringified)?;
                    if let Some(engine_config) = engine_config {
                        let result =
                            maybe_extract_engine_config(user_environment, Box::new(engine_config));
                        engines.extend(result);
                    }
                }
                Some(val) if *val == "defaultEngines" => {
                    default_engines_record = serde_json::from_str(&stringified)?;
                }
                Some(val) if *val == "engineOrders" => {
                    engine_orders_record = serde_json::from_str(&stringified)?;
                }
                // These cases are acceptable - we expect the potential for new
                // record types/options so that we can be flexible.
                Some(_val) => {}
                None => {}
            }
        }

        Ok(FilterRecordsResult {
            engines,
            default_engines_record,
            engine_orders_record,
        })
    }

[Standard8]

At the moment we'll have to do it the convert to string & then back to the records in a similar way. At some point we'll make that easier for ourselves.

[Standard8]

I can't add this above here, but please can we add a test similar to test_filter_engine_configuration_throws_without_config but for the overrides.

[Standard8]

It'd be nice to add a unit test for this - something fairly simple that checks we merge the parameters correctly (and don't change the telemetry suffix if none is supplied).