grab the last path part to get the "db name" instead of the first pat…#91
grab the last path part to get the "db name" instead of the first pat…#91hernan604 wants to merge 1 commit into
Conversation
3d0d990 to
c89bc92
Compare
|
There's already a connection.t file. With a much cleaner layout. |
…h part. this allows unix paths as the hostname. example: postgresql://user:@/var/run/postgresql:5432/MY_DB_NAME
c89bc92 to
3ee8553
Compare
Nice, thanks for that ! I have moved the relevant tests to connection.t I think this solves part of the problem to be able to use a "file path" as host in the connection string. However, the "host/port" are not detected and also not in the generated "dsn" because of the way Mojo::URL parses using the official regex from RFC 3986 https://github.com/mojolicious/mojo/blob/main/lib/Mojo/URL.pm#L55 ... as seen in test 3ee8553#diff-539ef6730bfa20f7792e8b643d382f181bffc1984fafe4fe7fd506614402814cR96-R97 |
|
Now i'm confused. Is this patch ready for review or not. Do you want to work on further improvements? |
|
IIRC i thought it was initially ready for review. However after the PR creation, i noticed the unix path does not get parsed, so it "works" with a default postgres unix path, but will break with a custom path. However, this PR "could be considered" an improvement as this PR picks the correct "db name". However it does not work a custom unix path. The example path "postgresql://user:@/var/run/postgresql:5432/db_name" is incompatible with how Mojo::URL parses path. So it solves only half the problem (it "accepts" any path and ignores it, but uses the correct db name). I am not sure the best approach to resolve this... Maybe decline the PR and i open another one if i come up with a better solution |
…h part. this allows unix paths as the hostname. example: postgresql://user:@/var/run/postgresql:5432/MY_DB_NAME
Summary
Allow "file paths" as hostname on the dsn.
Example: postgresql://user:@/var/run/postgresql:5432/db_name
Host should be: /var/run/postgresql
DB should be = "db_name"
However, since it grabs the first path part, it considers the db name = "var" as seen in the following test:
Motivation
EXPLAIN WHY YOU BELIEVE THESE CHANGES ARE NECESSARY HERE
References
LIST RELEVANT ISSUES, PULL REQUESTS AND FORUM DISCUSSIONS HERE