Inverting deps on SQL, Log and MQ plugins to make them optional dependencies of extended servers, Removing some dead code that brought in unused dependencies Filtering out some non-linux transitive deps. (#1057)

* initial Db helper split - make SQL and datastore packages optional

* abstracting log store

* break out DB, MQ and log drivers as extensions

* cleanup

* fewer deps

* fixing docker test

* hmm dbness

* updating db startup

* Consolidate all your extensions into one convenient package

* cleanup

* clean up dep constraints

api/datastore/datastore.go

@@ -2,35 +2,49 @@ package datastore
 
 import (
 	"context"
-	"fmt"
 	"net/url"
 
+	"fmt"
 	"github.com/fnproject/fn/api/common"
 	"github.com/fnproject/fn/api/datastore/internal/datastoreutil"
-	"github.com/fnproject/fn/api/datastore/sql"
 	"github.com/fnproject/fn/api/models"
 	"github.com/sirupsen/logrus"
 )
 
+// New creates a DataStore from the specified URL
 func New(ctx context.Context, dbURL string) (models.Datastore, error) {
-	return newds(ctx, dbURL) // teehee
-}
-
-func Wrap(ds models.Datastore) models.Datastore {
-	return datastoreutil.MetricDS(datastoreutil.NewValidator(ds))
-}
-
-func newds(ctx context.Context, dbURL string) (models.Datastore, error) {
 	log := common.Logger(ctx)
 	u, err := url.Parse(dbURL)
 	if err != nil {
 		log.WithError(err).WithFields(logrus.Fields{"url": dbURL}).Fatal("bad DB URL")
 	}
 	log.WithFields(logrus.Fields{"db": u.Scheme}).Debug("creating new datastore")
-	switch u.Scheme {
-	case "sqlite3", "postgres", "mysql":
-		return sql.New(ctx, u)
-	default:
-		return nil, fmt.Errorf("db type not supported %v", u.Scheme)
+
+	for _, provider := range providers {
+		if provider.Supports(u) {
+			return provider.New(ctx, u)
+		}
 	}
+	return nil, fmt.Errorf("no data store provider found for storage url %s", u)
+}
+
+func Wrap(ds models.Datastore) models.Datastore {
+	return datastoreutil.MetricDS(datastoreutil.NewValidator(ds))
+}
+
+// Provider is a datastore provider
+type Provider interface {
+	fmt.Stringer
+	// Supports indicates if this provider can handle a given data store.
+	Supports(url *url.URL) bool
+	// New creates a new data store from the specified URL
+	New(ctx context.Context, url *url.URL) (models.Datastore, error)
+}
+
+var providers []Provider
+
+// AddProvider globally registers a data store provider
+func AddProvider(provider Provider) {
+	logrus.Infof("Adding DataStore provider %s", provider)
+	providers = append(providers, provider)
 }

api/datastore/sql/dbhelper/dbhelper.go

@@ -0,0 +1,43 @@
+// dbhelpers wrap SQL and specific capabilities of an SQL db
+package dbhelper
+
+import (
+	"fmt"
+	"github.com/jmoiron/sqlx"
+	"github.com/sirupsen/logrus"
+	"net/url"
+)
+
+var sqlHelpers []Helper
+
+//Add registers a new SQL helper
+func Add(helper Helper) {
+	logrus.Infof("Registering DB helper %s", helper)
+	sqlHelpers = append(sqlHelpers, helper)
+}
+
+//Helper provides DB-specific SQL capabilities
+type Helper interface {
+	fmt.Stringer
+	//Supports indicates if this helper supports this driver name
+	Supports(driverName string) bool
+	//PreConnect calculates the connect URL for the db from a canonical URL used in Fn config
+	PreConnect(url *url.URL) (string, error)
+	//PostCreate  Apply any configuration to the DB prior to use
+	PostCreate(db *sqlx.DB) (*sqlx.DB, error)
+	//CheckTableExists checks if a table exists in the DB
+	CheckTableExists(tx *sqlx.Tx, table string) (bool, error)
+	//IsDuplicateKeyError determines if an error indicates if the prior error was caused by a duplicate key insert
+	IsDuplicateKeyError(err error) bool
+}
+
+//GetHelper returns a helper for a specific driver
+func GetHelper(driverName string) (Helper, bool) {
+	for _, helper := range sqlHelpers {
+		if helper.Supports(driverName) {
+			return helper, true
+		}
+		logrus.Printf("%s does not support %s", helper, driverName)
+	}
+	return nil, false
+}

api/datastore/sql/migratex/migrate.go

@@ -9,9 +9,8 @@ import (
 	"sort"
 	"strings"
 
-	"github.com/go-sql-driver/mysql"
+	"github.com/fnproject/fn/api/datastore/sql/dbhelper"
 	"github.com/jmoiron/sqlx"
-	"github.com/lib/pq"
 	"github.com/sirupsen/logrus"
 )
 
@@ -302,28 +301,29 @@ func SetVersion(ctx context.Context, tx *sqlx.Tx, version int64, dirty bool) err
 }
 
 func Version(ctx context.Context, tx *sqlx.Tx) (version int64, dirty bool, err error) {
+	helper, ok := dbhelper.GetHelper(tx.DriverName())
+	if !ok {
+		return 0, false, fmt.Errorf("no db helper registered for for %s", tx.DriverName())
+	}
+
+	tableExists, err := helper.CheckTableExists(tx, MigrationsTable)
+
+	if err != nil {
+		return 0, false, err
+	}
+
+	if !tableExists {
+		return NilVersion, false, nil
+	}
+
 	query := tx.Rebind(`SELECT version, dirty FROM ` + MigrationsTable + ` LIMIT 1`)
+
 	err = tx.QueryRowContext(ctx, query).Scan(&version, &dirty)
 	switch {
 	case err == sql.ErrNoRows:
 		return NilVersion, false, nil
 
 	case err != nil:
-		if e, ok := err.(*mysql.MySQLError); ok {
-			if e.Number == 0 {
-				return NilVersion, false, nil
-			}
-		}
-		if e, ok := err.(*pq.Error); ok {
-			if e.Code.Name() == "undefined_table" {
-				return NilVersion, false, nil
-			}
-		}
-		// sqlite3 returns 'no such table' but the error is not typed
-		if strings.Contains(err.Error(), "no such table") {
-			return NilVersion, false, nil
-		}
-
 		return 0, false, err
 
 	default:

api/datastore/sql/migratex/migrate_test.go

@@ -6,8 +6,8 @@ import (
 	"fmt"
 	"testing"
 
+	_ "github.com/fnproject/fn/api/datastore/sql/sqlite"
 	"github.com/jmoiron/sqlx"
-	_ "github.com/mattn/go-sqlite3"
 )
 
 const testsqlite3 = "file::memory:?mode=memory&cache=shared"

api/datastore/sql/mysql/mysql.go

@@ -0,0 +1,61 @@
+package mysql
+
+import (
+	"fmt"
+	"github.com/fnproject/fn/api/datastore/sql/dbhelper"
+	"github.com/go-sql-driver/mysql"
+	_ "github.com/go-sql-driver/mysql"
+	"github.com/jmoiron/sqlx"
+	"net/url"
+	"strings"
+)
+
+type mysqlHelper int
+
+func (mysqlHelper) Supports(scheme string) bool {
+	return scheme == "mysql"
+}
+
+func (mysqlHelper) PreConnect(url *url.URL) (string, error) {
+	return strings.TrimPrefix(url.String(), url.Scheme+"://"), nil
+}
+
+func (mysqlHelper) PostCreate(db *sqlx.DB) (*sqlx.DB, error) {
+	return db, nil
+
+}
+func (mysqlHelper) CheckTableExists(tx *sqlx.Tx, table string) (bool, error) {
+	query := tx.Rebind(fmt.Sprintf(`SELECT count(*)
+	FROM information_schema.TABLES
+	WHERE TABLE_NAME = '%s'
+`, table))
+
+	row := tx.QueryRow(query)
+
+	var count int
+	err := row.Scan(&count)
+	if err != nil {
+		return false, err
+	}
+
+	exists := count > 0
+	return exists, nil
+}
+
+func (mysqlHelper) String() string {
+	return "mysql"
+}
+
+func (mysqlHelper) IsDuplicateKeyError(err error) bool {
+	switch mErr := err.(type) {
+	case *mysql.MySQLError:
+		if mErr.Number == 1062 {
+			return true
+		}
+	}
+	return false
+}
+
+func init() {
+	dbhelper.Add(mysqlHelper(0))
+}

api/datastore/sql/postgres/postgres.go

@@ -0,0 +1,63 @@
+package postgres
+
+import (
+	"github.com/fnproject/fn/api/datastore/sql/dbhelper"
+	_ "github.com/go-sql-driver/mysql"
+	"github.com/jmoiron/sqlx"
+	"github.com/lib/pq"
+	"net/url"
+)
+
+type postgresHelper int
+
+func (postgresHelper) Supports(scheme string) bool {
+	switch scheme {
+	case "postgres", "pgx":
+		return true
+	}
+	return false
+}
+
+func (postgresHelper) PreConnect(url *url.URL) (string, error) {
+	return url.String(), nil
+}
+
+func (postgresHelper) PostCreate(db *sqlx.DB) (*sqlx.DB, error) {
+	return db, nil
+
+}
+func (postgresHelper) CheckTableExists(tx *sqlx.Tx, table string) (bool, error) {
+	query := tx.Rebind(`SELECT count(*)
+	FROM information_schema.TABLES
+	WHERE TABLE_NAME = 'apps'
+`)
+
+	row := tx.QueryRow(query)
+
+	var count int
+	err := row.Scan(&count)
+	if err != nil {
+		return false, err
+	}
+
+	exists := count > 0
+	return exists, nil
+}
+
+func (postgresHelper) String() string {
+	return "postgres"
+}
+
+func (postgresHelper) IsDuplicateKeyError(err error) bool {
+	switch dbErr := err.(type) {
+	case *pq.Error:
+		if dbErr.Code == "23505" {
+			return true
+		}
+	}
+	return false
+}
+
+func init() {
+	dbhelper.Add(postgresHelper(0))
+}

api/datastore/sql/sql.go

@@ -4,12 +4,10 @@ import (
 	"bytes"
 	"context"
 	"database/sql"
-	"errors"
 	"fmt"
 	"io"
 	"net/url"
 	"os"
-	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -18,13 +16,11 @@ import (
 	"github.com/fnproject/fn/api/datastore/sql/migratex"
 	"github.com/fnproject/fn/api/datastore/sql/migrations"
 	"github.com/fnproject/fn/api/models"
-	"github.com/go-sql-driver/mysql"
-	_ "github.com/go-sql-driver/mysql"
 	"github.com/jmoiron/sqlx"
-	"github.com/lib/pq"
-	_ "github.com/lib/pq"
-	"github.com/mattn/go-sqlite3"
-	_ "github.com/mattn/go-sqlite3"
+
+	"github.com/fnproject/fn/api/datastore"
+	"github.com/fnproject/fn/api/datastore/sql/dbhelper"
+	"github.com/fnproject/fn/api/logs"
 	"github.com/sirupsen/logrus"
 )
 
@@ -103,13 +99,44 @@ var ( // compiler will yell nice things about our upbringing as a child
 )
 
 type SQLStore struct {
-	db *sqlx.DB
+	helper dbhelper.Helper
+	db     *sqlx.DB
 }
 
+type sqlDsProvider int
+
 // New will open the db specified by url, create any tables necessary
 // and return a models.Datastore safe for concurrent usage.
-func New(ctx context.Context, url *url.URL) (*SQLStore, error) {
-	return newDS(ctx, url)
+func New(ctx context.Context, u *url.URL) (*SQLStore, error) {
+	return newDS(ctx, u)
+}
+
+func (sqlDsProvider) Supports(u *url.URL) bool {
+	_, ok := dbhelper.GetHelper(u.Scheme)
+	return ok
+}
+
+func (sqlDsProvider) New(ctx context.Context, u *url.URL) (models.Datastore, error) {
+	return newDS(ctx, u)
+}
+
+func (sqlDsProvider) String() string {
+	return "sql"
+}
+
+type sqlLogsProvider int
+
+func (sqlLogsProvider) String() string {
+	return "sql"
+}
+
+func (sqlLogsProvider) Supports(u *url.URL) bool {
+	_, ok := dbhelper.GetHelper(u.Scheme)
+	return ok
+}
+
+func (sqlLogsProvider) New(ctx context.Context, u *url.URL) (models.LogStore, error) {
+	return newDS(ctx, u)
 }
 
 // for test methods, return concrete type, but don't expose
@@ -117,27 +144,19 @@ func newDS(ctx context.Context, url *url.URL) (*SQLStore, error) {
 	driver := url.Scheme
 
 	log := common.Logger(ctx)
-	// driver must be one of these for sqlx to work, double check:
-	switch driver {
-	case "postgres", "pgx", "mysql", "sqlite3":
-	default:
-		return nil, errors.New("invalid db driver, refer to the code")
+	helper, ok := dbhelper.GetHelper(driver)
+
+	if !ok {
+		return nil, fmt.Errorf("DB helper '%s' is not supported", driver)
 	}
 
-	if driver == "sqlite3" {
-		// make all the dirs so we can make the file..
-		dir := filepath.Dir(url.Path)
-		err := os.MkdirAll(dir, 0755)
-		if err != nil {
-			return nil, err
-		}
+	uri, err := helper.PreConnect(url)
+
+	if err != nil {
+		return nil, fmt.Errorf("failed to initialise db helper %s : %s", driver, err)
 	}
 
-	uri := url.String()
-	if driver != "postgres" {
-		// postgres seems to need this as a prefix in lib/pq, everyone else wants it stripped of scheme
-		uri = strings.TrimPrefix(url.String(), url.Scheme+"://")
-	}
+	log.WithFields(logrus.Fields{"url": uri}).Info("Connecting to DB")
 
 	sqldb, err := sql.Open(driver, uri)
 	if err != nil {
@@ -158,12 +177,12 @@ func newDS(ctx context.Context, url *url.URL) (*SQLStore, error) {
 	db.SetMaxIdleConns(maxIdleConns)
 	log.WithFields(logrus.Fields{"max_idle_connections": maxIdleConns, "datastore": driver}).Info("datastore dialed")
 
-	switch driver { // NOTE: fixes weird sqlite3 behavior
-	case "sqlite3":
-		db.SetMaxOpenConns(1)
+	db, err = helper.PostCreate(db)
+	if err != nil {
+		log.WithFields(logrus.Fields{"url": uri}).WithError(err).Error("couldn't initialize db")
+		return nil, err
 	}
-
-	sdb := &SQLStore{db: db}
+	sdb := &SQLStore{db: db, helper: helper}
 
 	// NOTE: runMigrations happens before we create all the tables, so that it
 	// can detect whether the db did not exist and insert the latest version of
@@ -231,41 +250,11 @@ func pingWithRetry(ctx context.Context, db *sqlx.DB) (err error) {
 	return err
 }
 
-// checkExistence checks if tables have been created yet, it is not concerned
-// about the existence of the schema migration version (since migrations were
-// added to existing dbs, we need to know whether the db exists without migrations
-// or if it's brand new).
-func checkExistence(tx *sqlx.Tx) (bool, error) {
-	query := tx.Rebind(`SELECT count(*)
-	FROM information_schema.TABLES
-	WHERE TABLE_NAME = 'apps'
-`)
-
-	if tx.DriverName() == "sqlite3" {
-		// sqlite3 is special, of course
-		query = tx.Rebind(`SELECT count(*)
-		FROM sqlite_master
-		WHERE name = 'apps'
-		`)
-	}
-
-	row := tx.QueryRow(query)
-
-	var count int
-	err := row.Scan(&count)
-	if err != nil {
-		return false, err
-	}
-
-	exists := count > 0
-	return exists, nil
-}
-
 // check if the db already existed, if the db is brand new then we can skip
 // over all the migrations BUT we must be sure to set the right migration
 // number so that only current migrations are skipped, not any future ones.
 func (ds *SQLStore) runMigrations(ctx context.Context, tx *sqlx.Tx, migrations []migratex.Migration) error {
-	dbExists, err := checkExistence(tx)
+	dbExists, err := ds.helper.CheckTableExists(tx, "apps")
 	if err != nil {
 		return err
 	}
@@ -355,19 +344,8 @@ func (ds *SQLStore) InsertApp(ctx context.Context, app *models.App) (*models.App
 	);`)
 	_, err := ds.db.NamedExecContext(ctx, query, app)
 	if err != nil {
-		switch err := err.(type) {
-		case *mysql.MySQLError:
-			if err.Number == 1062 {
-				return nil, models.ErrAppsAlreadyExists
-			}
-		case *pq.Error:
-			if err.Code == "23505" {
-				return nil, models.ErrAppsAlreadyExists
-			}
-		case sqlite3.Error:
-			if err.ExtendedCode == sqlite3.ErrConstraintUnique || err.ExtendedCode == sqlite3.ErrConstraintPrimaryKey {
-				return nil, models.ErrAppsAlreadyExists
-			}
+		if ds.helper.IsDuplicateKeyError(err) {
+			return nil, models.ErrAppsAlreadyExists
 		}
 		return nil, err
 	}
@@ -924,3 +902,8 @@ func (ds *SQLStore) GetDatabase() *sqlx.DB {
 func (ds *SQLStore) Close() error {
 	return ds.db.Close()
 }
+
+func init() {
+	datastore.AddProvider(sqlDsProvider(0))
+	logs.AddProvider(sqlLogsProvider(0))
+}

api/datastore/sql/sql_test.go

@@ -10,6 +10,9 @@ import (
 	"github.com/fnproject/fn/api/datastore/internal/datastoreutil"
 	"github.com/fnproject/fn/api/datastore/sql/migratex"
 	"github.com/fnproject/fn/api/datastore/sql/migrations"
+	_ "github.com/fnproject/fn/api/datastore/sql/mysql"
+	_ "github.com/fnproject/fn/api/datastore/sql/postgres"
+	_ "github.com/fnproject/fn/api/datastore/sql/sqlite"
 	logstoretest "github.com/fnproject/fn/api/logs/testing"
 	"github.com/fnproject/fn/api/models"
 	"github.com/jmoiron/sqlx"

api/datastore/sql/sqlite/sqlite.go

@@ -0,0 +1,74 @@
+package sqlite
+
+import (
+	"fmt"
+	"github.com/fnproject/fn/api/datastore/sql/dbhelper"
+	"github.com/jmoiron/sqlx"
+	"github.com/mattn/go-sqlite3"
+	"net/url"
+	"os"
+	"path/filepath"
+	"strings"
+)
+
+type sqliteHelper int
+
+func (sqliteHelper) Supports(scheme string) bool {
+	switch scheme {
+	case "sqlite3", "sqlite":
+		return true
+	}
+	return false
+}
+
+func (sqliteHelper) PreConnect(url *url.URL) (string, error) {
+	// make all the dirs so we can make the file..
+	dir := filepath.Dir(url.Path)
+	err := os.MkdirAll(dir, 0755)
+	if err != nil {
+		return "", err
+	}
+
+	return strings.TrimPrefix(url.String(), url.Scheme+"://"), nil
+}
+
+func (sqliteHelper) PostCreate(db *sqlx.DB) (*sqlx.DB, error) {
+	db.SetMaxOpenConns(1)
+	return db, nil
+}
+
+func (sqliteHelper) CheckTableExists(tx *sqlx.Tx, table string) (bool, error) {
+	query := tx.Rebind(fmt.Sprintf(`SELECT count(*)
+		FROM sqlite_master
+		WHERE name = '%s'
+		`, table))
+
+	row := tx.QueryRow(query)
+
+	var count int
+	err := row.Scan(&count)
+	if err != nil {
+		return false, err
+	}
+
+	exists := count > 0
+	return exists, nil
+}
+
+func (sqliteHelper) String() string {
+	return "sqlite"
+}
+
+func (sqliteHelper) IsDuplicateKeyError(err error) bool {
+	sqliteErr, ok := err.(sqlite3.Error)
+	if ok {
+		if sqliteErr.ExtendedCode == sqlite3.ErrConstraintUnique || sqliteErr.ExtendedCode == sqlite3.ErrConstraintPrimaryKey {
+			return true
+		}
+	}
+	return false
+}
+
+func init() {
+	dbhelper.Add(sqliteHelper(0))
+}