Use reqwest as the request client backend.

gibbz00 commented 1 year ago

It adds the ability to switch to a reqwest backend by using either the reqwest-native-tls or the reqwest-rustls feature flag.

API changes:

RequestClient made public through a top-level export.
Inlined add_query_parameters.
Removes the isahc feature flags.
Request headers in Wasm context now shares the "Meilisearch Rust (vX)" user agent header value. (Previous value was "X-Meilisearch-Client".)

~~The CI workflow has been improved to test both backends,~~ but also by running cargo clippy on all targets.

The commits are fairly atomic/logical, so I would suggest at least glancing over them one at a time before reviewing the code in its entirety.

~~Adding more backends, such as using reqwest in the WASM context, is out of the scope of this PR.~~

All the best ✌️

irevoire commented 1 year ago

Hey, as I said in https://github.com/meilisearch/meilisearch-rust/pull/426, I’m not really interested in adding features for every backend, we're already struggling a lot to maintain this repository so the ideal thing for us would be to let people provide their own request handler as you can see in https://github.com/meilisearch/meilisearch-rust/pull/459 (the PR is mostly finished it just needs a huge rebase)

gibbz00 commented 1 year ago

I understand where you're coming from, but did you at least look at the PR changes?

irevoire commented 1 year ago

Yeah, it seems good apart from the user-agent in the wasm backend because that may throw an error in if set in Chrome or Firefox. I would just prefer merging the other PR and letting you (or anyone else) create a new crate that imports meilisearch-rust + src/request/reqwest_native_client.rs and re-export it so we don't have to handle all the errors that could come from reqwest.

gibbz00 commented 1 year ago

Okay, thanks. Then I know that we are somewhat on the same page. In #426 you said that "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." Do you still think that is the case with a centralized RequestClient::request function? All the logic is contained there, and each implementation is super thin, in which it only "re-exports" core request client methods. I don't see how #459 solves this, all the logic is still duplicated.

Compare this:

https://github.com/meilisearch/meilisearch-rust/blob/cc35c641efb0beb22de412cce19ef28ac692062b/src/request/reqwest_native_client.rs#L21-L65

to this:

https://github.com/meilisearch/meilisearch-rust/blob/04ea0e018635e7b0b9de1ce22a2841b43ca332a3/examples/cli-app-with-reqwest/src/main.rs#L32-L135

gibbz00 commented 1 year ago

Imagine the developer experience with #459.

You as maintainers have no control in how the requests are handled.
I have to rely on wonky third party crates that have little guarantees to work, as the client isn't being tested against your tests.
If you change the API, then I as a consumer become basically helpless when that breaks a separate request client crate.

c1wren commented 1 year ago

This PR also has the benefit of working around an issue with linking to curl on macOS Sonoma that causes crashes when running. See https://github.com/alexcrichton/curl-rust/issues/524 for reference.

irevoire commented 1 year ago

Do you still think that is the case with a centralized RequestClient::request function?

Yeah tbh I think your RequestClient looks better than what we had, we lose a bit of freedom, but I guess that’s not an issue for most people 🤔

I have to rely on wonky third party crates that have little guarantees to work, as the client isn't being tested against your tests.

But about that, we know it’s not ideal, but it’s precisely what we don't want to support 😬 We want to keep this crate as small as possible to reduce our need to update dependencies, handle breaking, and keep the maintainer work as small as possible (because testing against a lot of features is quite a burden).

So basically, get rid of the features and reqwest client + let people set their own HTTP client in the meilisearch_sdk::Client I think we could merge this one instead of the other one.

Also, FYI, in a second step/PR later, I would like to get rids of ishac entirely and replace it with reqwest by default because that’s what most people already have in their project, I guess. It should highly reduce the number of people who want to change their client for something else.

gibbz00 commented 1 year ago

I think the lost freedom can in most cases can still be worked around wherever necessary, such as overriding the user agent to "X-Meilisearch-Client" in the browser request client.

How about this then: We replace both isahc and browser client with the reqwest client and put their implementations in an contrib folder. And those that wish to use another client can simply fork this repo and replace the default impementation? (This enables support for non-browser based wasm contexts.)

I believe it is almost impossible to set a RequestClient in meilisearch_sdk::Client without any large and breaking API changes, changes that make will only make the library unwieldy to work with. (RequestClient has generic parameters and is definitely not object safe, so it can't simply be saved in a `Box.)

I'll go ahead and implement this so we can see how this turns out in practice.

gibbz00 commented 1 year ago

I've now done what as explained in the comment above 😊 Almost all conditional feature flag usage has been removed in relation to HTTP requests. New API are documented in the commit history and copied to the first comment in this issue.

I was, however, unable to double-check that the web_app example was working, not from the main branch either. If all looks good, and you're interested in merging this PR, could you just check that the web_app example still works?

c1wren commented 1 year ago

@gibbz00 Just FYI, your most recent batch of changes makes code that previously compiled to no longer compile.

Minimal reproducible example:

tokio::task::spawn(async move {
        let meilisearch_url = "some url";
        let meilisearch_master_key = "some master key";

        let client = meilisearch_sdk::Client::new(meilisearch_url, Some(meilisearch_master_key));

        let index = client.index("testing");

        let _task = index
            .add_documents_in_batches(&Vec::<MeilisearchRecord>::new(), Some(5000), Some("id"))
            .await;
    });

The error: the trait `Send` is not implemented for `dyn futures_lite::Future<Output = Result<TaskInfo, meilisearch_sdk::Error>>

gibbz00 commented 1 year ago

@gibbz00 Just FYI, your most recent batch of changes makes code that previously compiled to no longer compile.

F*ck. I had a commit amend that I must have overriden with a force push from my other pc. Fixing asap.

gibbz00 commented 1 year ago

@c1wren Hey again, think that should be resolved now. Thanks for the heads up.

c1wren commented 1 year ago

@gibbz00 I can confirm that it is resolved.

Jasperav commented 1 year ago

Is this gonna be merged since there isn't an update in 2 weeks?

Jasperav commented 1 year ago

@gibbz00 I see that this branch is out of sync now with base. Do you have any information if a maintainer even looks at this PR?

curquiza commented 1 year ago

Hello everyone here We see all your PRs, and we thank you for your work! However it's a huge work and we cannot prioritize these reviews at the moment We will come back to review and define what we want when we will have time for this 😊

Thank you again for your work

gibbz00 commented 1 year ago

I'll also try to rebase with master whenever time allows for it ✌️

emilk commented 8 months ago

Just to chime in: I appreciate a lot that this PR removes the dependency on openssl, replacing it with rustls ❤️ I really hope it can get merged!

gibbz00 commented 8 months ago

Not needing openssl was my original motivation for this PR 😊

emilk commented 8 months ago

In the end we decided to use ureq and the Meilisearch REST API directly, which saved us from adding hundreds of dependencies: https://github.com/rerun-io/rerun/blob/main/crates/re_build_search_index/src/meili.rs

gibbz00 commented 8 months ago

Huh, cool! Slightly off topic but how come you went for ureq over reqwest?

emilk commented 8 months ago

Huh, cool! Slightly off topic but how come you went for ureq over reqwest?

ureq has much, MUCH fewer dependencies compared to reqwest: https://twitter.com/ernerfeldt/status/1663585346238464002

nicrosengren commented 7 months ago

Just discovered this PR, really hope it gets merged! :) Currently using reqwest and http api directly to avoid another http tree in deps.

Thanks for doing this

irevoire commented 6 months ago

Hello everyone, I’m sorry for the huge delay. Thanks a lot, @gibbz00, for keeping this branch up to date all this time.

I finally got the time to work on the repo again, especially on https://github.com/meilisearch/meilisearch-rust/pull/459, which was, in my opinion, the most straightforward version of both PRs for the end user. It changes many things here and there, but in the end, if you want to create a new client, it’s just one simple method to implement. In https://github.com/meilisearch/meilisearch-rust/pull/563, I also switched from Isahc to Reqwest as the default http-client.

Hopefully, that solves the issue for everyone.

I’ll close this PR for now, but feel free to make propositions or open PRs on the repo.

jackpeters667 commented 6 months ago

@irevoire When's the planned release for a version with these changes in crates.io? The latest version there is 0.25.0 and that was before these changes made it to main. I'm going to patch if for now, but it would be nice to know when to expect the next rust sdk release

irevoire commented 6 months ago

Hey, it was planned for last week initially, but there is something not ideal with the client's usage. I'm going to fix it this week or next week, I think, and do a release right after

meilisearch / meilisearch-rust

Use reqwest as the request client backend. #524