Recover from driver panic on GEOGRAPHY/GEOMETRY column types

[Copilot]

• Understand the issue: panic occurs in go-mssqldb when calling ColumnTypes() for GEOGRAPHY/GEOMETRY types
• Add panic recovery wrapper around rows.ColumnTypes() call
• Add unit test for safeColumnTypes function
• Fix code formatting
• Build and verify compilation
• Run unit tests
• Address code review feedback (round 1)
  • Fix documentation placement for safeColumnTypes
  • Improve test documentation to clarify what is being validated
• Address code review feedback (round 2)
  • Remove redundant defer/recover in test
  • Simplify error message to be more general
• Run CodeQL security scanner - no issues found
• Address PR review feedback
  • Use localizer.Errorf instead of fmt.Errorf for consistent localized error messages
  • Make test locale-independent by removing assertion on English error text
  • Update translation catalog with new error message
  • Regenerate translation catalog with Windows-specific strings
  • Fix result set processing logic to prevent formatter state issues
  • Track BeginResultSet state to handle multiple MsgNext messages correctly
  • Return error directly to ensure proper error propagation to callers

Summary of Changes

Latest Change:

• pkg/sqlcmd/sqlcmd.go: Changed error handling to return the error directly (qe = err) instead of using handleError
  • Lines 497-502: When safeColumnTypes fails, set qe = err directly instead of qe = s.handleError(&retcode, err)

Problem Addressed:
The handleError function only returns non-nil errors for mssql.Error types that meet certain severity criteria. For our localizer.Errorf from panic recovery, it returns nil, causing runQuery to return (retcode, nil). Callers like goCommand only treat the batch as failed when err != nil, so this would silently ignore the fatal column type error.

Solution:
Return the error directly as qe = err so callers properly detect the failure and can set appropriate exit codes and stop execution as needed.

Verification:
✅ Build succeeds
✅ All unit tests pass
✅ go vet passes

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Crash when selecting NULL for GEOGRAPHY/GEOMETRY data type</issue_title>
<issue_description>This sequence crashes in v.1.8.1 (and 1.8.0, other versions not tested). Same for GEOMETRY type.
Occurs both on Windows and Linux.

1> declare @v geography
2> select @v
3> go
panic: not implemented makeGoLangScanType for type 240

goroutine 1 [running]:
github.com/microsoft/go-mssqldb.makeGoLangScanType({0xf0, 0x0, 0x21, 0xffff, 0x0, 0x0, {0xc0004d0000, 0xffff, 0xffff}, {
0x0, ...}, ...})
        /go/pkg/mod/github.com/microsoft/[email protected]/types.go:1141 +0xd74
github.com/microsoft/go-mssqldb.(*Rowsq).ColumnTypeScanType(0x1ef3450?, 0x1494100?)
        /go/pkg/mod/github.com/microsoft/[email protected]/mssql.go:1448 +0x185
[...]
```</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

• Fixes Crash when selecting NULL for GEOGRAPHY/GEOMETRY data type #553

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

[Copilot]

Pull request overview

Adds a panic-recovery wrapper around rows.ColumnTypes() to prevent sqlcmd from crashing when the SQL Server driver panics on GEOGRAPHY/GEOMETRY (type 240) metadata retrieval.

Changes:

• Introduces safeColumnTypes() to recover() from panics in rows.ColumnTypes() and convert them into errors.
• Switches runQuery to call safeColumnTypes() instead of rows.ColumnTypes() directly.
• Adds a unit test to verify panics are caught and converted to errors.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
pkg/sqlcmd/sqlcmd.go	Adds safeColumnTypes() and uses it in runQuery to prevent crashes from driver panics during column type inspection.
pkg/sqlcmd/sqlcmd_test.go	Adds a unit test validating the panic recovery behavior of safeColumnTypes().

Comments suppressed due to low confidence (1)

pkg/sqlcmd/sqlcmd.go:521

• If safeColumnTypes returns an error, runQuery still continues with rows.Next()/AddRow() even though BeginResultSet() was not called. This can both emit additional scan errors (Scan called with 0 destinations) and, more importantly, can overwrite the earlier -100 retcode with 0 when col1 == "" at end-of-resultset, masking the failure. Consider short-circuiting result-set processing when ColumnTypes fails (e.g., stop the loop / mark results=false / skip Next+AddRow) so retcode remains -100 and the query terminates cleanly after reporting the error.

			if first {
				first = false
				cols, err = safeColumnTypes(rows)
				if err != nil {
					retcode = -100
					qe = s.handleError(&retcode, err)
					s.Format.AddError(err)
				} else {
					s.Format.BeginResultSet(cols)
				}
			}
			inresult := rows.Next()
			for inresult {
				col1 := s.Format.AddRow(rows)
				inresult = rows.Next()
				if !inresult {
					if col1 == "" {
						retcode = 0
					} else if _, cerr := fmt.Sscanf(col1, "%d", &retcode); cerr != nil {
						retcode = -102
					}
				}
			}
			if retcode != -102 {
				if err = rows.Err(); err != nil {
					retcode = -100
					qe = s.handleError(&retcode, err)
					s.Format.AddError(err)
				}
			}
			s.Format.EndResultSet()

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

pkg/sqlcmd/sqlcmd.go:500

• If safeColumnTypes returns an error (e.g., recovered driver panic), runQuery continues into rows.Next()/Format.AddRow without calling BeginResultSet. That leads to scan errors (columnDetails is empty) and can also overwrite the earlier -100 retcode with 0, incorrectly reporting success. Consider aborting processing of the current result set when column type retrieval fails (e.g., set qe to the error / break out before scanning rows) so the batch reliably fails and doesn’t attempt to format rows without column metadata.

				cols, err = safeColumnTypes(rows)
				if err != nil {
					retcode = -100
					qe = s.handleError(&retcode, err)
					s.Format.AddError(err)
				} else {
					s.Format.BeginResultSet(cols)
				}

[dlevy-msft-sql]

@copilot fix the issue from the review

[Copilot]

@copilot fix the issue from the review

Fixed in commit 97d701d. The test is now locale-independent and will work correctly regardless of the SQLCMD_LANG setting.

[dlevy-msft-sql]

@copilot what do you think about that low confidence review item?

The latest Copilot review (comment 3737005328) indicates no new comments were generated for this PR. All previous review threads have been resolved. I don't see any "low confidence review item" in the current review comments. Could you please point me to the specific review item you're referring to? It may be in a different location or I may need additional context to locate it.

This was the comment:

Comments suppressed due to low confidence (1)

pkg/sqlcmd/sqlcmd.go:501

• If safeColumnTypes returns an error, the code still continues into rows.Next()/Format.AddRow and then calls EndResultSet. Since BeginResultSet is skipped on the error path, the formatter state (e.g., columnDetails) may be uninitialized/mismatched and AddRow can panic (or produce incorrect output) — which defeats the purpose of recovering from the driver panic. Consider short-circuiting when column types cannot be obtained (e.g., stop processing the result set and return the error, or set qe to a non-nil error to exit the loop) and ensure EndResultSet is only called after a successful BeginResultSet.

				cols, err = safeColumnTypes(rows)
				if err != nil {
					retcode = -100
					qe = s.handleError(&retcode, err)
					s.Format.AddError(err)
				} else {
					s.Format.BeginResultSet(cols)
				}
			}

[Copilot]

@copilot what do you think about that low confidence review item?

The latest Copilot review (comment 3737005328) indicates no new comments were generated for this PR. All previous review th...

Fixed in commit 50e47a4. Added continue statement when safeColumnTypes returns an error to skip processing the result set entirely. This ensures BeginResultSet, AddRow, and EndResultSet are only called when column types are successfully obtained, preventing panics from uninitialized formatter state.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[dlevy-msft-sql]

@copilot fix the review items

[Copilot]

@copilot fix the review items

Fixed in commit 9ed202a. Added state tracking to ensure formatter methods are only called when the result set has been properly initialized.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[dlevy-msft-sql]

@copilot fix that review comment

[Copilot]

@copilot fix that review comment

Fixed in commit db9262d. The column type error is now returned directly to ensure proper error propagation to callers.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.