Rows does not implement database/sql/driver.Rows

[bhcleek]

The new Rows struct is not backward compatible with the previous Rows interface; it's missing all of the driver.Rows methods. T

he changelog and commit message from #68 leads one to believe that was an inadvertent change.

[l3pp4rd]

Yes indeed, need to mention that. did not made any sense to keep the driver.Rows compatibility since it is not needed when mocking rows to return from driver. and though, changing from sqlmock.Rows interface to struct could also lead in some backward incompatible changes if interface was used in helper functions using the reference. There had to be changes though, due to 1.8 support of multi result sets. Hope it did not cause much of the trouble? Will add notes in readme and the pull request referenced, cheers.

[bhcleek]

It did cause a little bit of heartburn for us; it was tedious to adjust, but not difficult.

Is there any chance the exported Rows type could be changed back to an interface in order to minimize backward incompatibility?

[l3pp4rd]

well, I cannot change this back, since the release was already made and that would make again - backward incompatible change. though could have tagged it with 2.0.0 instead, but haven't thought that many users would refer to sqlmock.Rows by type somewhere in tests. anyway, a mistake was done and users have already adapted their code. I've made the release tag description with notification, hope to avoid such mistakes in the future, guess there won't be so many changes to the sql.driver next times.

[bhcleek]

Thank you for the quick response and for updating the release description.

I propose that the calculus is one of whether the bulk of users has upgraded to the most recent release. If most have upgraded and they had to change a reference to sqlmock.Rows to *sqlmock.Rows, then changing it back to an interface would be problematic for most users. Conversely, if most users have not upgraded, then changing Rows to an interface would beneficial for most.

Our use case for referencing sqlmock.Rows has been in table driven tests where the test struct type has a field of sqlmock.Rows.

[l3pp4rd]

I will not revert, because people have already upgraded and resolved these changes, I will not inflict that change again (that is the worst thing I can do now). Just migrate to the pointer.

[snargleplax]

Hey, I don't expect a change here but I wanted to chime in just to document another pain point associated with the removal of the interface. Because sqlmock.Rows doesn't implement database/sql/driver.Rows, I can no longer use it to pass to functions that expect the latter as a parameter.

My specific use case is in mocking out the parameter to "github.com/bhcleek/sqldecoder".NewDecoder, which accepts an interface that is a subset of database/sql/driver.Rows. I actually submitted an (accepted) PR to sqldecoder specifically to have NewDecoder accept an interface for the purpose of mocking, only to subsequently run afoul of sqlmock.Rows no longer implementing that interface.

I've been unable to find a way to adapt things to fit, so I'm having to give up on testing the function in question directly and instead factor out its guts and test those, leaving a thin layer of logic without test coverage. Or is there a way around this that I'm missing?

[l3pp4rd]

Hi @snargleplax sqlmock supports sql driver interface. I do not understand how you use Rows? Because when you mock query to return rows it will always return the Rows interface:

mock.ExpectQuery("SELECT (.+) FROM articles WHERE id = ?").
		WithArgs(5).
		WillReturnRows(rs)

	rows, err := db.Query("SELECT (.+) FROM articles WHERE id = ?", 5)

In this example rows implements sql.driver.Rows as defined by sql interface. Maybe you used Rows mock for this purpose? The change was due to new feature for go 1.8, which supports multi result sets for Queries. Not sure if that would be possible to put it back now

[snargleplax]

Hi @l3pp4rd, thanks for the response. My use case is a bit different than what you illustrate; it's more like this:

// foo.go
import (
	"github.com/bhcleek/sqldecoder"
)

// code under test
func processRows(decoder *sqldecoder.Decoder) error {
	// ... do stuff ...
	return nil
}

// foo_test.go
import (
	"testing"
	"github.com/DATA-DOG/go-sqlmock"
	"github.com/bhcleek/sqldecoder"
)

func TestProcessRows (t *testing.T) {
	rows := sqlmock.NewRows([]string{"header1", "header2"}).AddRow("banana", "bonanza")
	err := processRows(sqldecoder.NewDecoder(rows))
	if err != nil {
		t.Errorf("got err: %q", err)
	}
}

Previously, when the sqlmock.Rows type returned by AddRow was an interface that embedded driver.Rows, this all would have worked. Now I can't see how to make it happen so I'm having to refactor to work around it and leave the direct usage of the Decoder untested.

[snargleplax]

I guess I could also define my own Decoder interface and stub that out, so I do have that option as well if there's no better path here.

[bhcleek]

@snargleplax sqldecoder is intended to unmarshal "database/sql".Rows, not "database/sql/driver".Rows, so the rows value in your example shouldn't be passed directly to sqldecoder.NewDecoder. Instead, something like this should work:

func TestProcessRows (t *testing.T) {
       db, mock, err := sqlMock.New()
       if err != nil {
           t.Fatal(err)
       }
       defer db.Close()

       mock.ExpectQuery("SELECT").WillReturnRows(sqlmock.NewRows([]string{"header1", "header2"}).AddRow("banana", "bonanza")...)
       rows, err := db.Query("SELECT")
       if err != nil {
           t.Fatal(err)
       }
   
	err := processRows(sqldecoder.NewDecoder(rows))
	if err != nil {
            t.Errorf("got err: %q", err)
	}
}

[l3pp4rd]

thanks @bhcleek that was my point, sqlmock.NewRows should be used only for mocking sql database Query result Rows. Maybe that is a little confusing, since in general you create rows for sql database driver which should return it from Query, but that is exactly what sqlmock is meant for, to unit test sql database interactions.
Since it was implementing sql.driver.Rows before, it could have worked for a while if interface was used to satisfy the need for decoder you use, but that should not be the use case. And as far as I see now, sqldecoder you use, needs Scanner interface which sql.driver.Rows interface does not provide. They are not compatible

[snargleplax]

That did the trick, thanks guys. One more type named Rows in the mix than I had realized. :)