FXIOS-15132 - Merino Rust client cleanup by adudenamedruby · Pull Request #7274 · mozilla/application-services

adudenamedruby · 2026-03-18T20:10:42Z

As part of the work to update the Merino Rust client to fetch categories, this first PR does some prep work to make the updates easier, and to improve maintainability for the client.

The commits for this PR are fairly self explanatory for review, but, in general, the PR:

adds some documentation
improves error handling
improves some duplicated code re: locales
adds some tests

I've successfully tested the changes in the PR:

against a local AS build in Xcode in firefox iOS
with the merino-cli

I don't think this PR needs a CHANGELOG, because it's simply a refactor, not really adding/removing feature or capabilities to the client.

Please note: I am a Rust noob, so please let me know what improvements I can make.

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes, or
  - There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

components/merino/src/curated_recommendations/error.rs

jrconlin · 2026-03-18T21:47:45Z

components/merino/src/curated_recommendations/error.rs

-                reason: self.to_string(),
-            })
-            .report_error("merino-unexpected"),
+            Self::UrlParse(_) | Self::Json(_) => {


Hmm. Previously we were using _ as a universal trap for any unhandled error state. While this change now explicitly handles the two, previously unhandled states, it essentially doesn't raise a problem, but it does complicate future code if we ever introduce a new local Error state.

I'm a bit inclined to return this to the prior universal wrapper.

the counter argument is that adding a new local Error variant should force this to be looked at to determine if the default handler really is appropriate for the new variant.

Yeah, I guess as a Swift developer, I often this of Rust's match as an improved kind of switch, and the use of switch usually (though, not always) means, "handle all cases" so that the compiler yells at me when I add new cases. I'm happy to go back to _ but that's why I had made that change. Happy to undo it.

I'm not really bothered by either approach. Mark raises a good point about forcing inspection. I guess I'm used to an environment where only actionable errors are reported (due to cost reasons) so I've gotten into the habit of having default conditions. If y'all are fine with not having a default, I'm not going to argue.

components/merino/src/lib.rs

components/merino/src/curated_recommendations/tests.rs

components/merino/src/curated_recommendations/mod.rs

mhammond

A bit of a drive-by that initially mis-understood the scope and that this was just a refactor - it's probably worth what it cost you, but I'm hitting "submit" anyway :)

mhammond · 2026-03-18T22:20:37Z

components/merino/src/curated_recommendations/models/feeds.rs

+#[derive(Debug, Deserialize, PartialEq, uniffi::Record, Serialize)]
+pub struct Feeds {
+    /// High-priority "need to know" recommendations.
+    #[uniffi(default = None)]


fyi, I think you should be able to omit the = None part on these defaults. Again, this is just a refactor, and when this was written you could not omit = None part, but again, leaving it in-case you ever add new ones :)

mhammond · 2026-03-18T23:14:47Z

components/merino/src/curated_recommendations/models/feeds.rs

+///
+/// Each field corresponds to a content category or special feed type. Fields are
+/// `None` when the category was not requested or has no content available.
+#[derive(Debug, Deserialize, PartialEq, uniffi::Record, Serialize)]


eek: I just noticed that you did not actually introduce this, just refactored it. So feel free to ignore this entirely, but I'll leave it here as I think it still makes some points worth considering for your future endeavours :)

It's worth being careful deriving both uniffi::Record and serde traits on the same object. The issue is that you are making it such that a change in the "wire" protocol must end up being a breaking change for the mobile clients - this is probably worse for Serialize, and I suspect you don't actually need serialize here - I guess you only deserialize them? But a non-optional field addition (or even deletion!) ends up being seen by the ultimate consumer.
It also means decisions made a long way from the mobile clients ends up deciding the api exposed to these mobile clients - what makes perfect sense for the server code crafting the JSON responses might seem really odd represented as Swift.

Or to make the same point even stronger - what value are these structs providing to the mobile clients? Why not just send them the raw JSON and let them deserialize it?

Yeah, this is beyond me, honestly, as I don't much know the workings of Uniffi, and just making most of this refactor was really stretching my Rust knowledge lolololo. I think I'll leave this for now, it seems you're ok with that.

As for the bigger point you're making: "what value are these structs providing to the mobile clients? Why not just send them the raw JSON and let them deserialize it?" Ultimately, if we're passing raw JSON, then what would be the purpose of this client, because it's basically just a network call with extra steps. The original reasoning for implementing it was that, ideally, it acts as a singular interface to Merino that provides objects for the clients to use so that each client isn't manually maintaining its own Merino client - that's three times the work to implement any changes and maintenance vs the one. What the clients do with the objects/data afterwards, that makes sense to be on the clients, because they each handle that info differently.

Ultimately, if we're passing raw JSON, then what would be the purpose of this client, because it's basically just a network call with extra steps.

Exactly 😅

that's three times the work to implement any changes and maintenance vs the one

It's not immediately clear we have achieved that in practice - with this model, some changes will still require touching all 3 consumers of this crate - eg, adding a new field to a struct is technically a breaking change, but adding a new field to a json payload need not be.

If a Rust component is literally a wrapper around JSON and trivial network calls, and has no "difficult" logic or anything else (which is where the real burden of multiple impls come from), then yeah, it really shouldn't be in Rust imo. This single shared impl has a real cognitive burden - the author needs to understand uniffi, have at least a passing knowledge of all binding languages expected to be used (eg, kotlin and swift), and know Rust. A trivial JSON/network component written in, say, just swift is far more approachable to far more people.

(But as above, I know you didn't create this, so it's a meta comment rather than anything about this patch)

mhammond · 2026-03-19T00:29:17Z

components/merino/src/curated_recommendations/models/mod.rs

+pub mod response;
+
+// Re-export all model types for use by UniFFI bindings and downstream consumers.
+#[allow(unused_imports)]


why do you re-export these if they are unused? models isn't public, so the only possible use for these re-exports is in curated_recommendations, and if this clippy annotation is accurate, they aren't used - so why not just remove the unused re-exports instead of overriding clippy here? (It looks like maybe not all are actually unused though). And again, not clear you actually introduced this.

To be clear, my understanding of Uniffi is basically zero, but from the snippets of conversation I've had about Uniffi, I gather that Uniffi scaffolding needs types to be publicly accessible, and a flat module hierarchy makes the generated bindings cleaner. So the re-exports exist so that these types are available at the models:: path level, ie. models::Feeds instead of models::feeds::Feeds.

the #[allow(unused_imports)] annotations are there because within the Rust crate itself, nothing currently imports these types through the re-exported path. But I thought the re-exports are still needed for UniFFI's code generation to discover and expose them in the generated bindings - although, arguably, I did NOT test if that is the case or not. I just went off my understanding, and when tests passed and it worked in FXiOS without other changes, I just called it a day lololol

lemme know if that's not good, and I can clean it up

In short, uniffi just generates Rust code (usually via macros, sometimes an actual .rs) - if that code needed to reference a type, then it's just (hidden to you) Rust code that does, so the clippy warning would not fire. IOW, clippy is already taking uniffi into account when generating its warnings.

mhammond · 2026-03-19T00:33:47Z

components/merino/src/curated_recommendations/error.rs

-                reason: self.to_string(),
-            })
-            .report_error("merino-unexpected"),
+            Self::UrlParse(_) | Self::Json(_) => {


the counter argument is that adding a new local Error variant should force this to be looked at to determine if the default handler really is appropriate for the new variant.

gruberb · 2026-03-19T11:39:40Z

components/merino/README.md

+- **A/B experiment support** — Pass experiment name and branch parameters to support server-side experimentation.
+- **Cross-platform** — Rust core with UniFFI-generated bindings for Android (and other platforms).
+
+## Architecture


I would rather not have this Architecture session here. Once we change or add files, we always have to update the README. Which, in the end, can get rather large.

gruberb · 2026-03-19T11:40:33Z

components/merino/README.md

@@ -0,0 +1,39 @@
+# Merino
+
+A cross-platform Rust client library for Mozilla's [Merino](https://merino.services.mozilla.com) curated recommendations service. This powers the curated content recommendations (articles/stories) shown on Firefox's New Tab page.


Let's not get too specific, or we have to add and change the README every time we do a slight addition or change.

gruberb · 2026-03-19T14:34:16Z

components/merino/README.md

+
+The library provides a `CuratedRecommendationsClient` that fetches curated recommendations from the Merino backend via its REST API (`/api/v1/curated-recommendations`). It uses [UniFFI](https://mozilla.github.io/uniffi-rs/) to generate cross-platform bindings for Android (and other targets).
+
+## Features


I know this was generated with Claude, but can we make sure to actually include things in the README which are helpful for maintainers/users? :) I don't think we need to mention Cross-platfom etc.

Might be nit picky, but if I read stuff which makes me think this was auto-generated, I don't bother reading much of it at all.

components/merino/src/lib.rs

Co-authored-by: JR Conlin <src+github@jrconlin.com>

The pull request has been modified, dismissing previous reviews.

adudenamedruby added 4 commits March 18, 2026 09:46

Add Readme file

af1a1e7

clean up project organization

211a8a6

Add documentation

8e55cb3

Improve error handling and locale list

0b28174

misaniwere requested a review from a team March 18, 2026 20:16

Add some minor tests and remove dupes

5138f4f

adudenamedruby force-pushed the rgb/topsy-turvy-sparkly-tomato branch from a307ee0 to 5138f4f Compare March 18, 2026 20:21

jrconlin reviewed Mar 18, 2026

View reviewed changes

mhammond reviewed Mar 19, 2026

View reviewed changes

Address PR comments

6c691a3

adudenamedruby force-pushed the rgb/topsy-turvy-sparkly-tomato branch from 42bf612 to 6c691a3 Compare March 19, 2026 11:50

gruberb reviewed Mar 19, 2026

View reviewed changes

Make README more concise

0db2a79

jrconlin previously approved these changes Mar 19, 2026

View reviewed changes

components/merino/src/lib.rs Outdated Show resolved Hide resolved

Update components/merino/src/lib.rs

fa98e8d

Co-authored-by: JR Conlin <src+github@jrconlin.com>

		@@ -0,0 +1,39 @@
		# Merino

		A cross-platform Rust client library for Mozilla's [Merino](https://merino.services.mozilla.com) curated recommendations service. This powers the curated content recommendations (articles/stories) shown on Firefox's New Tab page.


		The library provides a `CuratedRecommendationsClient` that fetches curated recommendations from the Merino backend via its REST API (`/api/v1/curated-recommendations`). It uses [UniFFI](https://mozilla.github.io/uniffi-rs/) to generate cross-platform bindings for Android (and other targets).

		## Features

Conversation

adudenamedruby commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adudenamedruby commented Mar 18, 2026 •

edited

Loading