issue-1538-troubled-proxy-parsing #1539
Conversation
|
Sounds like a great idea to me, thanks! |
|
Running
|
src/proxy.rs
Outdated
| } | ||
| } else { | ||
| let text = format!("{}", err); | ||
| if text == "URL scheme is not allowed" { |
There was a problem hiding this comment.
Matching on the Display output of an error makes me nervous. In pretty much all error types, the output is not considered stable. Is there no other way to check? Is this check important?
There was a problem hiding this comment.
Yeah. That's not my proudest moment.
The check is important. The domain:port case ends up here. The string comparison was the easiest way to check for that error. I think a better option would be to carry a typed thing instead of a string. There's some risk but the reward is a better solution. I'll poke at it for a bit.
There was a problem hiding this comment.
Oh, if that error is owned by the reqwest crate, we can just make it a private unit struct, instead of a string, and then check via downcasting.
There was a problem hiding this comment.
Yup. Just pushed the update. Windows looks good. Checking Linux. Running cargo fmt. All good!
…dScheme instead of the error text to determine that a scheme is not present.
|
:woohoo: Thanks! |
Background
We have a desire for URL mistakes to result in errors when calling
Proxy::http.Change
Check for scheme during URL parse error handling. Lots of unit tests.
Fixes #1538.