Skip to content
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

Merged
merged 4 commits into from
Oct 19, 2023
Merged

Fix url splitting #1989

merged 4 commits into from
Oct 19, 2023

Conversation

MartinquaXD
Copy link
Contributor

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

@MartinquaXD MartinquaXD requested a review from a team as a code owner October 19, 2023 07:51
Copy link
Contributor

@sunce86 sunce86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lg

@MartinquaXD
Copy link
Contributor Author

Merging early since this is needed for enabling API keys for one of the solvers and makes managing our current infra config easier.
Feel free to review anyway. I will then address suggestions afterwards.

@MartinquaXD MartinquaXD merged commit 3bb4797 into main Oct 19, 2023
7 checks passed
@MartinquaXD MartinquaXD deleted the fix-url-splitting branch October 19, 2023 10:21
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2023
Copy link
Contributor

@fleupold fleupold left a 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!(
Copy link
Contributor

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

Copy link
Contributor Author

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. 👍

@MartinquaXD
Copy link
Contributor Author

It's not entirely clear to me why we even differentiate between base and solve_path in the HTTP API constructor?

The idea was to have the legacy wrapper just be a wrapper and not require changes for a component we are phasing out anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: legacy adapter scrambles URLs with query parameters
3 participants