Change collation lookup to be durable across additional engine/versions (petoju#158)

* Change collation lookup to be durable across additional engine/versions

This patch looks up collation based on INFORMATION_SCHEMA.COLLATIONS
and scopes the response to just the two required columns.

With this patch it's:
1. Mysql 5.7 and 8.0 without special casing
2. MariaDB without special casing
3. compatible with TiDB for 6.x and 7.x (v7.5.1 which advertises as 8.0.11-TiDB-v7.5.1
   but lacks the 7th column in SHOW statement previously used)

* Enable tidb testing for resource_database

* Fix tests for TiDB and add TiDB 7.5

* Update README about local test running

* Enable support for TiDB 7.5.2 and disable selective tests

* Update mysql/provider.go

* Remove debugging code

* Refactor to simplify testAccs that overlap

* Add back in test skips where it fails for TiDB 7.5.2

These are skipped because of syntactical differences between mysql
and TiDB. These tend to have mariadb/rds/tidb matching more closely
in behavior.

Author: zph

.circleci/config.yml

@@ -38,5 +38,5 @@ workflows:
       - integration:
           matrix:
             parameters:
-              target: ["testversion5.6", "testversion5.7", "testversion8.0", "testpercona5.7", "testpercona8.0", "testmariadb10.3", "testmariadb10.8", "testmariadb10.10", "testtidb6.1.0"]
+              target: ["testversion5.6", "testversion5.7", "testversion8.0", "testpercona5.7", "testpercona8.0", "testmariadb10.3", "testmariadb10.8", "testmariadb10.10", "testtidb6.1.0", "testtidb7.5.2"]
       - govet

GNUmakefile

@@ -22,7 +22,7 @@ bin/terraform:
 testacc: fmtcheck bin/terraform
 	PATH="$(CURDIR)/bin:${PATH}" TF_ACC=1 go test $(TEST) -v $(TESTARGS) -timeout=90s
 
-acceptance: testversion5.6 testversion5.7 testversion8.0 testpercona5.7 testpercona8.0 testmariadb10.3 testmariadb10.8 testmariadb10.10 testtidb6.1.0
+acceptance: testversion5.6 testversion5.7 testversion8.0 testpercona5.7 testpercona8.0 testmariadb10.3 testmariadb10.8 testmariadb10.10 testtidb6.1.0 testtidb7.5.2
 
 testversion%:
 	$(MAKE) MYSQL_VERSION=$* MYSQL_PORT=33$(shell echo "$*" | tr -d '.') testversion
@@ -57,6 +57,8 @@ testrdsdb:
 testtidb%:
 	$(MAKE) MYSQL_VERSION=$* MYSQL_PORT=34$(shell echo "$*" | tr -d '.') testtidb
 
+# WARNING: this does not work as a bare task run, it only instantiates correctly inside the versioned TiDB task run
+#          otherwise MYSQL_PORT and version are unset.
 testtidb:
 	@sh -c "'$(CURDIR)/scripts/tidb-test-cluster.sh' --init --port $(MYSQL_PORT) --version $(MYSQL_VERSION)"
 	@echo 'Waiting for TiDB...'

README.md

@@ -65,6 +65,13 @@ $ make bin
 $ $GOPATH/bin/terraform-provider-mysql
 ...
 ```
+### Ensure local requirements are present:
+
+1. Docker environment
+2. mysql-client binary which can be installed on Mac with `brew install [email protected]`
+   1. Then add it to your path OR `brew link [email protected]`
+
+### Running tests
 
 In order to test the provider, you can simply run `make test`.
 

mysql/provider.go

@@ -542,6 +542,25 @@ func serverVersionString(db *sql.DB) (string, error) {
 	return versionString, nil
 }
 
+// serverTiDB returns:
+// - it is a TiDB instance
+// - tidbVersion
+// - mysqlCompatibilityVersion
+// - err
+func serverTiDB(db *sql.DB) (bool, string, string, error) {
+	currentVersionString, err := serverVersionString(db)
+	if err != nil {
+		return false, "", "", err
+	}
+
+	if strings.Contains(currentVersionString, "TiDB") {
+		versions := strings.SplitN(currentVersionString, "-", 3)
+		return true, versions[2], versions[0], nil
+	}
+
+	return false, "", "", nil
+}
+
 func serverRds(db *sql.DB) (bool, error) {
 	var metadataVersionString string
 	err := db.QueryRow("SELECT @@GLOBAL.datadir").Scan(&metadataVersionString)

mysql/provider_test.go

@@ -3,11 +3,12 @@ package mysql
 import (
 	"context"
 	"fmt"
-	"github.com/hashicorp/go-version"
 	"os"
 	"strings"
 	"testing"
 
+	"github.com/hashicorp/go-version"
+
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
 )
@@ -153,25 +154,7 @@ func testAccPreCheckSkipMariaDB(t *testing.T) {
 }
 
 func testAccPreCheckSkipNotMySQL8(t *testing.T) {
-	testAccPreCheck(t)
-
-	ctx := context.Background()
-	db, err := connectToMySQL(ctx, testAccProvider.Meta().(*MySQLConfiguration))
-	if err != nil {
-		t.Fatalf("Cannot connect to DB (SkipNotMySQL8): %v", err)
-		return
-	}
-
-	currentVersion, err := serverVersion(db)
-	if err != nil {
-		t.Fatalf("Cannot get DB version string (SkipNotMySQL8): %v", err)
-		return
-	}
-
-	versionMin, _ := version.NewVersion("8.0.0")
-	if currentVersion.LessThan(versionMin) {
-		t.Skip("Skip on MySQL8")
-	}
+	testAccPreCheckSkipNotMySQLVersionMin(t, "8.0.0")
 }
 
 func testAccPreCheckSkipNotMySQLVersionMin(t *testing.T, minVersion string) {
@@ -180,19 +163,36 @@ func testAccPreCheckSkipNotMySQLVersionMin(t *testing.T, minVersion string) {
 	ctx := context.Background()
 	db, err := connectToMySQL(ctx, testAccProvider.Meta().(*MySQLConfiguration))
 	if err != nil {
-		t.Fatalf("Cannot connect to DB (SkipNotMySQLVersionMin): %v", err)
+		t.Fatalf("Cannot connect to DB (SkipNotMySQL8): %v", err)
 		return
 	}
 
 	currentVersion, err := serverVersion(db)
 	if err != nil {
-		t.Fatalf("Cannot get DB version string (SkipNotMySQLVersionMin): %v", err)
+		t.Fatalf("Cannot get DB version string (SkipNotMySQL8): %v", err)
 		return
 	}
 
 	versionMin, _ := version.NewVersion(minVersion)
 	if currentVersion.LessThan(versionMin) {
-		t.Skipf("Skip on MySQL version less than %s", minVersion)
+		// TiDB 7.x series advertises as 8.0 mysql so we batch its testing strategy with Mysql8
+		isTiDB, tidbVersion, mysqlCompatibilityVersion, err := serverTiDB(db)
+		if err != nil {
+			t.Fatalf("Cannot get DB version string (SkipNotMySQL8): %v", err)
+			return
+		}
+		if isTiDB {
+			mysqlVersion, err := version.NewVersion(mysqlCompatibilityVersion)
+			if err != nil {
+				t.Fatalf("Cannot get DB version string for TiDB (SkipNotMySQL8): %s %s %v", tidbVersion, mysqlCompatibilityVersion, err)
+				return
+			}
+			if mysqlVersion.LessThan(versionMin) {
+				t.Skip("Skip on MySQL8")
+			}
+		}
+
+		t.Skip("Skip on MySQL8")
 	}
 }
 

mysql/resource_database.go

@@ -5,11 +5,11 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
-	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"log"
 	"strings"
 
-	"github.com/hashicorp/go-version"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
+
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
 
@@ -116,23 +116,22 @@ func ReadDatabase(ctx context.Context, d *schema.ResourceData, meta interface{})
 		// MySQL doesn't return the collation if it's the default one for
 		// the charset, so if we don't have a collation we need to go
 		// hunt for the default.
-		stmtSQL := "SHOW COLLATION WHERE `Charset` = ? AND `Default` = 'Yes'"
-		var empty interface{}
+		stmtSQL := "SELECT COLLATION_NAME, CHARACTER_SET_NAME FROM INFORMATION_SCHEMA.COLLATIONS WHERE CHARACTER_SET_NAME = ? AND `IS_DEFAULT` = 'Yes';"
+		/*
+			Mysql (5.7, 8.0), TiDB (6.x, 7.x) example:
+			> SELECT COLLATION_NAME, CHARACTER_SET_NAME FROM INFORMATION_SCHEMA.COLLATIONS WHERE CHARACTER_SET_NAME = 'utf8mb4' AND `IS_DEFAULT` = 'Yes';
 
-		requiredVersion, _ := version.NewVersion("8.0.0")
+					+--------------------+--------------------+
+					| COLLATION_NAME     | CHARACTER_SET_NAME |
+					+--------------------+--------------------+
+					| utf8mb4_0900_ai_ci | utf8mb4            |
+					+--------------------+--------------------+
 
-		serverVersionString, err := serverVersionString(db)
-		if err != nil {
-			return diag.Errorf("could not get error version string: %v", err)
-		}
 
-		// MySQL 8 returns more data in a row.
-		var res error
-		if !strings.Contains(serverVersionString, "MariaDB") && getVersionFromMeta(ctx, meta).GreaterThan(requiredVersion) {
-			res = db.QueryRow(stmtSQL, defaultCharset).Scan(&defaultCollation, &empty, &empty, &empty, &empty, &empty, &empty)
-		} else {
-			res = db.QueryRow(stmtSQL, defaultCharset).Scan(&defaultCollation, &empty, &empty, &empty, &empty, &empty)
-		}
+		*/
+		var empty interface{}
+
+		res := db.QueryRow(stmtSQL, defaultCharset).Scan(&defaultCollation, &empty)
 
 		if res != nil {
 			if errors.Is(res, sql.ErrNoRows) {

mysql/resource_database_test.go

@@ -13,7 +13,7 @@ import (
 func TestAccDatabase(t *testing.T) {
 	dbName := "terraform_acceptance_test"
 	resource.Test(t, resource.TestCase{
-		PreCheck:          func() { testAccPreCheckSkipTiDB(t) },
+		PreCheck:          func() {},
 		ProviderFactories: testAccProviderFactories,
 		CheckDestroy:      testAccDatabaseCheckDestroy(dbName),
 		Steps: []resource.TestStep{
@@ -46,7 +46,7 @@ func TestAccDatabase_collationChange(t *testing.T) {
 	ctx := context.Background()
 
 	resource.Test(t, resource.TestCase{
-		PreCheck:          func() { testAccPreCheckSkipTiDB(t) },
+		PreCheck:          func() {},
 		ProviderFactories: testAccProviderFactories,
 		CheckDestroy:      testAccDatabaseCheckDestroy(dbName),
 		Steps: []resource.TestStep{
@@ -106,11 +106,21 @@ func testAccDatabaseCheckFull(rn string, name string, charset string, collation
 			return fmt.Errorf("error reading database: %s", err)
 		}
 
-		if strings.Index(createSQL, fmt.Sprintf("CHARACTER SET %s", charset)) == -1 {
+		if !strings.Contains(createSQL, fmt.Sprintf("CHARACTER SET %s", charset)) {
 			return fmt.Errorf("database default charset isn't %s", charset)
 		}
-		if strings.Index(createSQL, fmt.Sprintf("COLLATE %s", collation)) == -1 {
-			return fmt.Errorf("database default collation isn't %s", collation)
+		// TiDB does not include the COLLATE reference in `SHOW CREATE DATABASE`
+		// so perform a lookup based on the charset to find default collation
+		if !strings.Contains(createSQL, fmt.Sprintf("COLLATE %s", collation)) {
+			sql := `SELECT COLLATION_NAME FROM INFORMATION_SCHEMA.COLLATIONS WHERE IS_DEFAULT = 'Yes' AND CHARACTER_SET_NAME = ?;`
+			var fetchedCollation string
+			err = db.QueryRow(sql, charset).Scan(&fetchedCollation)
+			if err != nil {
+				return fmt.Errorf("database default collation expected %s vs actual %s with error: %e", collation, fetchedCollation, err)
+			}
+			if fetchedCollation != collation {
+				return fmt.Errorf("database default collation expected %s vs actual %s", collation, fetchedCollation)
+			}
 		}
 
 		return nil

mysql/resource_default_roles_test.go

@@ -16,6 +16,7 @@ func TestAccDefaultRoles_basic(t *testing.T) {
 			testAccPreCheck(t)
 			testAccPreCheckSkipNotMySQL8(t)
 			testAccPreCheckSkipMariaDB(t)
+			testAccPreCheckSkipTiDB(t)
 		},
 		ProviderFactories: testAccProviderFactories,
 		CheckDestroy:      testAccDefaultRolesCheckDestroy,

mysql/resource_grant_test.go

@@ -297,10 +297,10 @@ func TestAccGrantComplexMySQL8(t *testing.T) {
 	dbName := fmt.Sprintf("tf-test-%d", rand.Intn(100))
 	resource.Test(t, resource.TestCase{
 		PreCheck: func() {
-			testAccPreCheckSkipTiDB(t)
 			testAccPreCheckSkipRds(t)
 			testAccPreCheckSkipMariaDB(t)
 			testAccPreCheckSkipNotMySQLVersionMin(t, "8.0.0")
+			testAccPreCheckSkipTiDB(t)
 		},
 		ProviderFactories: testAccProviderFactories,
 		CheckDestroy:      testAccGrantCheckDestroy,
@@ -369,6 +369,7 @@ func TestAccGrant_roleToUser(t *testing.T) {
 			testAccPreCheck(t)
 			testAccPreCheckSkipRds(t)
 			testAccPreCheckSkipNotMySQLVersionMin(t, "8.0.0")
+			testAccPreCheckSkipTiDB(t)
 		},
 		ProviderFactories: testAccProviderFactories,
 		CheckDestroy:      testAccGrantCheckDestroy,
@@ -392,6 +393,7 @@ func TestAccGrant_complexRoleGrants(t *testing.T) {
 			testAccPreCheck(t)
 			testAccPreCheckSkipMariaDB(t)
 			testAccPreCheckSkipNotMySQLVersionMin(t, "8.0.0")
+			testAccPreCheckSkipTiDB(t)
 		},
 		ProviderFactories: testAccProviderFactories,
 		CheckDestroy:      testAccGrantCheckDestroy,
@@ -1075,20 +1077,20 @@ func TestAllowDuplicateUsersDifferentTables(t *testing.T) {
 	resource "mysql_database" "test" {
 	  name = "%s"
 	}
-	
+
 	resource "mysql_user" "test" {
 	  user     = "jdoe-%s"
 	  host     = "example.com"
 	}
-	
+
 	resource "mysql_grant" "grant1" {
 	  user       = "${mysql_user.test.user}"
 	  host       = "${mysql_user.test.host}"
 	  database   = "${mysql_database.test.name}"
       table      = "table1"
 	  privileges = ["UPDATE", "SELECT"]
 	}
-	
+
 	resource "mysql_grant" "grant2" {
 	  user       = "${mysql_user.test.user}"
 	  host       = "${mysql_user.test.host}"
@@ -1140,20 +1142,20 @@ func TestDisallowDuplicateUsersSameTable(t *testing.T) {
 	resource "mysql_database" "test" {
 	  name = "%s"
 	}
-	
+
 	resource "mysql_user" "test" {
 	  user     = "jdoe-%s"
 	  host     = "example.com"
 	}
-	
+
 	resource "mysql_grant" "grant1" {
 	  user       = "${mysql_user.test.user}"
 	  host       = "${mysql_user.test.host}"
 	  database   = "${mysql_database.test.name}"
       table      = "table1"
 	  privileges = ["UPDATE", "SELECT"]
 	}
-	
+
 	resource "mysql_grant" "grant2" {
 	  user       = "${mysql_user.test.user}"
 	  host       = "${mysql_user.test.host}"

mysql/resource_user_test.go

@@ -133,7 +133,6 @@ func TestAccUser_authConnect(t *testing.T) {
 func TestAccUser_authConnectRetainOldPassword(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck: func() {
-			testAccPreCheckSkipTiDB(t)
 			testAccPreCheckSkipMariaDB(t)
 			testAccPreCheckSkipRds(t)
 			testAccPreCheckSkipNotMySQLVersionMin(t, "8.0.14")