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

Parse returns postgres scheme for file protocol when directory in path exists #35

Open
GeorgeMac opened this issue Dec 5, 2023 · 11 comments

Comments

@GeorgeMac
Copy link

Since 0.19.1 dburl.Parse has been returning the postgres scheme when provided with a file: scheme path.
This only appears to occur when the path contains a directory that exists on the local filesystem.

u, _ := dburl.Parse("file:file.db")
fmt.Println(u) // prints: sqlite3:file.db

u, _ = dburl.Parse("file:/unknown/file.db")
fmt.Println(u) // prints: sqlite3:/unknown/file.db

u, _ = dburl.Parse("file:/etc/file.db")
fmt.Println(u) // prints: postgres:///etc/file.db
@GeorgeMac
Copy link
Author

For reference, dependabot has been trying to update to the latest version in our repo here flipt-io/flipt#2483

It is failing because a bunch of our integration tests try and boot with a path like this and suddenly our apps are trying to connect to a postgres that does not exist.

@kenshaw
Copy link
Member

kenshaw commented Dec 5, 2023

@GeorgeMac I apologize. I was concerned something like this might happen. The issue is that I decided to make the disk paths more friendly for the databases that use those (SQLite3, MySQL, PostgreSQL, DuckDB).

@kenshaw
Copy link
Member

kenshaw commented Dec 5, 2023

Oops, accidentally hit the 'Comment' button before I was finished.

There actually is a way to turn this off already, by changing the dburl.Stat func, but I will add a way to turn this off. I apologize.

You may want to fine tune your dependabot configs/settings; personally I hate the thing and turn it off in almost every project I work on.

@kenshaw
Copy link
Member

kenshaw commented Dec 5, 2023

I have a quick fix/change that will allow you to disable this behavior in your code. It's incoming, give me a bit. Again, apologies.

@kenshaw
Copy link
Member

kenshaw commented Dec 5, 2023

I just pushed v0.20.0 -- you can now disable the opening of paths on disk like the following:

import "github.com/xo/dburl"

func init() {
    dburl.ResolveSchemeType = false
}

@GeorgeMac
Copy link
Author

Nice one @kenshaw ! No worries, thank you for the speedy resolution!

@GeorgeMac
Copy link
Author

Just to close the loop on this: flipt-io/flipt#2533

I ended up overriding dburl.Stat as dburl.ResolveSchemeType = false led to all the cases above producing an unknown extension error. In my case, I am sinking the file not found errors so that it switches back into then just observing the files extension instead.
I think perhaps this might not be in the spirit of the latest changes, but appears to be more compatible with the behaviour we were depending on before.

@kenshaw
Copy link
Member

kenshaw commented Dec 13, 2023

Can you share with me what file extensions, and what driver you're expecting to open? I apologize for the change in behavior, but I updated dburl to handle more databases, one of which also handled opaque (ie, file://) URLs (specifically, DuckDB). I'd like to work with you to get the right behavior, I just need to know what the old behavior was.

@GeorgeMac
Copy link
Author

GeorgeMac commented Dec 13, 2023

Totally, the examples above are the only extensions and file type (sqlite3) that we depend upon.
Each of those cases in the issue description up top produces extension unknown errors when I set dburl.ResolveSchemaType = false. They each are just .db.

For context, we do also support postgres, mysql and cockroach drivers. However, I don't think we've necessarily ever communicated to folks that they can use file scheme for anything other thank sqlite.

@kenshaw
Copy link
Member

kenshaw commented Dec 14, 2023

Question, do you actually use the postgres driver? If the only thing your app supports is sqlite3, you could just disable every other driver through calls to Unregister.

@GeorgeMac
Copy link
Author

We do use postgres yes, and mysql and cockroach. We just have historically communicated the explicit postgres:// scheme for use with this driver. We haven't communicated that file can be used for anything other than identifying the sqlite file location. As in, we haven't communicated that it could be used to identify a postgres or mysql socket target.

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

No branches or pull requests

2 participants