Skip to content

Commit

Permalink
[DISCO-3212] Switch Relevancy to new remote settings API
Browse files Browse the repository at this point in the history
  • Loading branch information
bendk committed Feb 25, 2025
1 parent 5974fdc commit 5dba6b5
Show file tree
Hide file tree
Showing 8 changed files with 68 additions and 50 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 6 additions & 19 deletions components/relevancy/src/ingest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,10 @@ pub fn ensure_interest_data_populated<C: RelevancyRemoteSettingsClient>(
fn fetch_interest_data_inner<C: RelevancyRemoteSettingsClient>(
client: C,
) -> Result<Vec<(Interest, UrlHash)>> {
let remote_settings_response = client.get_records()?;
let mut result = vec![];

for record in remote_settings_response.records {
let attachment_data = match &record.attachment {
None => return Err(Error::FetchInterestDataError),
Some(a) => client.get_attachment(&a.location)?,
};
for record in client.get_records()? {
let attachment_data = client.get_attachment(&record)?;
let interest = get_interest(&record)?;
let urls = get_hash_urls(attachment_data)?;
result.extend(std::iter::repeat(interest).zip(urls));
Expand Down Expand Up @@ -98,7 +94,6 @@ mod test {
use std::{cell::RefCell, collections::HashMap};

use anyhow::Context;
use remote_settings::RemoteSettingsResponse;
use serde_json::json;

use super::*;
Expand Down Expand Up @@ -160,20 +155,12 @@ mod test {
}

impl RelevancyRemoteSettingsClient for SnapshotSettingsClient {
fn get_records(&self) -> Result<RemoteSettingsResponse> {
let records = self.snapshot.borrow().records.clone();
let last_modified = records
.iter()
.map(|record: &RemoteSettingsRecord| record.last_modified)
.max()
.unwrap_or(0);
Ok(RemoteSettingsResponse {
records,
last_modified,
})
fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>> {
Ok(self.snapshot.borrow().records.clone())
}

fn get_attachment(&self, location: &str) -> Result<Vec<u8>> {
fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result<Vec<u8>> {
let location = record.attachment.as_ref().unwrap().location.as_str();
Ok(self
.snapshot
.borrow()
Expand Down
15 changes: 11 additions & 4 deletions components/relevancy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,14 @@ pub mod url_hash;

use rand_distr::{Beta, Distribution};

use std::sync::Arc;

use parking_lot::Mutex;
use remote_settings::{RemoteSettingsClient, RemoteSettingsService};

pub use db::RelevancyDb;
pub use error::{ApiResult, Error, RelevancyApiError, Result};
pub use interest::{Interest, InterestVector};
use parking_lot::Mutex;
pub use ranker::score;

use error_support::handle_error;
Expand All @@ -35,7 +39,7 @@ uniffi::setup_scaffolding!();

#[derive(uniffi::Object)]
pub struct RelevancyStore {
inner: RelevancyStoreInner<remote_settings::RemoteSettings>,
inner: RelevancyStoreInner<Arc<RemoteSettingsClient>>,
}

/// Top-level API for the Relevancy component
Expand All @@ -47,9 +51,12 @@ impl RelevancyStore {
/// This is non-blocking since databases and other resources are lazily opened.
#[uniffi::constructor]
#[handle_error(Error)]
pub fn new(db_path: String) -> ApiResult<Self> {
pub fn new(db_path: String, remote_settings: Arc<RemoteSettingsService>) -> ApiResult<Self> {
Ok(Self {
inner: RelevancyStoreInner::new(db_path, rs::create_client()?),
inner: RelevancyStoreInner::new(
db_path,
remote_settings.make_client(rs::REMOTE_SETTINGS_COLLECTION.to_string())?,
),
})
}

Expand Down
49 changes: 24 additions & 25 deletions components/relevancy/src/rs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
*/

use crate::{Error, Result};
use remote_settings::{
RemoteSettings, RemoteSettingsConfig, RemoteSettingsResponse, RemoteSettingsServer,
};
use remote_settings::{RemoteSettingsClient, RemoteSettingsRecord};
use serde::Deserialize;
/// The Remote Settings collection name.
pub(crate) const REMOTE_SETTINGS_COLLECTION: &str = "content-relevance";
Expand All @@ -16,45 +14,46 @@ pub(crate) const REMOTE_SETTINGS_COLLECTION: &str = "content-relevance";
/// This trait lets tests use a mock client.
pub(crate) trait RelevancyRemoteSettingsClient {
/// Fetches records from the Suggest Remote Settings collection.
fn get_records(&self) -> Result<RemoteSettingsResponse>;
fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>>;

/// Fetches a record's attachment from the Suggest Remote Settings
/// collection.
fn get_attachment(&self, location: &str) -> Result<Vec<u8>>;
fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result<Vec<u8>>;
}

impl RelevancyRemoteSettingsClient for remote_settings::RemoteSettings {
fn get_records(&self) -> Result<RemoteSettingsResponse> {
Ok(remote_settings::RemoteSettings::get_records(self)?)
impl RelevancyRemoteSettingsClient for RemoteSettingsClient {
fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>> {
self.sync()?;
Ok(self
.get_records(false)
.expect("RemoteSettingsClient::get_records() returned None after `sync()` called"))
}

fn get_attachment(&self, location: &str) -> Result<Vec<u8>> {
Ok(remote_settings::RemoteSettings::get_attachment(
self, location,
)?)
fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result<Vec<u8>> {
Ok(self.get_attachment(record)?)
}
}

impl<T: RelevancyRemoteSettingsClient> RelevancyRemoteSettingsClient for &T {
fn get_records(&self) -> Result<RemoteSettingsResponse> {
fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>> {
(*self).get_records()
}

fn get_attachment(&self, location: &str) -> Result<Vec<u8>> {
(*self).get_attachment(location)
fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result<Vec<u8>> {
(*self).get_attachment(record)
}
}

pub fn create_client() -> Result<RemoteSettings> {
Ok(RemoteSettings::new(RemoteSettingsConfig {
collection_name: REMOTE_SETTINGS_COLLECTION.to_string(),
server: Some(RemoteSettingsServer::Prod),
server_url: None,
bucket_name: None,
})?)
impl<T: RelevancyRemoteSettingsClient> RelevancyRemoteSettingsClient for std::sync::Arc<T> {
fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>> {
(**self).get_records()
}

fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result<Vec<u8>> {
(**self).get_attachment(record)
}
}

/// A record in the Relevancy Remote Settings collection.
#[derive(Clone, Debug, Deserialize)]
pub struct RelevancyRecord {
#[allow(dead_code)]
Expand Down Expand Up @@ -115,11 +114,11 @@ pub mod test {
pub struct NullRelavancyRemoteSettingsClient;

impl RelevancyRemoteSettingsClient for NullRelavancyRemoteSettingsClient {
fn get_records(&self) -> Result<RemoteSettingsResponse> {
fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>> {
panic!("NullRelavancyRemoteSettingsClient::get_records was called")
}

fn get_attachment(&self, _location: &str) -> Result<Vec<u8>> {
fn get_attachment(&self, _record: &RemoteSettingsRecord) -> Result<Vec<u8>> {
panic!("NullRelavancyRemoteSettingsClient::get_records was called")
}
}
Expand Down
2 changes: 1 addition & 1 deletion components/remote_settings/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use crate::{ApiResult, Error, RemoteSettingsContext, Result};
/// This is the version used in the new API, hence the `2` at the end. The plan is to move
/// consumers to the new API, remove the RemoteSettingsConfig struct, then remove the `2` from this
/// name.
#[derive(Debug, Clone, uniffi::Record)]
#[derive(Debug, Default, Clone, uniffi::Record)]
pub struct RemoteSettingsConfig2 {
/// The Remote Settings server to use. Defaults to [RemoteSettingsServer::Prod],
#[uniffi(default = None)]
Expand Down
5 changes: 5 additions & 0 deletions components/remote_settings/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,11 @@ impl RemoteSettingsClient {
pub fn get_attachment(&self, record: &RemoteSettingsRecord) -> ApiResult<Vec<u8>> {
self.internal.get_attachment(record)
}

#[handle_error(Error)]
pub fn sync(&self) -> ApiResult<()> {
self.internal.sync()
}
}

impl RemoteSettingsClient {
Expand Down
1 change: 1 addition & 0 deletions examples/cli-support/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ license = "MPL-2.0"
[dependencies]
anyhow = "1.0"
fxa-client = { path = "../../components/fxa-client" }
remote_settings = { path = "../../components/remote_settings" }
sync_manager = { path = "../../components/sync_manager" }
log = "0.4"
sync15 = { path = "../../components/sync15", features=["sync-client"] }
Expand Down
20 changes: 19 additions & 1 deletion examples/cli-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
#![allow(unknown_lints)]
#![warn(rust_2018_idioms)]

use std::path::{Path, PathBuf};
use std::{
path::{Path, PathBuf},
sync::Arc,
};

use remote_settings::{RemoteSettingsConfig2, RemoteSettingsService};

pub mod fxa_creds;
pub mod prompt;
Expand Down Expand Up @@ -63,3 +68,16 @@ pub fn workspace_root_dir() -> PathBuf {
let cargo_toml_path = Path::new(std::str::from_utf8(&cargo_output).unwrap().trim());
cargo_toml_path.parent().unwrap().to_path_buf()
}

pub fn remote_settings_service() -> Arc<RemoteSettingsService> {
Arc::new(
RemoteSettingsService::new(
data_path(Some("remote-settings"))
.to_string_lossy()
.to_string(),
RemoteSettingsConfig2::default(),
None,
)
.expect("Error creating RemoteSettingsService"),
)
}

0 comments on commit 5dba6b5

Please sign in to comment.