sqlmock panics when closing empty Rows object

[raxod502-plaid]

trafficstars

Operating system and Go Version

macOS 14.4.1, Go 1.20

Issue

When using db.Query to perform a query that does not return any rows, sqlmock panics instead of performing a no-op as other database drivers do.

A workaround is to use db.Exec instead so that there is no rows object to close, but the sqlmock behavior is incorrect.

Reproduction steps

package main

import (
	"github.com/DATA-DOG/go-sqlmock"
)

func assert(err error) {
	if err != nil {
		panic(err.Error())
	}
}

func main() {
	db, mock, err := sqlmock.New()
	assert(err)
	mock.ExpectQuery("TRUNCATE table").WithoutArgs().WillReturnRows()
	rows, err := db.Query("TRUNCATE table")
	assert(err)
	err = rows.Close()
	assert(err)
}

Expected Result

No error (or, at worst, an error returned from rows.Close)

Actual Result

panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
github.com/DATA-DOG/go-sqlmock.(*rowSets).Close(0x14000124040)
	/Users/rrosborough/.asdf/installs/golang/1.20/packages/pkg/mod/github.com/!d!a!t!a-!d!o!g/[email protected]/rows.go:40 +0x64
database/sql.(*Rows).close.func1()
	/Users/rrosborough/.asdf/installs/golang/1.20/go/src/database/sql/sql.go:3287 +0x34
database/sql.withLock({0x100b84d50, 0x14000142000}, 0x14000149eb8)
	/Users/rrosborough/.asdf/installs/golang/1.20/go/src/database/sql/sql.go:3405 +0x7c
database/sql.(*Rows).close(0x14000134180, {0x0, 0x0})
	/Users/rrosborough/.asdf/installs/golang/1.20/go/src/database/sql/sql.go:3286 +0x130
database/sql.(*Rows).Close(0x140001360e0?)
	/Users/rrosborough/.asdf/installs/golang/1.20/go/src/database/sql/sql.go:3270 +0x24
main.main()
	/Users/rrosborough/temp/sqlmockexample/main.go:19 +0x12c

[diegommm]

Hi @raxod502-plaid! Thank you for your report. You are right that it shouldn't panic, that's a bug. It is also true that Exec would be preferable for executing any SQL that will never return any rows, like TRUNCATE.

Quick facts:

• A Query/QueryRow is expected to always return at least one RowSet.
• An Exec is like a Query/QueryRow whose RowSets are discarded (but has other properties).
• The panic happens because it tries to look for the first RowSet and the code doesn't check that there is none. This is the bug you describe.

Correct usage:

• The recommendation for your code is to use Exec as you found out. That's not a workaround, your code will benefit from explicitly stating that you are not expecting any RowSets to be returned, and additionally you may get a sql.Result (with rows affected and last inserted ID) if your driver supports it and if it makes sense for the operation.
• For code that could potentially return rows (like a SELECT, INSERT .. RETURNING, etc.), the correct expectation to be set is WillReturnRows(mock.NewRows(colNames)). This tells the mock that one RowSet is expected.
• It is valid to use WillReturnRows(mock.NewRows(nil)), because it states that you will expect exactly one RowSet with no columns, which is equivalent to an Exec discarding the sql.Result.

Consider a more complex example:

const myQuery := `
    SELECT disabled FROM users WHERE id = ?;
    DELETE FROM logins WHERE created_at < ?;
`
tx.QueryContext(ctx, myQuery, theID, theTimestamp)

That's a query returning two RowSets, the first one with a single column, and the second with none. A test expectation for it would be like:

mock.ExpectQuery(myQuery).WithoutArgs().WillReturnRows(
    mock.NewRows([]string{"disabled"}),
    mock.NewRows(nil),
)

Conclusions:

sqlmock should not panic this way if WillReturnRows is not passed any arguments. Instead, it should provide a help message explaining that it expects at least one (non-nil) argument to WillReturnRows. It should also not panic if NextResultSet is called and there is none provided, but instead fail the test stating that a call to that method was not expected (didn't check if that's already in place, just saying).

[ninadingole]

#314 will fix this issue

[ninadingole]

@raxod502-plaid could you check the issue as the PR is merged now please?