fix(mysql): handle brackets in connection URLs#2367
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR fixes bracket character handling in MySQL connection URLs. A new ChangesMySQL URL credential parsing with bracket safety
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds safer MySQL connection URL parsing to support raw [/] in credentials (without breaking IPv6 hosts), and expands unit coverage for URL decoding/override behavior.
Changes:
- Introduces
_parse_mysql_connection_url()to escape brackets in userinfo before parsing. - Updates
_build_mysql_connect_kwargs()to use the new parser andunquotedecoding. - Adds unit tests covering bracketed passwords, percent-decoding, literal
+, IPv6, kwargs overrides, and invalid schemes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/wren/src/wren/connector/mysql.py | Adds URL pre-sanitization and updates connect-kwargs parsing/decoding to handle bracketed credentials safely. |
| core/wren/tests/unit/test_mysql_helpers.py | Adds unit tests validating MySQL URL parsing edge cases and error behavior. |
Comments suppressed due to low confidence (1)
core/wren/src/wren/connector/mysql.py:506
- The error message is inaccurate: the code allows
mysql+pymysqlandmysql+mysqldbin addition tomysql. Update the message to list the supported schemes (or say “must use a MySQL scheme: mysql, mysql+pymysql, mysql+mysqldb”) to avoid misleading users.
if parsed.scheme not in {"mysql", "mysql+pymysql", "mysql+mysqldb"}:
raise WrenError(
ErrorCode.INVALID_CONNECTION_INFO,
"MySQL connection URL must use mysql:// scheme",
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| out: dict = { | ||
| "host": host, | ||
| "port": int(parsed.port) if parsed.port else 3306, | ||
| "user": parsed.username, | ||
| "passwd": unquote_plus(parsed.password) if parsed.password else "", | ||
| "db": parsed.path.lstrip("/") if parsed.path else None, | ||
| "user": unquote(parsed.username) if parsed.username else None, | ||
| "passwd": unquote(parsed.password) if parsed.password else "", |
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def _parse_mysql_connection_url(url: str): |
| class _FakeConnInfoFromUrl: | ||
| def __init__(self, url: str, kwargs: dict[str, str] | None = None) -> None: | ||
| self.connection_url = _FakeConnUrl(url) | ||
| self.kwargs = kwargs |
Summary
Fix MySQL
connectionUrlparsing when the password contains raw[or].Before this change, URL-based MySQL connections could fail during parsing with an invalid IPv6 URL error because
urllib.parse.urlparse()treated brackets in the password as hostdelimiters.
Root cause
The MySQL connector passed the raw connection URL directly to
urlparse(). A URL like:fails before any connection attempt because urlparse() interprets [ / ] in the netloc as IPv6 syntax.
There was also an adjacent decoding issue: MySQL URL userinfo was decoded with unquote_plus, which incorrectly turns literal + into a space.
Changes
add a MySQL-specific URL parse helper that percent-encodes raw [ / ] in userinfo before calling urlparse()
only sanitize the userinfo segment, so valid IPv6 hosts like [::1] still parse correctly
switch MySQL URL decoding from unquote_plus to unquote for:
keep existing scheme validation and kwarg precedence unchanged
Tests
Added unit coverage for:
Issue
Closes #459
Summary by CodeRabbit
Bug Fixes
Tests