refactor: improve BlockingLoreAPI errors and remove its unwraps

[lorenzoberts]

This PR was built over lorenzo/use-mockall-lib (#45) to avoid dealing with conflicts after it's merge, but they have no direct relation so you can ignore 38e6b14423fd706d64def263ca174da832bbc36d

With that being said, this is another PR which does not include any new functionality, it aims to improve general code quality by removing repetitive code and unnecessary unwraps mostly in src/lore_api_client.rs. The improvements are as follows:

1. Create a centralized error enum (ClientError) for all the client traits (and consequently for the client itself)

I thought it didn't make much sense to have a specific error to each trait, since all of them were focused on handling request errors (expect for EndOfFeed). Besides, by using the thiserror create we can convert errors easily, allowing ClientError to capture all rewquest:Errors and convert it into a ClientError. Also there were also unnecessary unwraps that could be removed by simply using ?.

2. Centralize the steps of making the HTTP request and getting it's body

It was also another point I thought could be improved, since all trait implementations had the same steps to get a response body for given target url.

3. Avoid using reqwest::blocking::get directly

According to reqwest::blocking::getdocs, this method should be avoided since it creates a single client for each of the requests. Even though this will probably not have noticeable changes in performance, I thought it was better to make rewquest::blocking::Client an attribute of BlockingAPIClient so that all requests can use the same HTTP client.

Regarding EndOfFeed, I struggled to decide if it made more sense to implement it in LoreSession or in the API client. In the end I decided to keep it where it was. Lastly, I considered using Arc around BlockingAPIClient, but I realized this only makes a difference in multi-threaded contexts. I don't know if it could help resolve #3, but since I couldn't reproduce this flaky behavior I don't know if it persists.

EDIT: After reading about Rc (the single-threaded equivalent of Arc) and the reqwest client, I found that it can be used to share the same client across structs that need the client. That said, simply cloning the client is also an option since it internally reuses underlying resources, so it doesn't recreate all pools, connections, etc., with every clone. Both solutions are likely better than creating a new client each time, so I will add another commit to implement this. As always, let me know what you think.

Helps #28

[davidbtadokoro]

Hey @lorenzoberts, thanks for yet another great PR!

At first glance, I thought you've only done some quality work on BlockingLoreAPIClient, but after reading the PR description (awesome, btw) and reviewing it, I feel like this is a major change!

Besides the great points you've brought up, which I agree with entirely, I think this has the potential of solving #3, indeed. This issue is one of the more confusing weak points in patch-hub , and I've always suspected it was due to the go-horse way I've implemented this HTTP client. To be honest, I've implemented this right from the outset of patch-hub when I had little to no knowledge of Rust :rofl:

I will test this for real ASAP, and even if it doesn't solve #3, I don't see a reason why not merge. Thanks for the impressive work, and I will hit you back soon.

[davidbtadokoro]

Hey @lorenzoberts, thanks for the wait. The text below isn't about your change, per se; it's just some context about the work I did to try to solve #3 from what you've brought to the table.

My Side Quest

I took more than expected as I was in high hopes of this PR solving #3 collaterally. I had some offline talks with Nelson about the root of the problem, and we were not convinced of any concrete reason. Just to inform you about the matter:

1. Sometimes, it may take 6+ seconds to get a response from Lore API. I found that in those scenarios, for some reason, connecting to a VPN provider, i.e., not changing a single line of code, only the networking conditions, "solves" the problem.
2. When the flaky behavior is in effect, the strange thing is that, through a web browser making the same exact underlying request, we don't observe the problem.

Some time ago, I saw that using reqwest::blocking::get was a really bad idea. From what I've explained above (and the convos with Nelson), it was a long shot that avoiding this instantiation could solve the problem, which was one I was on board to take. It goes without saying that this didn't work; otherwise, I wouldn't be writing this huge text :grimacing:

About the PR itself

I've ended up merging it as it was indeed a huge quality improvement to the lore_api_client mod, both in the quality of code and the actual functionality robustness. I must also give kudos for the great commit messages and overall good practices. I basically didn't alter anything, but, as always, edits are annotated in the commit messages.

Nevertheless, thank you so much for the great work! Change merged into the unstable branch :+1: