feat: implement :serverlist command#630
feat: implement :serverlist command#630dlevy-msft-sql wants to merge 3 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the :serverlist interactive command to discover SQL Server instances via the SQL Browser service, bringing go-sqlcmd to feature parity with ODBC sqlcmd.
Changes:
- Added
:serverlistcommand that queries the SQL Browser service on UDP port 1434 - Refactored server listing logic from
cmd/sqlcmd/sqlcmd.gotopkg/sqlcmd/serverlist.gofor code reuse - Both the
-Lcommand-line flag and:serverlistcommand now share theListLocalServers()function
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/sqlcmd/serverlist.go | New file containing refactored server listing functions (ListLocalServers, GetLocalServerInstances, parseInstances) |
| pkg/sqlcmd/serverlist_test.go | New test file with comprehensive tests for server listing and parsing functionality |
| pkg/sqlcmd/commands.go | Added SERVERLIST command registration and serverlistCommand handler function |
| cmd/sqlcmd/sqlcmd.go | Removed listLocalServers and parseInstances functions (moved to pkg/sqlcmd), updated -L flag to use sqlcmd.ListLocalServers() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Rename variables to follow Go naming conventions: - out_s -> outStr - got_name -> gotName - instdict -> instanceDict - Add argument validation to serverlistCommand
| _, err = conn.Write(bmsg) | ||
| if err != nil { | ||
| // Silently ignore errors, same as ODBC | ||
| return nil | ||
| } | ||
| read, err := conn.Read(resp) | ||
| if err != nil { | ||
| // Silently ignore errors, same as ODBC | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The error handling behavior has changed from the previous implementation. The old code in cmd/sqlcmd/sqlcmd.go printed non-timeout errors to stdout before returning. The new implementation silently ignores all errors. While the comment states this matches ODBC behavior, this represents a user-visible behavior change where network errors (connection refused, permission denied, etc.) that were previously visible to users are now completely silent. Consider whether this is the intended behavior or if at least some classes of errors should still be logged or returned.
| MYSERVER\SQL2019 | ||
| MYSERVER\SQL2022 |
There was a problem hiding this comment.
The example output shown here does not include the two-space indentation that the actual implementation produces. The code in ListLocalServers (serverlist.go:28) formats the output as " %s\n" with two leading spaces. The documentation should accurately show this formatting or the code should be updated to match the documentation.
| MYSERVER\SQL2019 | |
| MYSERVER\SQL2022 | |
| MYSERVER\SQL2019 | |
| MYSERVER\SQL2022 |
| To capture stderr separately (for error logging): | ||
| ```batch | ||
| sqlcmd -Q ":serverlist" 2>errors.log > servers.txt | ||
| if exist errors.log if not "%%~z errors.log"=="0" type errors.log |
There was a problem hiding this comment.
This batch script has a syntax error. The file size test operator %~z only works within a FOR loop context, not directly in an IF statement. A correct alternative would be to use FOR /F to check the file size, or simply check if the file exists and has content using: "if exist errors.log (for %%A in (errors.log) do if %%~zA GTR 0 type errors.log)"
| if exist errors.log if not "%%~z errors.log"=="0" type errors.log | |
| if exist errors.log (for %%A in (errors.log) do if %%~zA GTR 0 type errors.log) |
| func ListLocalServers(w io.Writer) { | ||
| instances, err := GetLocalServerInstances() | ||
| if err != nil { | ||
| fmt.Fprintln(os.Stderr, err) |
There was a problem hiding this comment.
Writing errors directly to os.Stderr creates an inconsistency with the rest of the sqlcmd architecture. When serverlistCommand calls ListLocalServers with s.GetOutput(), errors will bypass the configured error stream (s.GetError()). Consider either: (1) accepting an error writer parameter alongside the output writer, or (2) returning the error and letting the caller handle it. This would make error handling consistent with other commands like connectCommand which use s.GetError() or return errors to be handled by the framework.
| fmt.Fprintln(os.Stderr, err) | |
| fmt.Fprintln(w, err) |
| if len(instanceDict) == 0 { | ||
| break | ||
| } | ||
| // Only add if InstanceName key exists and is non-empty |
There was a problem hiding this comment.
The validation added here (checking if InstanceName exists and is non-empty) is a good improvement over the old implementation. However, consider adding a comment explaining why instances without InstanceName should be skipped, as this represents a defensive measure against malformed SQL Browser responses.
| // Only add if InstanceName key exists and is non-empty | |
| // Only add if InstanceName key exists and is non-empty. | |
| // This defensively skips malformed or partial SQL Browser responses | |
| // that do not include a valid InstanceName for an instance. |
| func TestParseInstances(t *testing.T) { | ||
| // Test parsing of SQL Browser response | ||
| // Format: 0x05 (response type), 2 bytes length, then semicolon-separated key=value pairs | ||
| // Each instance ends with two semicolons | ||
|
|
||
| t.Run("empty response", func(t *testing.T) { | ||
| result := parseInstances([]byte{}) | ||
| assert.Empty(t, result) | ||
| }) | ||
|
|
||
| t.Run("invalid header", func(t *testing.T) { | ||
| result := parseInstances([]byte{1, 0, 0}) | ||
| assert.Empty(t, result) | ||
| }) | ||
|
|
||
| t.Run("valid single instance", func(t *testing.T) { | ||
| // Simulating SQL Browser response format | ||
| // Header: 0x05 followed by 2 length bytes, then the instance data | ||
| data := []byte{5, 0, 0} | ||
| instanceData := "ServerName;MYSERVER;InstanceName;MSSQLSERVER;IsClustered;No;Version;15.0.2000.5;tcp;1433;;" | ||
| data = append(data, []byte(instanceData)...) | ||
|
|
||
| result := parseInstances(data) | ||
| assert.Len(t, result, 1) | ||
| assert.Contains(t, result, "MSSQLSERVER") | ||
| assert.Equal(t, "MYSERVER", result["MSSQLSERVER"]["ServerName"]) | ||
| assert.Equal(t, "1433", result["MSSQLSERVER"]["tcp"]) | ||
| }) | ||
|
|
||
| t.Run("valid multiple instances", func(t *testing.T) { | ||
| data := []byte{5, 0, 0} | ||
| instanceData := "ServerName;MYSERVER;InstanceName;MSSQLSERVER;tcp;1433;;ServerName;MYSERVER;InstanceName;SQLEXPRESS;tcp;1434;;" | ||
| data = append(data, []byte(instanceData)...) | ||
|
|
||
| result := parseInstances(data) | ||
| assert.Len(t, result, 2) | ||
| assert.Contains(t, result, "MSSQLSERVER") | ||
| assert.Contains(t, result, "SQLEXPRESS") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Consider adding a test case for an instance with a missing or empty ServerName to verify that the new validation logic in parseInstances correctly filters out such instances. This would test the defensive behavior added in lines 109-112 of serverlist.go.
| MYSERVER\SQL2019 | ||
| MYSERVER\SQL2022 |
There was a problem hiding this comment.
The example output doesn't show the behavior when the default MSSQLSERVER instance is present. According to the code (serverlist.go:82-83), the default instance produces two entries in the output: "(local)" and the actual server name (e.g., "MYSERVER"). Consider adding an example showing this case, such as " (local)\n MYSERVER\n MYSERVER\SQL2019".
| MYSERVER\SQL2019 | |
| MYSERVER\SQL2022 | |
| (local) | |
| MYSERVER | |
| MYSERVER\SQL2019 |
c103e26 to
db48592
Compare
- :serverlist queries SQL Browser service (UDP 1434) to discover instances - :help displays available sqlcmd commands - Refactored server listing logic to pkg/sqlcmd/serverlist.go for reuse by both -L flag and :serverlist command
db48592 to
5bc6080
Compare
- :serverlist queries SQL Browser service (UDP 1434) to discover instances - :help displays available sqlcmd commands - Refactored server listing logic to pkg/sqlcmd/serverlist.go for reuse
5bc6080 to
8fd1d8f
Compare
Summary
Implements the
:serverlistinteractive command to list available SQL Server instances via the SQL Browser service.Changes
:serverlistcommand that queries the SQL Browser service (UDP port 1434)cmd/sqlcmd/sqlcmd.gotopkg/sqlcmd/serverlist.gofor reuse-Lflag and:serverlistcommand now use the sharedListLocalServers()functionUsage
Motivation
ODBC sqlcmd supports the
:serverlistcommand to discover SQL Server instances. This brings go-sqlcmd to parity with that functionality.Testing
parseInstances()function:serverlistcommand