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(uri-parsing): handle password with bracket in connection url #9466

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

grieve54706
Copy link
Contributor

Description of changes

If the password is with a bracket in the connection url, urllib.parse.urlparse(url) will raise the error ValueError: Invalid IPv6 URL in backends.connect(). But if the caller quotes the password to avoid parsing failing, ibis does not unquote back the password.

Reproducible example

import ibis
import pytest

invalid_password = "invalid_password["
with pytest.raises(ValueError) as e:
    ibis.connect(f"postgres://username:{invalid_password}@localhost:5432/dbname")
assert str(e.value) == "Invalid IPv6 URL"

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

Thanks for putting this in @grieve54706 !

I marked a few spots that are causing the CI failures

@@ -17,7 +17,7 @@
from pathlib import Path
from typing import TYPE_CHECKING, Any
from urllib.parse import parse_qs, urlparse
from urllib.request import urlretrieve
from urllib.request import unquote_plus, urlretrieve
Copy link
Member

Choose a reason for hiding this comment

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

I think most of the CI failures stem from this import, which should be from urllib.parse

@@ -82,7 +82,7 @@ def _from_url(self, url: str, **kwargs) -> BaseBackend:

connect_args = {
"user": url.username,
"password": url.password or "",
"password": unquote_plus(url.password) or "",
Copy link
Member

Choose a reason for hiding this comment

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

the password attribute can be None depending on the connection string, so there needs to be a None check before calling unquote_plus

@grieve54706
Copy link
Contributor Author

@gforsyth Thanks for your suggestion. You saved my life.

@grieve54706 grieve54706 requested a review from gforsyth June 29, 2024 08:22
@cpcloud cpcloud changed the title fix: handle password with bracket in connection url fix(uri-parsing): handle password with bracket in connection url Jun 30, 2024
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

LGTM, I'll do a few cleanups of the excepting string matching in #9482

@cpcloud cpcloud added this to the 9.2 milestone Jul 1, 2024
@cpcloud cpcloud added bug Incorrect behavior inside of ibis ux User experience related issues labels Jul 1, 2024
@cpcloud cpcloud merged commit c73bcf0 into ibis-project:main Jul 1, 2024
80 checks passed
@cpcloud
Copy link
Member

cpcloud commented Jul 1, 2024

@grieve54706 @gforsyth Is there a corresponding issue for this PR? NBD if not, just wondering!

@gforsyth
Copy link
Member

gforsyth commented Jul 1, 2024

I don't think there was

@grieve54706
Copy link
Contributor Author

Nope. Thank you for your time.

@grieve54706 grieve54706 deleted the bugfix/url-quote branch July 2, 2024 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior inside of ibis ux User experience related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants