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

Incompatibility in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql #264

Closed
aakn opened this issue Aug 14, 2019 · 6 comments · Fixed by #265
Closed

Incompatibility in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql #264

aakn opened this issue Aug 14, 2019 · 6 comments · Fixed by #265

Comments

@aakn
Copy link

aakn commented Aug 14, 2019

Describe the Bug

go-sql-driver has a DSN format that the go-1.12.8 does not consider as a valid URL.
Since the migrate tool parses the DSN before passing it on to the go-sql-driver, it leads to an error with the connection string.

Steps to Reproduce
Steps to reproduce the behavior:

  1. Upgrade go to go-1.12.8
  2. Rebuild the migrate tool
  3. Run the command
    migrate -database "mysql://username:password@(127.0.0.1:3306)/database_name" -path db/migrations up
    
  4. You'll see an unexpected error
    error: database: parse mysql://username:password@(127.0.0.1:3306)/database_name: invalid port ":3306)" after host
    

Expected Behavior
Migrations should've run successfully.

Migrate Version
e.g. v4.5.0

Loaded Source Drivers
file

Loaded Database Drivers
stub, mysql

Go Version
go version go1.12.8 linux/amd64

Stacktrace
NA

@aakn aakn changed the title Incompatibility in the URL's parser with go 1.12.8 and github.com/go-sql-driver/mysql Incompatibility in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql Aug 14, 2019
@lkebin
Copy link

lkebin commented Aug 14, 2019

ditto

erikdubbelboer added a commit to erikdubbelboer/migrate that referenced this issue Aug 14, 2019
Change schemeFromURL to just split the url by :// to find the scheme.
It's not required to parse the whole URL. MySQL DSNs aren't valid URLs.

Fixes golang-migrate#264
@thaJeztah
Copy link
Contributor

Reported the change in behavior in Golang (to confirm it was intentional); golang/go#33646. I'll quote the response here;

golang/go#33646 (comment)

Thanks for reporting this and sorry for the disruption. Indeed, this is unfortunate but I'm afraid necessary. DSNs like that are not well-formed URIs per RFC 3986, which is what net/url targets. All things being equal we will prefer backwards compatibility to standard compliance, but it became clear that allowing malformed ports was having a security impact on applications that assumed RFC 3986 compliance, so unless we break the world here we should stay on the stricter behavior.

I mentioned this change in the golang-announce email, and had I noticed the test failure I would have opened an issue with github.com/go-sql-driver/mysql in advance. If somebody on this issue would like to open an issue there, I'd appreciate it, otherwise I'll look into which of their APIs are affected and let them know this evening.

@dhui dhui closed this as completed in #265 Aug 17, 2019
dhui pushed a commit that referenced this issue Aug 17, 2019
…ql (#265)

* Fix in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql

Change schemeFromURL to just split the url by :// to find the scheme.
It's not required to parse the whole URL. MySQL DSNs aren't valid URLs.

Fixes #264

* The mysql driver itself also used net/url.Parse

* Also fix TestPasswordUnencodedReservedURLChars

* Keep backwards compatibility with url encoded username and passwords

* Fix suggestions

* Reuse old function names
@dhui
Copy link
Member

dhui commented Aug 17, 2019

We won't be able to cut a new release until the builds are fixed: #254

@dhui dhui reopened this Aug 17, 2019
@dnldaniel
Copy link

@dhui what about now? the issue does have impact to my team's software

@dhui
Copy link
Member

dhui commented Aug 20, 2019

Unfortunately, we don't have a centralized communications channel/procedure for issues and the latest status can only be gleaned from looking at the latest comments on related issues and PRs...

Current status:
An issue was reported with that the current fix (already merged into master) is insufficient.
I'm waiting for #271 (the full fix) to be merged before cutting a new release.

If you believe that the fixes in master are sufficient for your needs, go ahead and use the master branch. If you'd like a more stable release, you'll need to wait for the next release. You can always use master for now and cut back to the next release when it's available.

@dnldaniel On a related note, if migrate is that important to your team, any help to improve it would be greatly appreciated. 😉

@dhui
Copy link
Member

dhui commented Aug 20, 2019

Fixed in v4.6.0

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 a pull request may close this issue.

5 participants