From 5aef51a729f428bfd4b2c28fd2ba7950660608e0 Mon Sep 17 00:00:00 2001
From: Daniel Theophanes <kardianos@gmail.com>
Date: Wed, 18 Mar 2020 10:03:51 -0700
Subject: [PATCH] database/sql: add test for Conn.Validator interface

This addresses comments made by Russ after
https://golang.org/cl/174122 was merged. It addes a test
for the connection validator and renames the interface to just
"Validator".

Change-Id: Iea53e9b250c9be2e86e9b75906e7353e26437c5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/223963
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
---
 src/database/sql/driver/driver.go |  8 ++++----
 src/database/sql/fakedb_test.go   |  4 ++--
 src/database/sql/sql.go           |  4 ++--
 src/database/sql/sql_test.go      | 31 +++++++++++++++++++++++++++++++
 4 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/src/database/sql/driver/driver.go b/src/database/sql/driver/driver.go
index a2b844d71f..76f1bd3aa1 100644
--- a/src/database/sql/driver/driver.go
+++ b/src/database/sql/driver/driver.go
@@ -261,15 +261,15 @@ type SessionResetter interface {
 	ResetSession(ctx context.Context) error
 }
 
-// ConnectionValidator may be implemented by Conn to allow drivers to
+// Validator may be implemented by Conn to allow drivers to
 // signal if a connection is valid or if it should be discarded.
 //
 // If implemented, drivers may return the underlying error from queries,
 // even if the connection should be discarded by the connection pool.
-type ConnectionValidator interface {
-	// ValidConnection is called prior to placing the connection into the
+type Validator interface {
+	// IsValid is called prior to placing the connection into the
 	// connection pool. The connection will be discarded if false is returned.
-	ValidConnection() bool
+	IsValid() bool
 }
 
 // Result is the result of a query execution.
diff --git a/src/database/sql/fakedb_test.go b/src/database/sql/fakedb_test.go
index 73dab101b7..b6e9a5707e 100644
--- a/src/database/sql/fakedb_test.go
+++ b/src/database/sql/fakedb_test.go
@@ -396,9 +396,9 @@ func (c *fakeConn) ResetSession(ctx context.Context) error {
 	return nil
 }
 
-var _ driver.ConnectionValidator = (*fakeConn)(nil)
+var _ driver.Validator = (*fakeConn)(nil)
 
-func (c *fakeConn) ValidConnection() bool {
+func (c *fakeConn) IsValid() bool {
 	return !c.isBad()
 }
 
diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index 95906b1318..4093ffe1bb 100644
--- a/src/database/sql/sql.go
+++ b/src/database/sql/sql.go
@@ -512,8 +512,8 @@ func (dc *driverConn) validateConnection(needsReset bool) bool {
 	if needsReset {
 		dc.needReset = true
 	}
-	if cv, ok := dc.ci.(driver.ConnectionValidator); ok {
-		return cv.ValidConnection()
+	if cv, ok := dc.ci.(driver.Validator); ok {
+		return cv.IsValid()
 	}
 	return true
 }
diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go
index 0fc994d0a1..f08eba93b3 100644
--- a/src/database/sql/sql_test.go
+++ b/src/database/sql/sql_test.go
@@ -1543,6 +1543,37 @@ func TestConnTx(t *testing.T) {
 	}
 }
 
+// TestConnIsValid verifies that a database connection that should be discarded,
+// is actually discarded and does not re-enter the connection pool.
+// If the IsValid method from *fakeConn is removed, this test will fail.
+func TestConnIsValid(t *testing.T) {
+	db := newTestDB(t, "people")
+	defer closeDB(t, db)
+
+	db.SetMaxOpenConns(1)
+
+	ctx := context.Background()
+
+	c, err := db.Conn(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = c.Raw(func(raw interface{}) error {
+		dc := raw.(*fakeConn)
+		dc.stickyBad = true
+		return nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	c.Close()
+
+	if len(db.freeConn) > 0 && db.freeConn[0].ci.(*fakeConn).stickyBad {
+		t.Fatal("bad connection returned to pool; expected bad connection to be discarded")
+	}
+}
+
 // Tests fix for issue 2542, that we release a lock when querying on
 // a closed connection.
 func TestIssue2542Deadlock(t *testing.T) {