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

DSN: support / in database name #1483

Closed
derekperkins opened this issue Sep 28, 2023 · 9 comments
Closed

DSN: support / in database name #1483

derekperkins opened this issue Sep 28, 2023 · 9 comments

Comments

@derekperkins
Copy link

Issue description

I use Vitess as a sharded MySQL instance. To connect to a specific shard of the database, the syntax expects dbname/shardname in the DSN, which looks something like this

mysql://username:password@host:1234/dbname/0?interpolateParams=true

I'm able to successfully use this style of DSN in other language drivers - specifically Python via mycli.

Someone brought up a similar issue in #1193 that was closed. I can see in the parsing code that it relies on finding the last slash
https://github.com/zihengCat/mysql/blob/18e71cd4a02b285470363e0f4ddbe58b549230a7/dsn.go#L295-L297

Would you be open to a PR that supported a slash in the db name?

Related issue:

@methane
Copy link
Member

methane commented Sep 29, 2023

I'm open, but please don't assume I will merge it.
Please be clear about edge cases and backward compatibility.

@derekperkins
Copy link
Author

Do you feel that the current test cases are sufficient to cover edge cases and backwards compatibility?

@methane
Copy link
Member

methane commented Sep 29, 2023

I don't know.

@dolmen
Copy link
Contributor

dolmen commented Oct 17, 2023

@derekperkins Have you tried filling a Config (use NewConfig) with your settings and call its FormatDSN method to see what it outputs? I expect that your case is already handled (I see a call to url.PathEscape in the code).

Here is a template: https://go.dev/play/p/8E3WFHsTTJc

@derekperkins
Copy link
Author

@dolmen I'm not using this driver directly, I'm configuring the backing grafana database. That being said, I've been using the single url option, so maybe setting the individual fields will have a better outcome

https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#url

@dolmen
Copy link
Contributor

dolmen commented Oct 17, 2023

@derekperkins mysql://username:password@host:1234/dbname/0?interpolateParams=true is not a valid DSN for the MySQL driver. Our DSN doesn't have that mysql:// prefix. It looks like Grafana has another DSN layer over it. You should start investigating there first.

@dolmen
Copy link
Contributor

dolmen commented Oct 17, 2023

Our README says:

dbname is escaped by PathEscape() since v1.8.0. If your database name is dbname/withslash

... but v1.8.0 is not yet released.

This is commit d3e4fe6 from #1432.

So the issue is fixed, but not released yet.

@derekperkins
Copy link
Author

Looks like Grafana does a url parse
https://github.com/grafana/grafana/blob/41e3b3bea2d91f9da38d65bfee89f321499ea3f9/pkg/services/sqlstore/sqlstore.go#L471-L491

Then fmt it into a go-sql-driver compatible DSN
https://github.com/grafana/grafana/blob/16d0aff26707b80fda408708d1c67bb2fe0586e9/pkg/services/sqlstore/sqlstore.go#L283-L317C33

I also checked with https://github.com/xo/usql, which uses https://github.com/xo/dburl to parse the agnostic string, and it returned the same error from go-sql-driver.

I can test it out directly to see if it works going straight through the driver

@derekperkins
Copy link
Author

So the issue is fixed, but not released yet.

Oh, awesome - sorry for not seeing your comment before I posted. Thanks for tracking down that PR @dolmen and thanks @methane for the implementation!

@methane methane closed this as completed Oct 19, 2023
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

3 participants