Create a Rust component for a Merino client (#5345) #5346
Conversation
Each Merino suggestion provider returns a set of base fields, and additional fields specific to that provider. For example, adM suggestions have keywords and additional URLs; Top Picks suggestions have a `block_id`; AccuWeather suggestions (not implemented yet) have a forecast and current conditions. This commit reworks the Merino suggestion response model to capture the base fields in a `MerinoSuggestionDetails` dictionary, and expose provider-specific fields as associated data for each provider variant.
There was a problem hiding this comment.
@tiftran I left some comments on each commit, with some pointers that might be interesting for you! 😊
megazords/ios-rust/MozillaTestServices/MozillaTestServices.xcodeproj/project.pbxproj
Show resolved
Hide resolved
| }; | ||
|
|
||
| [Enum] | ||
| interface MerinoSuggestion { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Some things for us to work on!
I'm trying out "conventional comments" for review feedback—what do you think? I'm not sure how I feel about them yet... 😄
| interface MerinoSuggestion { | ||
| 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); |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
suggestion (blocking): Let's add some more tests! Ideas for test cases:
- Other kinds of suggestions (Top Picks, AccuWeather).
- Specifying additional settings, like the default providers, client variants.
- Explicitly specifying
providerswhen callingfetch. - Session ID and sequence number rotation.
- Error responses (non-200 response code, bad JSON in body).
/cc @tiftran
This commit changes the session ID to use a hyphenated version 4 UUID, matching Desktop, and removes the now-unused `hex` dependency.
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #5346 +/- ##
==========================================
- Coverage 46.58% 46.48% -0.11%
==========================================
Files 181 170 -11
Lines 14614 14285 -329
==========================================
- Hits 6808 6640 -168
+ Misses 7806 7645 -161 see 85 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
We decided to go in a different direction, and focus on shipping the "basic" Firefox Suggest experience, instead of the "improved" experience powered by Merino. The basic experience downloads all suggestions from a Remote Settings collection, and matches suggestions on-device. Adding a Merino backend for the improved Suggest experience to our new Thanks, folks! |
Pull Request checklist
[ci full]to the PR title.Branch builds: add
[ff-android: firefox-android-branch-name]and/or[fenix: fenix-branch-name]to the PR title.