adds AWC as client option. Need test work for Document

[igaul]

Pull Request

Related issue

• None: Small change, so opening pr for visibility. The tests in documents will need to be rewritten to work with other clients before mainlining. Can also add a feature for reqwest if interest ( and work on tests is done ).
• This pr allows users to drop isahc as a dependency when using actix_web and run in environments where the current isahc configuration does not support tls.

What does this PR do?

• Adds feature for using Actix Web Client as the request client.
• Will need cleanup before mainlining.

PR checklist

Please check if your PR fulfills the following requirements:

• Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
• Have you read the contributing guidelines?
• Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

[irevoire]

Hey, could you explain how it is helpful for this project?
It seems like it adds a lot of complexity for no real benefits to me right now 🤔

[igaul]

The primary benefit is that it works 😀 where the current does not. After that, it reduces complexity, dependencies, and binary size for users. Updating the tests would also allow other drop in replacements like reqwest, which is already used in most rust web projects.

"This pr allows users to drop isahc as a dependency when using actix_web and run in environments where the current isahc configuration does not support tls"

[irevoire]

Hey, I see you took the time to update your PR, and thanks for that, but there are a few blockers to merging your PR.

1. I still don't get the point of this PR. « The primary benefit is that it works 😀 where the current does not », cool but it's wrong. It already works; here is a sample project that proves it, so either fix your code or explain what doesn't work currently.
2. I don't understand why we should use awc; it's very uncommon in the rust ecosystem and doesn't support wasm from what I see. Reqwest or surf seems like way better options
3. I don't want to support both ishac and awc
4. There are no tests

[igaul]

Hey, I think it's all covered already, but since you're confused:

1. Like I said "run in environments where the current isahc configuration does not support tls", your example project doesn't address that at all, so I'm not sure what you're claiming it proves. If you deploy it to something like flyio, it will fail to validate some certs, while reqwest and awc work fine as is with rustls. This aspect could possibly be solved with a different isahc configuration, but it still doesn't make a lot of sense to bundle libcurl when you already have hyper dependencies.

2. Like I said, this library will need to make the same changes to support reqwest in test suite ( which I offered to add also, its trivial ). I don't recall the exact problem with your tests, maybe just they are sync only? This is feature gated, so no one has to use it. It's also not a replacement for the default ishac setup, its opt in. Wasm is also gated and doesn't have anything to do with this.

3. They are both rfc compliant, so it shouldn't be any technical burden, same with reqwest, which would also be a drop in.

4. Like I mentioned, new tests are blocked by your current setup. We have tests around this, but I didn't want to spend time adding them here if you're not interested in supporting any other clients.

I only synced this to backfill a missing api, not to progress this pr, I didn't realize this sdk wasn't complete when I suggested using it. I generated a reqwest client from the oai spec, but it was pretty ugly, so it seemed easiest path forward is to just add missing features here as needed. But thanks to everyone for their work on meilisearch and congrats on getting to 1.0! Do ping me if you decide you want to support other clients or make this library on par with your js client ( which is what we have been using ).

[irevoire]

Sorry, I don't have a lot of time to allocate to this repository, nor do I know much about the state of frontend in rust currently, so it's quite hard to make a decision.

What I was saying for 3️⃣ is that one day, we're going to update the request function you duplicated for awc. And I don't want to update it once for isahc, reqwest, awc, wasm, and possibly more.
I, I would be more in favor of getting rid of isahc entirely and moving to something else easier to maintain.

From what I see of the rust ecosystem, isahc seems like a pretty bad option for everyone, but moving everyone to awc doesn't seem like the best option either.

What do you think?
Does moving to surf or reqwest entirely with feature flags to choose your SSL backend seems like a good solution to you?

---

Another question, if we were to continue with your PR.
How would we test the whole SDK with your backend? Doing a cargo test --feature xxx would work, I guess, but is there a way to do that automatically without installing an external tool?
What kind of tests did you want to test?

[igaul]

Yeah, reqwest is ( by a massive margin ) the most common client in rust, awc is more common than most ( like isahc or surf ), due to actix being the leading framework, but is also being supplanted by reqwest. So, if you're only going to support one client, reqwest is an easy choice. I haven't used surf, but it seems to be a wrapper around isahc and hyper, with wasm support, so as long as you export the feature flags, that would also cover a lot of options.

As far as adding functions, I don't think that's significant since for any rfc compliant client, you would only need a single bridge implementation to this lib per client and all clients would be feature gated.

A simple solution for everyone ( though I understand not wanting to do it ) would be to split this into two crates:
1. with all the interfaces that match a version of MS ( this could be generated from oai or MS http crate ), the types are really what consumers need from this crate, clients are a few lines to implement.
2. with an actual client implementation using the first

This would allow anyone to use any client they want without extra burden on MS team as new clients become popular or change. It's fairly common in js sdks, so that one can consume it with browser, node, react-native, etc http clients. The first crate simply guarantees req/resp/error types.

As far as feature flag testing, its not very pretty yet, using a script/action in ci to call each feature test is the most common way I'm aware of. But really, a test on each client's implementation and whole test suite on whichever client is default would be enough-ish, since any legitimate client is already tested in their own crates.

[irevoire]

A simple solution for everyone ( though I understand not wanting to do it ) would be to split this into two crates:

1. with all the interfaces that match a version of MS ( this could be generated from oai or MS http crate ), the types are really what consumers need from this crate, clients are a few lines to implement.
2. with an actual client implementation using the first

That would be really nice, but I don’t think it’s going to happen in a reasonable time scale since it impacts meilisearch (the oai we provide is updated by hand and might miss a bunch of things) 😰

Do you think it would be reasonable to make the request function configurable? Something like providing a handle to your function when creating a new client?

Where by default, the SDK uses and guarantees the support of only one crate (ideally reqwest if most people already have it in their workflow). And with the help of a feature flag, you can get rid of reqwest, but we don’t provide and maintain anything else.
I guess it would be pretty easy for everyone to customize what they want afterward almost without any changes to this repo 🤔

tbh I would really like to avoid;

1. Having to test multiple features flags to ensure your code works, we have a lot of new rust users here, and the experience is quite bad
2. Having multiple request functions to update every time we change something to our Method enum or the generic parameters

[bidoubiwa]

This PR solves the issue you raised: #459. Closing this issue for now, thanks a lot for the time you spend to improve this library. Sorry it did not resulted in a merge.