FXIOS-15132 - Merino Rust client cleanup

[adudenamedruby]

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.

[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]

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?

[adudenamedruby]

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.

[mhammond]

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)

[adudenamedruby]

I'll leave this for posterity but there are planned reasons which account for the existence of this component (certain filtering to be done per platform, implementing some sort of cache for this information for more effective retrieval/usage, etc), because otherwise, I'm onboard with you re: this shouldn't just be a middle-man. Those reasons haven't been implemented yet because a) priorities have kept changing and b) because iOS is currently the only consumer of this component. This is about to change, where Android will be switching over to this.

Another reason this was done is that iOS doesn't have a network stack, currently, and any non-webkit things are done pretty haphazardly by setting up individual network calls - which we really shouldn't do if we want to be smart about our architecture.... but nearly all network things we do that are not webkit related, are AS related, so the decision was made to, instead of investing in a network stack on iOS, to streamline things through AS because, as more platfroms used those components, it just made more sense.

The pull request has been modified, dismissing previous reviews.

[adudenamedruby]

Heya'll, is there anything else you think I need to do on this PR to get it approved & merged? Just trying to get it on it's merry way! :D