dsn: default Net to tcp in NewConfig so Addr-only configs round-trip by c-tonneslan · Pull Request #1770 · go-sql-driver/mysql

c-tonneslan · 2026-05-18T17:29:07Z

Closes #1616.

NewConfig() left Net empty, so building a Config in code with just an Addr and calling FormatDSN produced / instead of tcp(my-address)/. Default Net to tcp (matching the docs) and skip the bare protocol in FormatDSN when there's no Addr, so a fresh NewConfig().FormatDSN() doesn't grow a useless tcp prefix.

Closes go-sql-driver#1616. NewConfig() left Net empty, so building a Config in code with just an Addr and calling FormatDSN produced '/' (no tcp prefix, no address) instead of 'tcp(my-address)/'. Default Net to 'tcp' to match the docs, and skip the bare protocol in FormatDSN when there's no Addr so a fresh NewConfig().FormatDSN() doesn't grow a useless 'tcp' prefix. Signed-off-by: Charlie Tonneslan <cst0520@gmail.com>

coderabbitai · 2026-05-18T17:30:07Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 97adfbca-281c-4c62-8745-b46b9656bfaf

📥 Commits

Reviewing files that changed from the base of the PR and between 02c50c9 and 2db657f.

📒 Files selected for processing (1)

dsn_test.go

Walkthrough

FormatDSN now omits the (address) suffix when Addr is empty; it defaults Net to "tcp" when Net is empty and emits net(cfg.Addr) accordingly. New tests validate behavior for Net-only and Addr-only configurations.

Changes

DSN formatting

Layer / File(s)	Summary
FormatDSN address gating and Net fallback `dsn.go`, `dsn_test.go`	`(*Config).FormatDSN` now only writes the `protocol[(address)]` segment when `cfg.Addr` is non-empty; if `cfg.Net` is empty it is treated as `"tcp"` before emitting `net(cfg.Addr)`. Tests `TestFormatDSN_NetWithoutAddr` and `TestFormatDSN_AddrWithoutNet` validate Net-only and Addr-only formatting.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: defaulting Net to tcp in NewConfig so Addr-only configs round-trip correctly.
Description check	✅ Passed	The description is clearly related to the changeset, explaining the problem, the fix, and its rationale with specific examples.
Linked Issues check	✅ Passed	The PR fully addresses issue `#1616` by defaulting Net to tcp in NewConfig and adjusting FormatDSN to handle Addr-only configs correctly while preserving round-trip behavior.
Out of Scope Changes check	✅ Passed	All changes are within scope: FormatDSN logic updates and test additions directly address the requirements from issue `#1616` with no extraneous modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coveralls · 2026-05-18T18:35:29Z

coverage: 82.731% (+0.03%) from 82.701% — c-tonneslan:fix/newconfig-default-net into go-sql-driver:master

Copilot

Pull request overview

This PR updates DSN formatting defaults so code-built configs with only Addr set include the default TCP network, addressing issue #1616.

Changes:

Sets NewConfig().Net to "tcp".
Changes FormatDSN to avoid emitting a bare protocol when no address is set.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+	if len(cfg.Net) > 0 && len(cfg.Addr) > 0 {
 		buf.WriteString(cfg.Net)
-		if len(cfg.Addr) > 0 {
-			buf.WriteByte('(')
-			buf.WriteString(cfg.Addr)
-			buf.WriteByte(')')
-		}
+		buf.WriteByte('(')
+		buf.WriteString(cfg.Addr)
+		buf.WriteByte(')')


methane · 2026-05-19T04:20:31Z

+	if len(cfg.Net) > 0 && len(cfg.Addr) > 0 {
 		buf.WriteString(cfg.Net)


Suggested change

if len(cfg.Net) > 0 && len(cfg.Addr) > 0 {

buf.WriteString(cfg.Net)

if len(cfg.Addr) > 0 {

net := cfg.Net

if net == "" {

net = "tcp"

}

buf.WriteString(net)

Good call, applied. I also dropped the NewConfig change since defaulting inline here covers Config{} literals too.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@dsn.go`:
- Around line 267-275: The current emission block only runs when cfg.Addr is
non-empty, dropping an explicit cfg.Net when Addr is empty; update the block so
it runs when either cfg.Net != "" or cfg.Addr != "" (e.g., if cfg.Net != "" ||
len(cfg.Addr) > 0), compute net := cfg.Net and default to "tcp" only if net ==
"" as before, then write buf.WriteString(net) and only add the '(' + cfg.Addr +
')' wrapper when cfg.Addr is non-empty (so preserve Net-only configs like
Net:"unix" while keeping existing behavior when Addr is present).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c031631b-5289-4a4c-998d-99a892adbf72

📥 Commits

Reviewing files that changed from the base of the PR and between cc7b8e9 and b0823bb.

📒 Files selected for processing (1)

dsn.go

Signed-off-by: Charlie Tonneslan <cst0520@gmail.com>

c-tonneslan · 2026-05-19T09:27:50Z

Good catch @coderabbitai. Restored the explicit-Net-without-Addr emission so non-default protocols like unix still round-trip — gated on cfg.Net != "tcp" since dropping a default-equivalent tcp on its own is harmless (ParseDSN of "" yields the same Net:"tcp" Config). Added TestFormatDSN_NetWithoutAddr covering unix-only, tcp-only, and empty-only cases. Force-pushed.

coderabbitai · 2026-05-19T09:28:28Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the current PR diff
gh pr view 1770 --repo go-sql-driver/mysql --json files,headRefName

# Get the current diff
gh pr diff 1770 --repo go-sql-driver/mysql

Length of output: 1925

@c-tonneslan Thanks for the follow-up — the updated diff looks correct and well-reasoned. Here's a quick summary of why each case in the new logic holds:

`cfg.Net`	`cfg.Addr`	Output	Why
`""` or `"tcp"`	non-empty	`tcp(addr)`	Defaults Net to `"tcp"`, original bug fixed
`"unix"` (any non-tcp)	non-empty	`unix(addr)`	Uses explicit protocol as-is
`"unix"` (any non-tcp)	empty	`unix`	Preserved — `unix` is non-default and must survive round-trip
`"tcp"`	empty	(nothing)	Safe — `ParseDSN("")` already restores `Net:"tcp"`
`""`	empty	(nothing)	Same as above

The cfg.Net != "tcp" gate in the else if branch is the key insight: since ParseDSN normalises a missing protocol back to "tcp", silently dropping a bare "tcp" is truly lossless. And the three test cases in TestFormatDSN_NetWithoutAddr cover all the meaningful branches of that new condition. The implementation is clean and minimal.

🐇 ✨

coderabbitai

🧹 Nitpick comments (1)

dsn_test.go (1)
333-356: ⚡ Quick win

Consider adding a test for the primary Addr-only scenario.

This test verifies FormatDSN behavior when Addr is empty, but the main issue from #1616 was that setting only Addr (non-empty) produced an incorrect DSN. Consider adding a test case to directly verify that scenario:
func TestFormatDSN_AddrOnly(t *testing.T) {
    cfg := NewConfig()
    cfg.Addr = "myhost:3306"
    got := cfg.FormatDSN()
    want := "tcp(myhost:3306)/"
    if got != want {
        t.Errorf("FormatDSN() with Addr-only = %q, want %q", got, want)
    }
}
This would explicitly verify that the documented default Net="tcp" is preserved when formatting a config with only Addr set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dsn_test.go` around lines 333 - 356, Add a new unit test that covers the
"Addr-only" case so FormatDSN preserves the default Net when only Addr is set:
create a test (e.g., TestFormatDSN_AddrOnly) that calls NewConfig(), sets
cfg.Addr = "myhost:3306" (leaving cfg.Net unset), invokes cfg.FormatDSN(), and
asserts the returned string equals "tcp(myhost:3306)/"; reference FormatDSN and
NewConfig to locate the implementation and add the test alongside
TestFormatDSN_NetWithoutAddr.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@dsn_test.go`:
- Around line 333-356: Add a new unit test that covers the "Addr-only" case so
FormatDSN preserves the default Net when only Addr is set: create a test (e.g.,
TestFormatDSN_AddrOnly) that calls NewConfig(), sets cfg.Addr = "myhost:3306"
(leaving cfg.Net unset), invokes cfg.FormatDSN(), and asserts the returned
string equals "tcp(myhost:3306)/"; reference FormatDSN and NewConfig to locate
the implementation and add the test alongside TestFormatDSN_NetWithoutAddr.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5e5e55ce-a2c2-4071-9140-a95a1b7a6363

📥 Commits

Reviewing files that changed from the base of the PR and between b0823bb and 02c50c9.

📒 Files selected for processing (2)

dsn.go
dsn_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

dsn.go

Signed-off-by: Charlie Tonneslan <cst0520@gmail.com>

methane requested a review from Copilot May 19, 2026 04:13

Copilot started reviewing on behalf of methane May 19, 2026 04:14 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

Comment thread dsn.go Outdated

Comment on lines +270 to +274

if len(cfg.Net) > 0 && len(cfg.Addr) > 0 {

buf.WriteString(cfg.Net)

if len(cfg.Addr) > 0 {

buf.WriteByte('(')

buf.WriteString(cfg.Addr)

buf.WriteByte(')')

}

buf.WriteByte('(')

buf.WriteString(cfg.Addr)

buf.WriteByte(')')

methane reviewed May 19, 2026

View reviewed changes

coderabbitai Bot reviewed May 19, 2026

View reviewed changes

Comment thread dsn.go

dsn: default Net to tcp inline in FormatDSN instead of NewConfig

02c50c9

Signed-off-by: Charlie Tonneslan <cst0520@gmail.com>

c-tonneslan force-pushed the fix/newconfig-default-net branch from b0823bb to 02c50c9 Compare May 19, 2026 09:27

coderabbitai Bot reviewed May 19, 2026

View reviewed changes

dsn: add direct AddrWithoutNet round-trip test

2db657f

Signed-off-by: Charlie Tonneslan <cst0520@gmail.com>

methane approved these changes May 20, 2026

View reviewed changes

methane merged commit 0182914 into go-sql-driver:master May 20, 2026
37 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dsn: default Net to tcp in NewConfig so Addr-only configs round-trip#1770

dsn: default Net to tcp in NewConfig so Addr-only configs round-trip#1770
methane merged 3 commits into
go-sql-driver:masterfrom
c-tonneslan:fix/newconfig-default-net

c-tonneslan commented May 18, 2026

Uh oh!

coderabbitai Bot commented May 18, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

coveralls commented May 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

methane May 19, 2026

Uh oh!

c-tonneslan May 19, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

c-tonneslan commented May 19, 2026

Uh oh!

coderabbitai Bot commented May 19, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		if len(cfg.Net) > 0 && len(cfg.Addr) > 0 {
		buf.WriteString(cfg.Net)

Conversation

c-tonneslan commented May 18, 2026

Uh oh!

coderabbitai Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coveralls commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

methane May 19, 2026

Choose a reason for hiding this comment

Uh oh!

c-tonneslan May 19, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

c-tonneslan commented May 19, 2026

Uh oh!

coderabbitai Bot commented May 19, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coderabbitai Bot commented May 18, 2026 •

edited

Loading

coveralls commented May 18, 2026 •

edited

Loading