Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a Rust component for a Merino client (#5345) #5346

Closed
wants to merge 12 commits into from
Closed

Create a Rust component for a Merino client (#5345) #5346

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7a43d3f
Initial scaffolding for a Merino client.
linabutler Dec 28, 2022
f105624
merino_client: Implement fetching suggestions.
linabutler Dec 29, 2022
d23fac0
merino_client: Build Swift bindings.
linabutler Dec 30, 2022
bcfd149
merino_client: Support provider-specific fields.
linabutler Jan 9, 2023
f879e4d
merino_client: Tighten up error mapping code.
linabutler Jan 12, 2023
4674fe4
merino_client: Add initial tests.
linabutler Jan 12, 2023
8cfd96f
merino_client: Extract session ID generation into `generate_session_id`.
linabutler Feb 7, 2023
07a9b5d
merino_client: `session_params()` => `current_session_params()`.
linabutler Feb 7, 2023
7e36365
Add `MerinoClient::for_server` and tests.
tiftran Feb 7, 2023
29e7fad
additional tests
tiftran Mar 16, 2023
12e20ae
added accuweather provider
tiftran Mar 22, 2023
8d1c699
clippy fixes
tiftran Mar 22, 2023
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
118 changes: 96 additions & 22 deletions components/merino_client/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,7 @@ impl MerinoClient {
Ok(response
.suggestions
.into_iter()
.map(|suggestion| MerinoSuggestion {
title: suggestion.title,
url: suggestion.url,
provider: suggestion.provider,
is_sponsored: suggestion.is_sponsored,
score: suggestion.score,
icon: suggestion.icon,
request_id: response.request_id.clone(),
})
.map(MerinoSuggestion::from)
.collect())
}
}
Expand Down Expand Up @@ -147,31 +139,113 @@ pub struct MerinoClientFetchOptions {
pub providers: Option<Vec<String>>,
}

#[derive(Clone, Debug, PartialEq)]
pub struct MerinoSuggestion {
#[derive(Clone, Debug, Deserialize, PartialEq)]
pub struct MerinoSuggestionDetails {
pub title: String,
pub url: String,
pub provider: String,
pub is_sponsored: bool,
pub score: f64,
pub icon: Option<String>,
pub request_id: String,
}

#[derive(Clone, Debug, PartialEq)]
pub enum MerinoSuggestion {
linabutler marked this conversation as resolved.
Show resolved Hide resolved
Adm {
details: MerinoSuggestionDetails,
block_id: i64,
full_keyword: String,
advertiser: String,
impression_url: Option<String>,
click_url: Option<String>,
},
TopPicks {
details: MerinoSuggestionDetails,
block_id: i64,
is_top_pick: bool,
},
Other {
details: MerinoSuggestionDetails,
provider: String,
},
}

impl From<SuggestResponseSuggestion> for MerinoSuggestion {
fn from(suggestion: SuggestResponseSuggestion) -> Self {
match suggestion {
SuggestResponseSuggestion::Known(SuggestResponseKnownProviderSuggestion::Adm {
details,
block_id,
full_keyword,
advertiser,
impression_url,
click_url,
}) => MerinoSuggestion::Adm {
details,
block_id,
full_keyword,
advertiser,
impression_url,
click_url,
},
SuggestResponseSuggestion::Known(
SuggestResponseKnownProviderSuggestion::TopPicks {
details,
block_id,
is_top_pick,
},
) => MerinoSuggestion::TopPicks {
details,
block_id,
is_top_pick,
},
SuggestResponseSuggestion::Unknown { details, provider } => {
MerinoSuggestion::Other { details, provider }
}
}
}
}

#[derive(Deserialize)]
struct SuggestResponse {
suggestions: Vec<SuggestionResponse>,
request_id: String,
suggestions: Vec<SuggestResponseSuggestion>,
}

#[derive(Deserialize)]
struct SuggestionResponse {
pub title: String,
pub url: String,
pub provider: String,
pub is_sponsored: bool,
pub score: f64,
pub icon: Option<String>,
#[serde(untagged)]
enum SuggestResponseSuggestion {
Known(SuggestResponseKnownProviderSuggestion),
// `#[serde(other)]` doesn't support associated data, so we can't
// deserialize the response suggestion directly into a `MerinoSuggestion`.
// Instead, we have an "outer", untagged `SuggestResponseSuggestion` that
// has our unknown / other variant, and an "inner", internally tagged
// `SuggestResponseKnownProviderSuggestion` with our known variants.
linabutler marked this conversation as resolved.
Show resolved Hide resolved
Unknown {
#[serde(flatten)]
details: MerinoSuggestionDetails,
provider: String,
},
}

#[derive(Deserialize)]
#[serde(tag = "provider")]
enum SuggestResponseKnownProviderSuggestion {
#[serde(rename = "adm")]
linabutler marked this conversation as resolved.
Show resolved Hide resolved
Adm {
#[serde(flatten)]
details: MerinoSuggestionDetails,
block_id: i64,
full_keyword: String,
advertiser: String,
impression_url: Option<String>,
click_url: Option<String>,
},
#[serde(rename = "top_picks")]
TopPicks {
#[serde(flatten)]
details: MerinoSuggestionDetails,
block_id: i64,
is_top_pick: bool,
},
}

struct SessionState {
Expand Down
2 changes: 1 addition & 1 deletion components/merino_client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ mod error;
pub use crate::{
client::{
MerinoClient, MerinoClientFetchOptions, MerinoClientSettings, MerinoServer,
MerinoSuggestion,
MerinoSuggestion, MerinoSuggestionDetails,
},
error::MerinoClientError,
};
Expand Down
11 changes: 8 additions & 3 deletions components/merino_client/src/merino_client.udl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,19 @@ dictionary MerinoClientFetchOptions {
sequence<string>? providers = null;
};

dictionary MerinoSuggestion {
dictionary MerinoSuggestionDetails {
string title;
string url;
string provider;
boolean is_sponsored;
f64 score;
string? icon;
string request_id;
};

[Enum]
interface MerinoSuggestion {
Copy link
Member Author

@linabutler linabutler Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: UDL dictionaries don't support inheritance—and neither do Rust or Swift structs—so we can't do something like dictionary AdmSuggestionDetails : MerinoSuggestionDetails, and have a separate dictionary type for each provider that inherits the base details. That's why the base details are always the first field in each provider variant, followed by the provider-specific fields.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: Even though we can't have inheritance, I've found it super helpful to have accessors for the details in both Kotlin and Swift:

val MerinoSuggestion.details: MerinoSuggestionDetails
    get() = when (this) {
        is MerinoSuggestion.Adm -> details
        is MerinoSuggestion.TopPicks -> details
        is MerinoSuggestion.Other -> details
    }
extension MerinoSuggestion {
    var details: MerinoSuggestionDetails {
        switch self {
        case .adm(let details, _, _, _, _, _):
            return details
        case .topPicks(let details, _, _):
            return details
        case .other(let details, _):
            return details
        }
    }
}

That lets us write code like suggestion.details.url and suggestion.details.icon, instead of having to pattern-match on the suggestion with when/switch, and extract the details every time we want a base suggestion field.

UniFFI enum interfaces can't have methods or getters, so we can't put that accessor in the UDL—but we can still add hand-written Kotlin and Swift source files with those extensions to our component! The FxA client does something like that in its Kotlin and Swift bindings.

This would be a great exercise for getting more familiar with the component directory structure, and updating the Xcode project to include the hand-written Swift extension!

/cc @tiftran

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: Even though we can't have inheritance, I've found it super helpful to have accessors for the details in both Kotlin and Swift:

That's really nice! I wonder if we can teach UniFFI about that?

UniFFI enum interfaces can't have methods or getters, so we can't put that accessor in the UDL—but we can still add hand-written Kotlin and Swift source files with those extensions to our component!

This raises an interesting point. The hand-written foreign code next to the component was initially hugely valuable - our hand-written FFI was so painful that wrappers made a lot of sense. However, as UniFFI means the bindings are quite ergonomic, the app-services team has had an informal decision to keep these hand-wrappers as small as possible - if at all. Part of the reason was that we found understanding old code became quite difficult, particularly for Fenix - by the time our component ended up being consumed we were often many levels of indirection away from the component itself - we have our hand-wrappers, a couple of wrappers in android-components (often a "concept" and a "service"), then the final consumer. Nimbus has gone the other way (the "canonical" wrappers are next to their component). I don't think there is a single correct answer here, but it is a tension that we still feel.

(One example of what I'm referring to is our autofill component - the only hand-written kotlin there is working around a UniFFI limitation (or probably more accurately, a limitation in our megazord layout) - ie, so similar to your example above. But any additional ergonomic improvements tends to be in our consumer code (eg, in android-components)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really nice! I wonder if we can teach UniFFI about that?

I filed mozilla/uniffi-rs#1470. 😄

However, as UniFFI means the bindings are quite ergonomic, the app-services team has had an informal decision to keep these hand-wrappers as small as possible - if at all.

nod That makes sense, for all the reasons you mentioned—having a substantial amount of hand-written code in Application Services adds another layer of wrapping that can hide important implementation details. I totally agree with your approach of keeping that layer as small as possible (if at all), and saving the ergonomics for our consumers.

Adm(MerinoSuggestionDetails details, i64 block_id, string full_keyword, string advertiser, string? impression_url, string? click_url);
TopPicks(MerinoSuggestionDetails details, i64 block_id, boolean is_top_pick);
Other(MerinoSuggestionDetails details, string provider);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (blocking): Let's handle AccuWeather suggestions, too. I think adding them will be a fun exercise that covers updating the UniFFI definition, Serde deserialization, and tests! 🎉

/cc @tiftran

};

interface MerinoClient {
Expand Down