Fix CDC URI parsing for Cloud account usernames and special characters#25002
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes because the updated test code does not compile as written.
openDbWithConnector is now assigned from sql.OpenDB, so its signature is func(driver.Connector) *sql.DB.
But in Test_tryConn, the new gostub callback still returns db, nil instead of just db.
So the test stub has the wrong arity and the file will fail to compile. I think the stub body just needs to return db.
|
|
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
LGTM. The current head fixes the previous blockers I requested changes for:
- Connection creation now stays on github.com/matrixorigin/mysql.NewConnector, matching the executor's mysql.ReuseQueryBuf named-argument path.
- The tryConn/openDbWithConnector test stub now matches sql.OpenDB's actual signature.
- URI parsing now uses the terminal @ as the host separator and the terminal : inside user-info as the user/password separator, with percent-decoding for both username and password. That covers MatrixOne Cloud-style account:user:role credentials and passwords containing @.
- The new fake MySQL-server regression exercises ExecSQL after tryConn and verifies that a COM_QUERY is emitted, so the connector + ReuseQueryBuf execution path is covered instead of only testing config construction.
Local verification on the current head:
CGO-enabled go test ./pkg/cdc -run 'Test_(compUriInfo|makeMysqlConfig|openDbConn|openDbConnFailed|tryConn)|TestExecutor_ExecSQLAfterTryConnUsesReuseQueryBuf' -count=1
One remaining non-code blocker is external to this change: CLA assistant is still pending, and the branch is currently behind main.
Merge Queue Status
This pull request spent 3 hours 47 minutes 53 seconds in the queue, including 1 hour 10 minutes 12 seconds running CI. Required conditions to merge
|
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #24833
What this PR does / why we need it:
What Changed
This PR fixes CDC connection handling for MatrixOne Cloud-style credentials, where the username may contain multiple colons such as
account:user:role, and the password may contain special characterssuch as
@.Changes include:
@as the user-info/host separator.:in user-info, preserving usernames likeaccount:user:role.mysql.Configandmysql.NewConnectorinstead of manually formatting a DSN string, avoiding DSN parsing issues with:in usernames.@, and connection config generation.Testing
gofmtgit diff --checkgo test ./pkg/cdc -run 'Test_(compUriInfo|makeMysqlConfig|openDbConn|tryConn)' -count=1could not complete in my local environment because required CGO headers are missing:usearch.hxxhash.hCGO_ENABLED=0is also blocked bympoolrequiring CGO-backed allocator symbols.