Skip to content

Commit 6a007c9

Browse files
0c0w30c0w3
committed
Bug 1960786 - Dismiss AMP suggestions by full keyword instead of URL
This adds `SuggestStore::dismiss`, which takes a `Suggestion`, and deprecates `SuggestStore::dismiss_suggestion`, which takes a URL. It also makes one other substantive change: If an AMP suggestion doesn't have `full_keywords` defined in its RS data, then `Suggestion.full_keyword` will now be an empty string instead of a string computed at query time from its keywords. The problem with using such a computed "full keyword" as the dismissal key is that it's not stable. It's just the prefix of one of the suggestion's keywords up to the next full word after the query string, so it changes the more you type. When the RS data doesn't have a `full_keywords`, I set `Suggestion.full_keyword` to an empty string and then fall back to the suggestion's URL as the dismissal key. In practice, all AMP suggestions have full keywords in the RS data (I checked), and going forward with the new AMP API that won't change, so we shouldn't ever hit this to begin with. Other possible ways to address this: * Change the query-time full keyword calculation so that the longest matching keyword is used as the full keyword. That might actually be good for UX too, maybe not in all cases. * Add a `Suggestion` member called `is_full_keyword_from_db` or something (like the similarly named variable in the AMP fetch function), and then we'd use the URL as the dismissal key if `is_full_keyword_from_db` is false. These are both more complex than setting `full_keywords` to an empty string and using that to tell if the dismissal key should be the URL, and since we shouldn't ever hit this anyway, I didn't bother.
1 parent 70e8e4f commit 6a007c9

File tree

6 files changed

+157
-55
lines changed

6 files changed

+157
-55
lines changed

‎components/suggest/src/db.rs‎

Lines changed: 7 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,12 @@ impl<'a> SuggestDao<'a> {
308308
s.provider = :provider
309309
AND k.keyword = :keyword
310310
{where_extra}
311-
AND NOT EXISTS (SELECT 1 FROM dismissed_suggestions WHERE url=s.url)
311+
AND NOT EXISTS (
312+
-- For AMP suggestions dismissed with the deprecated URL-based dismissal API,
313+
-- `dismissed_suggestions.url` will be the suggestion URL. With the new
314+
-- `Suggestion`-based API, it will be the full keyword.
315+
SELECT 1 FROM dismissed_suggestions WHERE url IN (fk.full_keyword, s.url)
316+
)
312317
"#
313318
),
314319
named_params! {
@@ -322,24 +327,6 @@ impl<'a> SuggestDao<'a> {
322327
let score: f64 = row.get("score")?;
323328
let full_keyword_from_db: Option<String> = row.get("full_keyword")?;
324329

325-
let keywords: Vec<String> = self.conn.query_rows_and_then_cached(
326-
r#"
327-
SELECT
328-
keyword
329-
FROM
330-
keywords
331-
WHERE
332-
suggestion_id = :suggestion_id
333-
AND rank >= :rank
334-
ORDER BY
335-
rank ASC
336-
"#,
337-
named_params! {
338-
":suggestion_id": suggestion_id,
339-
":rank": row.get::<_, i64>("rank")?,
340-
},
341-
|row| row.get(0),
342-
)?;
343330
self.conn.query_row_and_then(
344331
r#"
345332
SELECT
@@ -372,8 +359,7 @@ impl<'a> SuggestDao<'a> {
372359
title,
373360
url: cooked_url,
374361
raw_url,
375-
full_keyword: full_keyword_from_db
376-
.unwrap_or_else(|| full_keyword(keyword_lowercased, &keywords)),
362+
full_keyword: full_keyword_from_db.unwrap_or_default(),
377363
icon: row.get("icon")?,
378364
icon_mimetype: row.get("icon_mimetype")?,
379365
impression_url: row.get("impression_url")?,

‎components/suggest/src/schema.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,8 @@ CREATE TABLE geonames_metrics(
224224
max_name_word_count INTEGER NOT NULL
225225
) WITHOUT ROWID;
226226
227+
-- `url` may be an opaque dismissal key rather than a URL depending on the
228+
-- suggestion type.
227229
CREATE TABLE dismissed_suggestions (
228230
url TEXT PRIMARY KEY
229231
) WITHOUT ROWID;

‎components/suggest/src/store.rs‎

Lines changed: 90 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -203,11 +203,17 @@ impl SuggestStore {
203203
self.inner.query(query)
204204
}
205205

206+
/// Dismiss a suggestion.
207+
#[handle_error(Error)]
208+
pub fn dismiss(&self, suggestion: &Suggestion) -> SuggestApiResult<()> {
209+
self.inner.dismiss(suggestion)
210+
}
211+
212+
/// Deprecated, use [SuggestStore::dismiss] instead.
213+
///
206214
/// Dismiss a suggestion
207215
///
208216
/// Dismissed suggestions will not be returned again
209-
///
210-
/// In the case of AMP suggestions this should be the raw URL.
211217
#[handle_error(Error)]
212218
pub fn dismiss_suggestion(&self, suggestion_url: String) -> SuggestApiResult<()> {
213219
self.inner.dismiss_suggestion(suggestion_url)
@@ -442,6 +448,15 @@ impl<S> SuggestStoreInner<S> {
442448
})
443449
}
444450

451+
fn dismiss(&self, suggestion: &Suggestion) -> Result<()> {
452+
if let Some(dismissal_key) = suggestion.dismissal_key() {
453+
self.dbs()?
454+
.writer
455+
.write(|dao| dao.insert_dismissal(dismissal_key))?;
456+
}
457+
Ok(())
458+
}
459+
445460
fn dismiss_suggestion(&self, suggestion_url: String) -> Result<()> {
446461
self.dbs()?
447462
.writer
@@ -1022,7 +1037,7 @@ pub(crate) mod tests {
10221037
store.ingest(SuggestIngestionConstraints::all_providers());
10231038
assert_eq!(
10241039
store.fetch_suggestions(SuggestionQuery::amp("lo")),
1025-
vec![los_pollos_suggestion("los", None)],
1040+
vec![los_pollos_suggestion("los pollos", None)],
10261041
);
10271042
Ok(())
10281043
}
@@ -1082,7 +1097,7 @@ pub(crate) mod tests {
10821097

10831098
assert_eq!(
10841099
store.fetch_suggestions(SuggestionQuery::amp("lo")),
1085-
vec![los_pollos_suggestion("los", None)]
1100+
vec![los_pollos_suggestion("los pollos", None)]
10861101
);
10871102
assert_eq!(
10881103
store.fetch_suggestions(SuggestionQuery::amp("la")),
@@ -1110,25 +1125,78 @@ pub(crate) mod tests {
11101125
],
11111126
})),
11121127
// AMP attachment without full keyword data
1113-
good_place_eats_amp(),
1128+
good_place_eats_amp().remove("full_keywords"),
11141129
])))
11151130
.with_record(SuggestionProvider::Amp.icon(los_pollos_icon()))
11161131
.with_record(SuggestionProvider::Amp.icon(good_place_eats_icon()))
11171132
);
11181133
store.ingest(SuggestIngestionConstraints::all_providers());
11191134

1120-
assert_eq!(
1121-
store.fetch_suggestions(SuggestionQuery::amp("lo")),
1122-
// This keyword comes from the provided full_keywords list
1123-
vec![los_pollos_suggestion("los pollos", None)],
1124-
);
1135+
// (query string, expected suggestion, expected dismissal key)
1136+
let tests = [
1137+
(
1138+
"lo",
1139+
los_pollos_suggestion("los pollos", None),
1140+
Some("los pollos"),
1141+
),
1142+
(
1143+
"los pollos",
1144+
los_pollos_suggestion("los pollos", None),
1145+
Some("los pollos"),
1146+
),
1147+
(
1148+
"los pollos h",
1149+
los_pollos_suggestion("los pollos hermanos (restaurant)", None),
1150+
Some("los pollos hermanos (restaurant)"),
1151+
),
1152+
(
1153+
"la",
1154+
good_place_eats_suggestion("", None),
1155+
Some("https://www.lasagna.restaurant"),
1156+
),
1157+
(
1158+
"lasagna",
1159+
good_place_eats_suggestion("", None),
1160+
Some("https://www.lasagna.restaurant"),
1161+
),
1162+
(
1163+
"lasagna come out tomorrow",
1164+
good_place_eats_suggestion("", None),
1165+
Some("https://www.lasagna.restaurant"),
1166+
),
1167+
];
1168+
for (query, expected_suggestion, expected_dismissal_key) in tests {
1169+
// Do a query and check the returned suggestions.
1170+
let suggestions = store.fetch_suggestions(SuggestionQuery::amp(query));
1171+
assert_eq!(suggestions, vec![expected_suggestion.clone()]);
11251172

1126-
assert_eq!(
1127-
store.fetch_suggestions(SuggestionQuery::amp("la")),
1128-
// Good place eats did not have full keywords, so this one is
1129-
// calculated at runtime
1130-
vec![good_place_eats_suggestion("lasagna", None)],
1131-
);
1173+
// Check the returned suggestion's dismissal key.
1174+
assert_eq!(suggestions[0].dismissal_key(), expected_dismissal_key);
1175+
1176+
// Dismiss the suggestion.
1177+
store.inner.dismiss(&suggestions[0])?;
1178+
assert_eq!(store.fetch_suggestions(SuggestionQuery::amp(query)), vec![]);
1179+
1180+
// Clear dismissals and fetch again.
1181+
store.inner.clear_dismissed_suggestions()?;
1182+
assert_eq!(
1183+
store.fetch_suggestions(SuggestionQuery::amp(query)),
1184+
vec![expected_suggestion.clone()]
1185+
);
1186+
1187+
// Dismiss the suggestion by its raw URL using the deprecated API.
1188+
store
1189+
.inner
1190+
.dismiss_suggestion(expected_suggestion.raw_url().unwrap().to_string())?;
1191+
assert_eq!(store.fetch_suggestions(SuggestionQuery::amp(query)), vec![]);
1192+
1193+
// Clear dismissals and fetch again.
1194+
store.inner.clear_dismissed_suggestions()?;
1195+
assert_eq!(
1196+
store.fetch_suggestions(SuggestionQuery::amp(query)),
1197+
vec![expected_suggestion]
1198+
);
1199+
}
11321200

11331201
Ok(())
11341202
}
@@ -1299,7 +1367,7 @@ pub(crate) mod tests {
12991367
store.ingest(SuggestIngestionConstraints::all_providers());
13001368
assert_eq!(
13011369
store.fetch_suggestions(SuggestionQuery::amp("lo")),
1302-
vec![los_pollos_suggestion("los", None)],
1370+
vec![los_pollos_suggestion("los pollos", None)],
13031371
);
13041372

13051373
Ok(())
@@ -1527,7 +1595,7 @@ pub(crate) mod tests {
15271595
store.ingest(SuggestIngestionConstraints::all_providers());
15281596
assert_eq!(
15291597
store.fetch_suggestions(SuggestionQuery::amp("lo")),
1530-
vec![los_pollos_suggestion("los", None)],
1598+
vec![los_pollos_suggestion("los pollos", None)],
15311599
);
15321600
assert_eq!(
15331601
store.fetch_suggestions(SuggestionQuery::amp("la")),
@@ -2051,10 +2119,12 @@ pub(crate) mod tests {
20512119
json!([
20522120
los_pollos_amp().merge(json!({
20532121
"keywords": ["amp wiki match"],
2122+
"full_keywords": [("amp wiki match", 1)],
20542123
"score": 0.3,
20552124
})),
20562125
good_place_eats_amp().merge(json!({
20572126
"keywords": ["amp wiki match"],
2127+
"full_keywords": [("amp wiki match", 1)],
20582128
"score": 0.1,
20592129
})),
20602130
]),
@@ -2196,7 +2266,7 @@ pub(crate) mod tests {
21962266
// This should have been ingested
21972267
assert_eq!(
21982268
store.fetch_suggestions(SuggestionQuery::amp("lo")),
2199-
vec![los_pollos_suggestion("los", None)]
2269+
vec![los_pollos_suggestion("los pollos", None)]
22002270
);
22012271
// This should not have been ingested, since it wasn't in the providers list
22022272
assert_eq!(
@@ -2657,7 +2727,7 @@ pub(crate) mod tests {
26572727
);
26582728
assert_eq!(
26592729
store.fetch_suggestions(SuggestionQuery::amp("lo")),
2660-
vec![los_pollos_suggestion("los", None)],
2730+
vec![los_pollos_suggestion("los pollos", None)],
26612731
);
26622732
// Test deleting one of the records
26632733
store

‎components/suggest/src/suggestion.rs‎

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,29 @@ impl Ord for Suggestion {
134134
}
135135

136136
impl Suggestion {
137+
/// Get the suggestion's dismissal key, which should be stored in the
138+
/// `dismissed_suggestions` table when the suggestion is dismissed. Some
139+
/// suggestions may not have dismissal keys and cannot be dismissed.
140+
pub fn dismissal_key(&self) -> Option<&str> {
141+
match self {
142+
Self::Amp { full_keyword, .. } => {
143+
if !full_keyword.is_empty() {
144+
Some(full_keyword)
145+
} else {
146+
self.raw_url()
147+
}
148+
}
149+
Self::Pocket { .. }
150+
| Self::Wikipedia { .. }
151+
| Self::Amo { .. }
152+
| Self::Yelp { .. }
153+
| Self::Mdn { .. }
154+
| Self::Weather { .. }
155+
| Self::Fakespot { .. }
156+
| Self::Dynamic { .. } => self.raw_url(),
157+
}
158+
}
159+
137160
/// Get the URL for this suggestion, if present
138161
pub fn url(&self) -> Option<&str> {
139162
match self {
@@ -144,7 +167,7 @@ impl Suggestion {
144167
| Self::Yelp { url, .. }
145168
| Self::Mdn { url, .. }
146169
| Self::Fakespot { url, .. } => Some(url),
147-
_ => None,
170+
Self::Weather { .. } | Self::Dynamic { .. } => None,
148171
}
149172
}
150173

@@ -154,13 +177,15 @@ impl Suggestion {
154177
/// "cooked" using template interpolation, while `raw_url` is the URL template.
155178
pub fn raw_url(&self) -> Option<&str> {
156179
match self {
157-
Self::Amp { raw_url: url, .. }
158-
| Self::Pocket { url, .. }
159-
| Self::Wikipedia { url, .. }
160-
| Self::Amo { url, .. }
161-
| Self::Yelp { url, .. }
162-
| Self::Mdn { url, .. } => Some(url),
163-
_ => None,
180+
Self::Amp { raw_url, .. } => Some(raw_url),
181+
Self::Pocket { .. }
182+
| Self::Wikipedia { .. }
183+
| Self::Amo { .. }
184+
| Self::Yelp { .. }
185+
| Self::Mdn { .. }
186+
| Self::Weather { .. }
187+
| Self::Fakespot { .. }
188+
| Self::Dynamic { .. } => self.url(),
164189
}
165190
}
166191

‎components/suggest/src/testing/data.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ pub fn los_pollos_amp() -> JsonValue {
1414
"advertiser": "Los Pollos Hermanos",
1515
"iab_category": "8 - Food & Drink",
1616
"keywords": ["lo", "los", "los p", "los pollos", "los pollos h", "los pollos hermanos"],
17+
"full_keywords": [("los pollos", 4), ("los pollos hermanos", 2)],
1718
"title": "Los Pollos Hermanos - Albuquerque",
1819
"url": "https://www.lph-nm.biz",
1920
"icon": "los-pollos-favicon",
@@ -59,6 +60,7 @@ pub fn good_place_eats_amp() -> JsonValue {
5960
"advertiser": "Good Place Eats",
6061
"iab_category": "8 - Food & Drink",
6162
"keywords": ["la", "las", "lasa", "lasagna", "lasagna come out tomorrow"],
63+
"full_keywords": [("lasagna", 2), ("lasagna come out tomorrow", 2)],
6264
"title": "Lasagna Come Out Tomorrow",
6365
"url": "https://www.lasagna.restaurant",
6466
"icon": "good-place-eats-favicon",

‎components/suggest/src/testing/mod.rs‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,24 +15,41 @@ use serde_json::Value as JsonValue;
1515
pub use serde_json::json;
1616
pub use sql_support::ConnExt;
1717

18+
use std::{borrow::Borrow, hash::Hash};
19+
1820
/// Trait with utility functions for JSON handling in the tests
1921
pub trait JsonExt {
2022
fn merge(self, other: JsonValue) -> JsonValue;
23+
24+
fn remove<Q>(self, key: &Q) -> JsonValue
25+
where
26+
String: Borrow<Q>,
27+
Q: ?Sized + Ord + Eq + Hash;
2128
}
2229

2330
impl JsonExt for JsonValue {
2431
fn merge(mut self, mut other: JsonValue) -> JsonValue {
25-
let self_map = match &mut self {
26-
JsonValue::Object(obj) => obj,
27-
_ => panic!("merge called on non-object: {self:?}"),
32+
let JsonValue::Object(self_map) = &mut self else {
33+
panic!("merge called on non-object `self`: {self:?}");
2834
};
29-
let other_map = match &mut other {
30-
JsonValue::Object(obj) => obj,
31-
_ => panic!("merge called on non-object: {other:?}"),
35+
let JsonValue::Object(other_map) = &mut other else {
36+
panic!("merge called on non-object `other`: {other:?}");
3237
};
3338
self_map.append(other_map);
3439
self
3540
}
41+
42+
fn remove<Q>(mut self, key: &Q) -> JsonValue
43+
where
44+
String: Borrow<Q>,
45+
Q: ?Sized + Ord + Eq + Hash,
46+
{
47+
let JsonValue::Object(obj) = &mut self else {
48+
panic!("remove called on non-object: {self:?}");
49+
};
50+
obj.remove(key);
51+
self
52+
}
3653
}
3754

3855
/// Extra methods for testing

0 commit comments

Comments
 (0)