URL.render_as_string doesn't always give an URL that is parsed same way

[AlexeyDmitriev]

Describe the bug

So the code below fails because database is not escaped

Optional link from https://docs.sqlalchemy.org which documents the behavior that is expected

No response

SQLAlchemy Version in Use

2.0.1

DBAPI (i.e. the database driver)

sqlite

Database Vendor and Major Version

sqlite

Python Version

3.11

Operating system

linux

To Reproduce

import sqlalchemy
from sqlalchemy.engine import URL
first_url = URL.create('sqlite', username=None, password=None, host=None, port=None, database="a?b=c") # I could have this filename
first_url_as_string = first_url.render_as_string()
second_url = sqlalchemy.make_url(first_url_as_string)
assert first_url.database == second_url.database, f"Fail, expected: {first_url.database} found: {second_url.database}"

Error

AssertionError: Fail, expected: a?b=c found: a

Additional context

I didn't see any doc that claims that this should be roundtrip conversion, but it seems it's intended to be because there's escaping of other fields.

[CaselIT]

Hi,

I think this is expected behaviour. A string is interpreted as an url, so the ? delimits the query from the path.

[zzzeek]

it looks like a bug . they are creating the URL programattically. The name of the database is a?b=c. that should round trip.

That said, nobody would ever have a database name like that. so what's the XY problem here? (e.g. what do you actually need to do ?)

[AlexeyDmitriev]

@CaselIT Hi, I might have been confusing:
I understand that if I pass sqlite:///a?b=c then, b=c are parsed as query and it is expected

What in my view is not expected is the fact, that first_url_as_string contains this ? directly (instead of some kind of escaping, e.g urlencoded, etc), so that it would roundtrip well

[zzzeek]

this is the patch, this is for 2.1 only as URL adjustments are extremely high risk

diff --git a/lib/sqlalchemy/engine/url.py b/lib/sqlalchemy/engine/url.py
index 1eeb73a23..7565252c7 100644
--- a/lib/sqlalchemy/engine/url.py
+++ b/lib/sqlalchemy/engine/url.py
@@ -641,7 +641,7 @@ class URL(NamedTuple):
         if self.port is not None:
             s += ":" + str(self.port)
         if self.database is not None:
-            s += "/" + self.database
+            s += "/" + quote_plus(self.database)
         if self.query:
             keys = list(self.query)
             keys.sort()
@@ -888,11 +888,10 @@ def _parse_url(name: str) -> URL:
             query = None
         components["query"] = query
 
-        if components["username"] is not None:
-            components["username"] = unquote(components["username"])
+        for comp in "username", "password", "database":
+            if components[comp] is not None:
+                components[comp] = unquote(components[comp])
 
-        if components["password"] is not None:
-            components["password"] = unquote(components["password"])
 
         ipv4host = components.pop("ipv4host")
         ipv6host = components.pop("ipv6host")

[AlexeyDmitriev]

@zzzeek

I tried to understand whether I needed to escape my path when building an URL as a string (we don't have any strange character in the path to database but I wanted to fix it just in case). I've found that I can use URL class.
But while I was playing with it, I've noticed that behaviour is not what I expected

So, this behaviour doesn't bother me, I've already solved my problem, just decided to report what for me looks like a bug

[CaselIT]

I fear that the patch above will break existing use cases, since I'm not sure what all the various db driver would do in this case.

tried to create a db named "my?db=1" in pg and all kind of tools are failing to connect. Like I can't manage to connect using psql... Psycopg2 works though

[zzzeek]

this is less sensitive than "password". this breaks if someone had a database named "a%3Fb%3Dc" with URL escape symbols in it, that's basically it.

[CaselIT]

sure, probably not a big dial, but needs a breaking change node