Skip to content

result: don't panic when affectedRows/insertIds are empty#1769

Closed
c-tonneslan wants to merge 1 commit into
go-sql-driver:masterfrom
c-tonneslan:fix/result-empty-slice-panic
Closed

result: don't panic when affectedRows/insertIds are empty#1769
c-tonneslan wants to merge 1 commit into
go-sql-driver:masterfrom
c-tonneslan:fix/result-empty-slice-panic

Conversation

@c-tonneslan
Copy link
Copy Markdown
Contributor

Closes #1733.

mysqlResult.LastInsertId / RowsAffected indexed into their slices without checking length, so any code path that left the slices empty (e.g. an error before the affected-rows packet) panicked the caller. Return 0, nil instead, which matches the database/sql.Result contract for "no rows affected".

Closes go-sql-driver#1733.

mysqlResult.LastInsertId / RowsAffected indexed into their slices
without checking length, so any code path that left the result slices
empty (e.g. an error before the affected-rows packet) panicked the
caller. Return 0, nil instead, which matches the database/sql.Result
contract for 'no rows affected'.

Signed-off-by: Charlie Tonneslan <cst0520@gmail.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 18, 2026

Review Change Stack

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: be13f3c9-1584-4c4d-87e7-55b9e0697805

📥 Commits

Reviewing files that changed from the base of the PR and between a065b60 and ef5b26a.

📒 Files selected for processing (2)
  • result.go
  • result_test.go

Walkthrough

This PR fixes a panic in LastInsertId() when called on an empty result by adding a defensive check for empty insertIds slice. The method now returns (0, nil) early instead of unconditionally indexing into the slice. A regression test TestEmptyResult verifies both LastInsertId() and RowsAffected() return zero values safely on empty results.

Changes

Empty result panic fix

Layer / File(s) Summary
Empty result handling and test coverage
result.go, result_test.go
LastInsertId() adds an early return for empty insertIds, replacing unsafe slice indexing. TestEmptyResult regression test validates that both LastInsertId() and RowsAffected() return zero values without error when called on an empty result.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: preventing panics in affectedRows/insertIds when slices are empty, which is the core issue being fixed.
Description check ✅ Passed The description is directly related to the changeset, explaining the problem (panic from unchecked slice access) and the solution (returning 0, nil for empty slices).
Linked Issues check ✅ Passed The PR directly addresses issue #1733 by adding length checks to prevent panics in RowsAffected and LastInsertId when slices are empty, matching the linked issue's requirement.
Out of Scope Changes check ✅ Passed All changes are in-scope: the slice-length safety checks in result.go and the regression test in result_test.go directly address the panic issue without introducing unrelated modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

Comment thread result_test.go

// Regression for #1733: indexing into an empty result slice used to panic.
func TestEmptyResult(t *testing.T) {
r := &mysqlResult{}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide reproducible steps for users.

@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 82.718% (+0.02%) from 82.701% — c-tonneslan:fix/result-empty-slice-panic into go-sql-driver:master

@c-tonneslan
Copy link
Copy Markdown
Contributor Author

Honestly, I went hunting for a real user-level repro on master and I can't get there. Every path that returns a result goes through readResultSetHeaderPacket first, which appends to both slices before any error check, so by the time RowsAffected runs the slices are length >= 1.

The #1733 report is on v1.8.0 and the stack doesn't show how the conn ended up with empty slices, so I can't put a faithful repro in the test. I'll close this and defer to you and @methane on whether the guard is worth adding without one.

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 this pull request may close these issues.

RowsAffected panic

3 participants