-
Notifications
You must be signed in to change notification settings - Fork 86
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
Fix url splitting #1989
Fix url splitting #1989
Conversation
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.
Lg
Merging early since this is needed for enabling API keys for one of the solvers and makes managing our current infra config easier. |
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.
It's not entirely clear to me why we even differentiate between base and solve_path in the HTTP API constructor? It looks like we reuse base to append "/notify". Probably better to have the origin extraction logic there?
But this works as well.
/// Path that were split like this can be joined to the original URL using | ||
/// [`join`]. | ||
pub fn split_at_path(url: &Url) -> Result<(Url, String)> { | ||
let base = format!( |
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.
nit: If I'm not mistaken, this should be the same as url.origin().unicode_serialization
https://docs.rs/url/latest/url/struct.Url.html#method.origin
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.
Was looking for something like this but couldn't find it. 👍
The idea was to have the legacy wrapper just be a wrapper and not require changes for a component we are phasing out anyway. |
Description
Fixes #1988
Changes
Added a helper function to split a URL into base and path which is used for the
legacy
solver adapter.How to test
added a unit test