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 whitespace issues in SQLite absolute paths in DATABASE_URL #266

Merged

Conversation

Enrico204
Copy link
Contributor

When the DSN is in the form sqlite:/absolute/path or sqlite://absolute/path or sqlite:///absolute/path, url.Parse
will consider the file path as:

  • absolute as the hostname
  • path (and the rest until ?) as the URL path.
    Instead, when the DSN is in the form "sqlite:", the (relative) file path is stored in the "Opaque" field.
    See: https://pkg.go.dev/net/url#URL

While Opaque is not escaped, the URL Path is. So, in absolute paths the full file path is being escaped as if it was an URL path.
This commit works around this behavior in ConnectionString() (sqlite.go) so that the return string is always a correct (url-unescaped) file path.

Fixes #264 .

When the DSN is in the form "sqlite:/absolute/path" or
"sqlite://absolute/path" or "sqlite:///absolute/path", url.Parse
will consider the file path as :
 - "absolute" as the hostname
 - "path" (and the rest until "?") as the URL path.
Instead, when the DSN is in the form "sqlite:", the (relative) file
path is stored in the "Opaque" field.
See: https://pkg.go.dev/net/url#URL

While Opaque is not escaped, the URL Path is. So, in absolute paths
the full file path is being escaped as if it was an URL path.
This commit works around this behavior in ConnectionString (sqlite.go)
so that the return string is always a correct (url-unescaped) file path.
This commit fixes the error message text assertion for DumpSchema (SQLite)
errors.
The DumpSchema returns an error with the connection string (aka the file
path) when there is an error while opening the database. Specifically, in
this case, the DumpSchema was tested to check whether an invalid file
was triggering the error.

The test is failing because, due to the previous commit, the connection
string is now rebuilt when url.Parse detects an URL path. While other
cases are covered by unit tests, the case of "." as file path was not
tested (no need to, it's not valid). In this case, the ConnectionString
function returns "/." as a connection string instead of ".".

While the ConnectionString behavior is wrong with ".", this fix doesn't
address that, as the connection string will be invalid anyway. So, I fixed
the error message instead.
@Enrico204
Copy link
Contributor Author

I fixed the error in the pipeline by editing an assert for an error. The nature of the error was not altered - the change was actually due to the fact that the error have the path embedded in the string, and now the (invalid) path changed a bit.

(the goal of that test was to assert the error opening a invalid/non existent file, regardless of which file it was. So, there should be no issue with that)

The path change actually comes from my edits in ConnectionString. I decided to avoid fixing ConnectionString because:

  1. the path . is invalid for SQLite [1]
  2. other cases (relative paths, absolute paths) are handled correctly, and tested

[1] the current behavior for . path is now: ConnectionString(".") == "/.". The previous one was ConnectionString(".") == "."

@amacneil
Copy link
Owner

Spaces in a URL aren’t actually valid (should use %20). But this makes sense if people want to copy paste a local path.

@amacneil
Copy link
Owner

Do we have any tests for the sqlite://foo/bar.sqlite (two leading slashes) behavior you described? I can’t see any.

@amacneil amacneil merged commit 3fd6d5c into amacneil:main Jan 30, 2022
@Enrico204 Enrico204 deleted the supports-whitespace-in-path-in-sqlite-driver branch January 30, 2022 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DATABASE_URL doesn't support whitespaces
2 participants