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

Build URLs using urljoin #1217

Open
wants to merge 1 commit into
base: master
from
Open

Build URLs using urljoin #1217

wants to merge 1 commit into from

Conversation

@cgoldberg
Copy link
Member

cgoldberg commented Jan 6, 2020

This PR makes a small change to how URL's are created when a Host is specified. Previously, the URL was built by using string concatenation. This changes it to use urljoin.

This fixes the edge case where a host is entered containing a trailing slash (i.e. https://example.com/ rather than https://example.com). Once merged, it should work with or without trailing a slash.

supersedes #1216

@cgoldberg

This comment has been minimized.

Copy link
Member Author

cgoldberg commented Jan 6, 2020

arg.. CI is failing with this change.. need to investigate when I have a few mins.

@TBBle

This comment has been minimized.

Copy link
Contributor

TBBle commented Jan 7, 2020

The second test_wrong_url case failed, because

assert urljoin("telnet://127.0.0.1", "/") == '/'

instead of

assert "%s%s" % ("telnet://127.0.0.1", "/") == 'telnet://127.0.0.1/'

which changed the failure exception from requests.

This is because urljoin knows you can't put a path on a telnet: URL, and since it's job is to fill in missing bits on the second parameter by taking from the first, it does that by taking nothing from the first.

I suspect urljoin might be the wrong thing to use here, unless we can validate the input earlier so that we get a meaningful exception with the input value, rather than a complaint about a / that was not a parameter provided by the user/caller.

Also, I'm suspicious that this is doing the wrong thing anyway, as if the base_url contains a path and does not end in a /, it now loses the last path element, against due to how urljoin is defined.

>>> urljoin("http://127.0.0.1/root/path/", "extra")
'http://127.0.0.1/root/path/extra'
>>> urljoin("http://127.0.0.1/root/path", "extra")
'http://127.0.0.1/root/extra'

I suspect this change will break examples/browse_docs_test.py, for example, due to how urljoin handles absolute paths on the second parameter.

>>> urljoin("https://docs.locust.io/en/latest/", "/")
'https://docs.locust.io/'

It might be better to just use urlsplit, modify the path carefully, and urlunsplit perhaps. That would also let the base_url contain query parameters, if that was a sensible thing to do...

@cyberw

This comment has been minimized.

Copy link
Contributor

cyberw commented Jan 15, 2020

@cgoldberg Looks nice! can you also fix it for FastHttpLocust at the same time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.