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

Allow Connecting to Databases with Slashes in the Name via Url Escaping #1406

Closed
wants to merge 3 commits into from

Conversation

bheni
Copy link
Contributor

@bheni bheni commented Mar 16, 2023

Description

Currently mysql will allow you to create a database with slashes in the name, however this driver does not provide any support for connecting to such databases. This change allows you to use url escaping to include slashes in your database name.

Pic showing mysql allows forward slashes in creation if backtick escaped:
Screen Shot 2023-03-16 at 11 00 33 AM

Checklist

  • [ X ] Code compiles correctly
  • [ X ] Created tests which fail without the change (if possible)
  • [ X ] All tests passing
  • [ X ] Extended the README / documentation, if necessary
  • [ X ] Added myself / the copyright holder to the AUTHORS file

@methane
Copy link
Member

methane commented Mar 17, 2023

Currently mysql will allow you to create a database with slashes in the name, however this driver does not provide any support for connecting to such databases.

This driver supports connecting to such database with NewConfig.

I know this driver's DSN is not perfect and it can not be fixed without breaking backward compatibility.

This change allows you to use url escaping to include slashes in your database name.

But it isn't backward compatible, is it?

@bheni
Copy link
Contributor Author

bheni commented Mar 17, 2023

You are correct, it is not backward compatible. Any DSN with a % in the database name would need to be updated from:

/dbname%withpercent to /dbname%25withpercent

But it still supports those databases with that small change. I can look at other methods of implementing this if that isn't acceptable. But if I took the time to fully reimplement ParseDSN so that this worked properly would you accept the changes? I thought this smaller change, which supports more database name characters, even if not backward compatible, might be more readily accepted.

@@ -196,7 +196,8 @@ func (cfg *Config) FormatDSN() string {

// /dbname
buf.WriteByte('/')
buf.WriteString(cfg.DBName)
dbNameEncoded := url.QueryEscape(cfg.DBName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be PathEscape rather than QueryEscape.
QueryEscape will escape many characters that we don't want to escape.

@methane
Copy link
Member

methane commented Mar 17, 2023

I want to fix DSN design in v2. But there is no plan for v2 now.
Until v2, I am very conservative about breaking changes.
You can use Config object anyway. It is recommended. DSN is legacy.

@methane methane mentioned this pull request May 19, 2023
5 tasks
@methane methane closed this May 25, 2023
@methane
Copy link
Member

methane commented May 25, 2023

Fixed in #1432.

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.

2 participants