-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom Client Constructors #43
Conversation
It would be great as well if reqwest was updated to the latest version. I caught it right after submitting the pr. |
I have the same issue as you described in #42. The solution with setting the user agent also works for me (maybe a short help with this info should be added to
Furthermore, should reqwest be re-exported so that one does not have to fiddle with the correct version? |
The tests seem to suffer from the same problem, we would need another solution here too. |
You just need to specify a user agent to fix the 429 response issue:
|
/// ``` | ||
pub fn new_w_proxy(url: &str, auth: Option<(&str, &str)>) -> Result<Self, YahooError> { | ||
let client = if auth.is_some() { | ||
let auth = auth.expect("Conditioned"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, use something like .map_err(...)?
instead. It is no good practice to let a library panic.
let proxy = reqwest::Proxy::all(url)? | ||
.basic_auth(auth.0, auth.1); | ||
|
||
reqwest::Client::builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Client::builder()
instead of reqwest::Client::builder()
. Client
is already imported and direct reference here uses only the non-blocking
Client implementation.
.proxy(proxy) | ||
.build()? | ||
} else { | ||
reqwest::Client::builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
|
||
} | ||
|
||
pub fn new_w_client(client: Client) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment here.
Thank your for your pull request. I have added some comments |
I have adopted your suggestion and it does seem to solve the problem |
@xemwebe Will there be a new version published with this fix soon? |
Just did so. |
@austinjp17: The 429 response issue seems to solved by specifying a user agent. If you are still interested in this PR (e.g. for adding the additional constructors), please update your PR to solve the open issues, otherwise I would reject this PR. |
Using the new constructor with the predefined agent "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/122.0.0.0 Safari/537.36" worked again for a few days then returned to 429s. I am far from a networking professional but imagine it's because every crate user is requesting from under the same agent. I have returned to using a unique app id string as the agent and haven't had any request rejections since. Let me clean it up and I'll update. |
I may be that yahoo takes some measures to identify and block users who try to download an excessive amount of data. Maybe some randomization is required, or using your own custom user agent (which is possible now). |
Are you still interested in the PR or could this be closed? |
A unique user agent cleaned it all up for me, can be closed. Appreciate your effort on this |
Added two new YahooConnector constructors. One using a parameter passed client, giving the user complete discretion over the client build. The second a shortcut to create a proxy-tunneled client.
After a week or so testing I ran into persistent 429 responses, setting the user agent fixed this for me. I assume the proxy would as well but didn't try it.