Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

feat(suggest): Add score field to suggestion #119

Merged
merged 2 commits into from
Sep 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.lock

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

6 changes: 5 additions & 1 deletion merino-adm/src/remote_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use http::Uri;
use lazy_static::lazy_static;
use merino_settings::Settings;
use merino_suggest::{
SetupError, SuggestError, Suggestion, SuggestionProvider, SuggestionRequest, SuggestionResponse,
Proportion, SetupError, SuggestError, Suggestion, SuggestionProvider, SuggestionRequest,
SuggestionResponse,
};
use remote_settings_client::client::FileStorage;
use serde::Deserialize;
Expand Down Expand Up @@ -195,6 +196,7 @@ impl RemoteSettingsSuggester {
is_sponsored: !NON_SPONSORED_IAB_CATEGORIES
.contains(&adm_suggestion.iab_category.as_str()),
icon: icon_url,
score: Proportion::from(0.2),
});
for keyword in adm_suggestion.keywords {
suggestions.insert(keyword, merino_suggestion.clone());
Expand Down Expand Up @@ -365,6 +367,7 @@ mod tests {
provider: "test".to_string(),
is_sponsored: false,
icon: Uri::from_static("https://en.wikipedia.org/favicon.ico"),
score: Proportion::zero(),
}),
);
let rs_suggester = RemoteSettingsSuggester { suggestions };
Expand Down Expand Up @@ -404,6 +407,7 @@ mod tests {
provider: "test".to_string(),
is_sponsored: false,
icon: Uri::from_static("https://en.wikipedia.org/favicon.ico"),
score: Proportion::zero(),
}),
);
let rs_suggester = RemoteSettingsSuggester { suggestions };
Expand Down
1 change: 1 addition & 0 deletions merino-cache/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ uuid = "0.8"
http = "^0.2"
proptest = "^1"
fake = "2.4"
tokio = { version = "1", features = ["macros"] }
4 changes: 3 additions & 1 deletion merino-cache/src/redis/domain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ impl FromRedisValue for RedisTtl {
mod tests {
use anyhow::{anyhow, Result};
use http::Uri;
use merino_suggest::Suggestion;
use merino_suggest::{Proportion, Suggestion};
use proptest::prelude::*;
use redis::{FromRedisValue, ToRedisArgs};

Expand All @@ -120,6 +120,7 @@ mod tests {
provider: "One Inc.".to_string(),
is_sponsored: true,
icon: Uri::from_static("https://example.com/icon/one.png"),
score: Proportion::zero(),
}];

let mut serialized = SERIALIZATION_VERSION.to_vec();
Expand Down Expand Up @@ -187,6 +188,7 @@ mod tests {
provider: "One Inc.".to_string(),
is_sponsored: true,
icon: Uri::from_static("https://example.com/icon/one.png"),
score: Proportion::zero(),
}];

let val = RedisSuggestions(original_suggestions.clone()).to_redis_args();
Expand Down
7 changes: 4 additions & 3 deletions merino-cache/src/redis/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ where
#[cfg(test)]
mod test {
use http::Uri;
use merino_suggest::Suggestion;
use merino_suggest::{Proportion, Suggestion};

use super::*;

Expand Down Expand Up @@ -434,6 +434,7 @@ mod test {
provider: text.clone(),
is_sponsored: false,
icon: uri.clone(),
score: Proportion::zero(),
};
let suggestion2 = Suggestion {
id: 5678,
Expand Down Expand Up @@ -476,7 +477,7 @@ mod test {
.query_async::<redis::aio::ConnectionManager, String>(&mut redis_connection)
.await
.unwrap();
assert_eq!(res1, res2);
assert_eq!(res1, res2, "cached values should match");

// trying to write with an new lock should work and release the lock.
assert!(rlock
Expand All @@ -491,7 +492,7 @@ mod test {
.query_async::<redis::aio::ConnectionManager, String>(&mut redis_connection)
.await
.unwrap();
assert_ne!(res1, res2);
assert_ne!(res1, res2, "cached values should not match");

Ok(())
}
Expand Down
1 change: 1 addition & 0 deletions merino-integration-tests/src/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ async fn test_expected_fields(TestingTools { test_client, .. }: TestingTools) ->
"advertiser",
"is_sponsored",
"icon",
"score",
];
dbg!(&keys);
for expected_key in expected_keys {
Expand Down
4 changes: 3 additions & 1 deletion merino-suggest/src/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ use fake::{Fake, Faker};
use merino_settings::Settings;

use crate::{
SetupError, SuggestError, Suggestion, SuggestionProvider, SuggestionRequest, SuggestionResponse,
Proportion, SetupError, SuggestError, Suggestion, SuggestionProvider, SuggestionRequest,
SuggestionResponse,
};

/// A toy suggester to test the system.
Expand Down Expand Up @@ -49,6 +50,7 @@ impl<'a> SuggestionProvider<'a> for DebugProvider {
Ok(SuggestionResponse::new(vec![Suggestion {
title: json,
provider: "Merino::Debug".into(),
score: Proportion::zero(),
..Faker.fake()
}]))
}
Expand Down
140 changes: 140 additions & 0 deletions merino-suggest/src/domain.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
//! Datatypes to better represent the domain of Merino.

use anyhow::ensure;
use rand::distributions::{Distribution, Standard};
use serde::{de, Deserialize, Serialize};
use std::convert::{TryFrom, TryInto};
use std::fmt::Debug;

/// Represents a value from 0.0 to 1.0, inclusive. That is: a portion of
/// something that cannot be negative or exceed 100%.
///
/// Stored internally as a u32.
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct Proportion(u32);

impl Proportion {
/// The lowest value for a portion, corresponding to 0%.
pub fn zero() -> Self {
Proportion(0)
}

/// The highest value for a portion, corresponding to 100%.
pub fn one() -> Self {
Proportion(u32::MAX)
}

/// Converts a float value into a Proportion. Panics if the value is not
/// between zero and one.
///
/// This is not implemented using [`std::config::From`] because you cannot
/// implement both Try and TryFrom for the same pair of types, due to a
/// blanket `impl TryFor<T> for U where U: Try<T>`.
pub fn from<T>(v: T) -> Self
where
T: TryInto<Self>,
<T as TryInto<Self>>::Error: Debug,
{
v.try_into().unwrap()
}
}

macro_rules! impl_for_float {
($type: ty) => {
impl TryFrom<$type> for Proportion {
type Error = anyhow::Error;

fn try_from(v: $type) -> Result<Self, Self::Error> {
ensure!(!v.is_infinite(), "v cannot be infinite");
ensure!(v >= 0.0, "v must be positive");
ensure!(v <= 1.0, "v cannot be greater than 1");

Ok(Self((v * (u32::MAX as $type)) as u32))
}
}

impl From<Proportion> for $type {
fn from(portion: Proportion) -> $type {
(portion.0 as $type) / (u32::MAX as $type)
}
}

impl From<&Proportion> for $type {
fn from(portion: &Proportion) -> $type {
(portion.0 as $type) / (u32::MAX as $type)
}
}
};
}

impl_for_float!(f32);
impl_for_float!(f64);

impl Distribution<Proportion> for Standard {
fn sample<R: rand::Rng + ?Sized>(&self, rng: &mut R) -> Proportion {
Proportion(rng.gen())
}
}

impl Serialize for Proportion {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
serializer.serialize_f64(self.into())
}
}

impl<'de> Deserialize<'de> for Proportion {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
/// Visitor for deserializing a Proportion
struct Visitor;

impl<'de> de::Visitor<'de> for Visitor {
type Value = Proportion;

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(formatter, "value between 0.0 and 1.0")
}

fn visit_i64<E>(self, v: i64) -> Result<Self::Value, E>
where
E: de::Error,
{
if v >= 0 {
self.visit_u64(v as u64)
} else {
Err(de::Error::invalid_value(de::Unexpected::Signed(v), &self))
}
}

// u8, u16, and u32 delegate to this
fn visit_u64<E>(self, v: u64) -> Result<Self::Value, E>
where
E: de::Error,
{
if v == 0 {
Ok(Proportion::zero())
} else if v == 1 {
Ok(Proportion::one())
} else {
Err(de::Error::invalid_value(de::Unexpected::Unsigned(v), &self))
}
}

// f32 delegates to this
fn visit_f64<E>(self, v: f64) -> Result<Self::Value, E>
where
E: de::Error,
{
v.try_into()
.map_err(|_err| de::Error::invalid_value(de::Unexpected::Float(v), &self))
}
}

deserializer.deserialize_any(Visitor)
}
}
13 changes: 12 additions & 1 deletion merino-suggest/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

mod debug;
pub mod device_info;
mod domain;
mod multi;
mod wikifruit;

Expand All @@ -13,6 +14,7 @@ use std::hash::Hash;
use std::ops::Range;
use std::time::Duration;

use crate::device_info::DeviceInfo;
use async_trait::async_trait;
use fake::{
faker::{
Expand All @@ -27,7 +29,7 @@ use serde_with::{serde_as, DisplayFromStr};
use thiserror::Error;

pub use crate::debug::DebugProvider;
use crate::device_info::DeviceInfo;
pub use crate::domain::Proportion;
pub use crate::multi::Multi;
pub use crate::wikifruit::WikiFruit;

Expand Down Expand Up @@ -195,6 +197,14 @@ pub struct Suggestion {
/// The URL of the icon to show along side this suggestion.
#[serde_as(as = "DisplayFromStr")]
pub icon: Uri,

/// A value used to compare suggestions. When choosing a suggestion to show
/// the user, higher scored suggestions are preferred. Should range from 0.0
/// to 1.0.
///
/// Note that Firefox uses a static value of 0.2 for Remote Settings
/// provided suggestions.
pub score: Proportion,
}

impl<'a, F> fake::Dummy<F> for Suggestion {
Expand All @@ -209,6 +219,7 @@ impl<'a, F> fake::Dummy<F> for Suggestion {
provider: Words(2..4).fake_with_rng::<Vec<String>, R>(rng).join(" "),
is_sponsored: rng.gen(),
icon: fake_example_url(rng),
score: rng.gen(),
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion merino-suggest/src/wikifruit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ use http::Uri;
use merino_settings::Settings;

use crate::{
SetupError, SuggestError, Suggestion, SuggestionProvider, SuggestionRequest, SuggestionResponse,
domain::Proportion, SetupError, SuggestError, Suggestion, SuggestionProvider,
SuggestionRequest, SuggestionResponse,
};

/// A toy suggester to test the system.
Expand Down Expand Up @@ -56,6 +57,7 @@ impl<'a> SuggestionProvider<'a> for WikiFruit {
provider: "Merino::WikiFruit".to_string(),
is_sponsored: false,
icon: Uri::from_static("https://en.wikipedia.org/favicon.ico"),
score: Proportion::zero(),
}),
"banana" => Some(Suggestion {
id: 1,
Expand All @@ -67,6 +69,7 @@ impl<'a> SuggestionProvider<'a> for WikiFruit {
provider: "Merino::WikiFruit".to_string(),
is_sponsored: false,
icon: Uri::from_static("https://en.wikipedia.org/favicon.ico"),
score: Proportion::zero(),
}),
"cherry" => Some(Suggestion {
id: 1,
Expand All @@ -78,6 +81,7 @@ impl<'a> SuggestionProvider<'a> for WikiFruit {
provider: "Merino::WikiFruit".to_string(),
is_sponsored: false,
icon: Uri::from_static("https://en.wikipedia.org/favicon.ico"),
score: Proportion::zero(),
}),
_ => None,
};
Expand Down
2 changes: 2 additions & 0 deletions merino-web/src/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ impl<'a> Serialize for SuggestionWrapper<'a> {
is_sponsored: bool,
icon: String,
advertiser: &'a str,
score: f32,
}

let provider = &self.0.provider;
Expand All @@ -176,6 +177,7 @@ impl<'a> Serialize for SuggestionWrapper<'a> {
is_sponsored: self.0.is_sponsored,
icon: self.0.icon.to_string(),
advertiser: provider,
score: self.0.score.into(),
};

generated.serialize(serializer)
Expand Down
4 changes: 4 additions & 0 deletions merino/src/docs/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ following keys:
small square image, suitable to be included inline with the text, such as a
site's favicon.

- `score` - A value between 0.0 and 1.0 used to compare suggestions. When
choosing a suggestion to show the user, higher scored suggestions are
preferred.

### Response Headers

Responses will carry standard HTTP caching headers that indicate the validity of
Expand Down