Use an SQL querybuilder

Signed-off-by: Elliot Maincourt <e.maincourt@gmail.com>

go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/DATA-DOG/go-sqlmock v1.4.1
 	github.com/Masterminds/semver/v3 v3.0.3
 	github.com/Masterminds/sprig/v3 v3.0.2
+	github.com/Masterminds/squirrel v1.2.0
 	github.com/Masterminds/vcs v1.13.1
 	github.com/asaskevich/govalidator v0.0.0-20200108200545-475eaeb16496
 	github.com/containerd/containerd v1.3.2

go.sum

@@ -34,6 +34,8 @@ github.com/Masterminds/semver/v3 v3.0.3 h1:znjIyLfpXEDQjOIEWh+ehwpTU14UzUPub3c3s
 github.com/Masterminds/semver/v3 v3.0.3/go.mod h1:VPu/7SZ7ePZ3QOrcuXROw5FAcLl4a0cBrbBpGY/8hQs=
 github.com/Masterminds/sprig/v3 v3.0.2 h1:wz22D0CiSctrliXiI9ZO3HoNApweeRGftyDN+BQa3B8=
 github.com/Masterminds/sprig/v3 v3.0.2/go.mod h1:oesJ8kPONMONaZgtiHNzUShJbksypC5kWczhZAf6+aU=
+github.com/Masterminds/squirrel v1.2.0 h1:K1NhbTO21BWG47IVR0OnIZuE0LZcXAYqywrC3Ko53KI=
+github.com/Masterminds/squirrel v1.2.0/go.mod h1:yaPeOnPG5ZRwL9oKdTsO/prlkPbXWZlRVMQ/gGlzIuA=
 github.com/Masterminds/vcs v1.13.1 h1:NL3G1X7/7xduQtA2sJLpVpfHTNBALVNSjob6KEjPXNQ=
 github.com/Masterminds/vcs v1.13.1/go.mod h1:N09YCmOQr6RLxC6UNHzuVwAdodYbbnycGHSmwVJjcKA=
 github.com/Microsoft/go-winio v0.4.15-0.20190919025122-fc70bd9a86b5 h1:ygIc8M6trr62pF5DucadTWGdEB4mEyvzi0e2nbcmcyA=
@@ -355,6 +357,10 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
 github.com/kr/pty v1.1.5/go.mod h1:9r2w37qlBe7rQ6e1fg1S/9xpWHSnaqNdHD3WcMdbPDA=
 github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
+github.com/lann/builder v0.0.0-20180802200727-47ae307949d0 h1:SOEGU9fKiNWd/HOJuq6+3iTQz8KNCLtVX6idSoTLdUw=
+github.com/lann/builder v0.0.0-20180802200727-47ae307949d0/go.mod h1:dXGbAdH5GtBTC4WfIxhKZfyBF/HBFgRZSWwZ9g/He9o=
+github.com/lann/ps v0.0.0-20150810152359-62de8c46ede0 h1:P6pPBnrTSX3DEVR4fDembhRWSsG5rVo6hYhAB/ADZrk=
+github.com/lann/ps v0.0.0-20150810152359-62de8c46ede0/go.mod h1:vmVJ0l/dxyfGW6FmdpVm2joNMFikkuWg0EoCKLGUMNw=
 github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
 github.com/lib/pq v1.2.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
 github.com/lib/pq v1.3.0 h1:/qkRGz8zljWiDcFvgpwUpwIAPu3r07TDvs3Rws+o/pU=

pkg/storage/driver/mock_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	sqlmock "github.com/DATA-DOG/go-sqlmock"
+	sq "github.com/Masterminds/squirrel"
 	"github.com/jmoiron/sqlx"
 
 	v1 "k8s.io/api/core/v1"
@@ -259,5 +260,6 @@ func newTestFixtureSQL(t *testing.T, releases ...*rspb.Release) (*SQL, sqlmock.S
 		db:               sqlxDB,
 		Log:              func(a string, b ...interface{}) {},
 		namespace:        "default",
+		statementBuilder: sq.StatementBuilder.PlaceholderFormat(sq.Dollar),
 	}, mock
 }

pkg/storage/driver/sql.go

@@ -19,12 +19,13 @@ package driver // import "helm.sh/helm/v3/pkg/storage/driver"
 import (
 	"fmt"
 	"sort"
-	"strings"
 	"time"
 
 	"github.com/jmoiron/sqlx"
 	migrate "github.com/rubenv/sql-migrate"
 
+	sq "github.com/Masterminds/squirrel"
+
 	// Import pq for postgres dialect
 	_ "github.com/lib/pq"
 
@@ -73,6 +74,7 @@ const (
 type SQL struct {
 	db               *sqlx.DB
 	namespace        string
+	statementBuilder sq.StatementBuilderType
 
 	Log func(string, ...interface{})
 }
@@ -194,6 +196,7 @@ func NewSQL(dialect, connectionString string, logger func(string, ...interface{}
 	driver := &SQL{
 		db:               db,
 		Log:              logger,
+		statementBuilder: sq.StatementBuilder.PlaceholderFormat(sq.Dollar),
 	}
 
 	if err := driver.ensureDBSetup(); err != nil {
@@ -209,16 +212,20 @@ func NewSQL(dialect, connectionString string, logger func(string, ...interface{}
 func (s *SQL) Get(key string) (*rspb.Release, error) {
 	var record SQLReleaseWrapper
 
-	query := fmt.Sprintf(
+	qb := s.statementBuilder.
-		"SELECT %s FROM %s WHERE %s = $1 AND %s = $2",
+		Select(sqlReleaseTableBodyColumn).
-		sqlReleaseTableBodyColumn,
+		From(sqlReleaseTableName).
-		sqlReleaseTableName,
+		Where(sq.Eq{sqlReleaseTableKeyColumn: key}).
-		sqlReleaseTableKeyColumn,
+		Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace})
-		sqlReleaseTableNamespaceColumn,
+
-	)
+	query, args, err := qb.ToSql()
+	if err != nil {
+		s.Log("failed to build query: %v", err)
+		return nil, err
+	}
 
 	// Get will return an error if the result is empty
-	if err := s.db.Get(&record, query, key, s.namespace); err != nil {
+	if err := s.db.Get(&record, query, args...); err != nil {
 		s.Log("got SQL error when getting release %s: %v", key, err)
 		return nil, ErrReleaseNotFound
 	}
@@ -234,19 +241,20 @@ func (s *SQL) Get(key string) (*rspb.Release, error) {
 
 // List returns the list of all releases such that filter(release) == true
 func (s *SQL) List(filter func(*rspb.Release) bool) ([]*rspb.Release, error) {
-	query := fmt.Sprintf(
+	sb := s.statementBuilder.
-		"SELECT %s FROM %s WHERE %s = $1",
+		Select(sqlReleaseTableBodyColumn).
-		sqlReleaseTableBodyColumn,
+		From(sqlReleaseTableName).
-		sqlReleaseTableName,
+		Where(sq.Eq{sqlReleaseTableOwnerColumn: sqlReleaseDefaultOwner})
-		sqlReleaseTableOwnerColumn,
-	)
-
-	args := []interface{}{sqlReleaseDefaultOwner}
 
 	// If a namespace was specified, we only list releases from that namespace
 	if s.namespace != "" {
-		query = fmt.Sprintf("%s AND %s = $2", query, sqlReleaseTableNamespaceColumn)
+		sb = sb.Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace})
-		args = append(args, s.namespace)
+	}
+
+	query, args, err := sb.ToSql()
+	if err != nil {
+		s.Log("failed to build query: %v", err)
+		return nil, err
 	}
 
 	var records = []SQLReleaseWrapper{}
@@ -272,15 +280,18 @@ func (s *SQL) List(filter func(*rspb.Release) bool) ([]*rspb.Release, error) {
 
 // Query returns the set of releases that match the provided set of labels.
 func (s *SQL) Query(labels map[string]string) ([]*rspb.Release, error) {
-	var sqlFilterKeys []string
+	sb := s.statementBuilder.
-	sqlFilter := map[string]interface{}{}
+		Select(sqlReleaseTableBodyColumn).
-	for key, val := range labels {
+		From(sqlReleaseTableName)
-		// Build a slice of where filters e.g
+
-		// labels = map[string]string{ "foo": "foo", "bar": "bar" }
+	keys := make([]string, 0, len(labels))
-		// []string{ "foo=?", "bar=?" }
+	for key := range labels {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
+	for _, key := range keys {
 		if _, ok := labelMap[key]; ok {
-			sqlFilterKeys = append(sqlFilterKeys, strings.Join([]string{key, "=:", key}, ""))
+			sb = sb.Where(sq.Eq{key: labels[key]})
-			sqlFilter[key] = val
 		} else {
 			s.Log("unknown label %s", key)
 			return nil, fmt.Errorf("unknow label %s", key)
@@ -289,36 +300,27 @@ func (s *SQL) Query(labels map[string]string) ([]*rspb.Release, error) {
 
 	// If a namespace was specified, we only list releases from that namespace
 	if s.namespace != "" {
-		sqlFilterKeys = append(sqlFilterKeys, fmt.Sprintf("%s=:namespace", sqlReleaseTableNamespaceColumn))
+		sb = sb.Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace})
-		sqlFilter["namespace"] = s.namespace
 	}
 
-	sort.Strings(sqlFilterKeys)
-
 	// Build our query
-	query := strings.Join([]string{
+	query, args, err := sb.ToSql()
-		fmt.Sprintf("SELECT %s FROM %s", sqlReleaseTableBodyColumn, sqlReleaseTableName),
-		"WHERE",
-		strings.Join(sqlFilterKeys, " AND "),
-	}, " ")
-
-	rows, err := s.db.NamedQuery(query, sqlFilter)
 	if err != nil {
-		s.Log("failed to query with labels: %v", err)
+		s.Log("failed to build query: %v", err)
 		return nil, err
 	}
 
-	var releases []*rspb.Release
+	var records = []SQLReleaseWrapper{}
-	for rows.Next() {
+	if err := s.db.Select(&records, query, args...); err != nil {
-		var record SQLReleaseWrapper
+		s.Log("list: failed to query with labels: %v", err)
-		if err = rows.StructScan(&record); err != nil {
-			s.Log("failed to scan record %q: %v", record, err)
 		return nil, err
 	}
 
+	var releases []*rspb.Release
+	for _, record := range records {
 		release, err := decodeRelease(record.Body)
 		if err != nil {
-			s.Log("failed to decode release: %v", err)
+			s.Log("list: failed to decode release: %v: %v", record, err)
 			continue
 		}
 		releases = append(releases, release)
@@ -351,9 +353,9 @@ func (s *SQL) Create(key string, rls *rspb.Release) error {
 		return fmt.Errorf("error beginning transaction: %v", err)
 	}
 
-	query := fmt.Sprintf(
+	insertQuery, args, err := s.statementBuilder.
-		"INSERT INTO %s (%s, %s, %s, %s, %s, %s, %s, %s, %s) VALUES (:key, :type, :body, :name, :namespace, :version, :status, :owner, :createdAt)",
+		Insert(sqlReleaseTableName).
-		sqlReleaseTableName,
+		Columns(
 			sqlReleaseTableKeyColumn,
 			sqlReleaseTableTypeColumn,
 			sqlReleaseTableBodyColumn,
@@ -363,34 +365,39 @@ func (s *SQL) Create(key string, rls *rspb.Release) error {
 			sqlReleaseTableStatusColumn,
 			sqlReleaseTableOwnerColumn,
 			sqlReleaseTableCreatedAtColumn,
-	)
+		).
+		Values(
+			key,
+			sqlReleaseDefaultType,
+			body,
+			rls.Name,
+			namespace,
+			int(rls.Version),
+			rls.Info.Status.String(),
+			sqlReleaseDefaultOwner,
+			int(time.Now().Unix()),
+		).ToSql()
+	if err != nil {
+		s.Log("failed to build insert query: %v", err)
+		return err
+	}
 
-	if _, err := transaction.NamedExec(query,
+	if _, err := transaction.Exec(insertQuery, args...); err != nil {
-		&SQLReleaseWrapper{
-			Key:  key,
-			Type: sqlReleaseDefaultType,
-			Body: body,
-
-			Name:      rls.Name,
-			Namespace: namespace,
-			Version:   int(rls.Version),
-			Status:    rls.Info.Status.String(),
-			Owner:     sqlReleaseDefaultOwner,
-			CreatedAt: int(time.Now().Unix()),
-		},
-	); err != nil {
 		defer transaction.Rollback()
 
-		query := fmt.Sprintf(
+		selectQuery, args, buildErr := s.statementBuilder.
-			"SELECT %s FROM %s WHERE %s = $1 and %s = $2",
+			Select(sqlReleaseTableKeyColumn).
-			sqlReleaseTableKeyColumn,
+			From(sqlReleaseTableName).
-			sqlReleaseTableName,
+			Where(sq.Eq{sqlReleaseTableKeyColumn: key}).
-			sqlReleaseTableKeyColumn,
+			Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}).
-			sqlReleaseTableNamespaceColumn,
+			ToSql()
-		)
+		if buildErr != nil {
+			s.Log("failed to build select query: %v", buildErr)
+			return err
+		}
 
 		var record SQLReleaseWrapper
-		if err := transaction.Get(&record, query, key, s.namespace); err == nil {
+		if err := transaction.Get(&record, selectQuery, args...); err == nil {
 			s.Log("release %s already exists", key)
 			return ErrReleaseExists
 		}
@@ -417,31 +424,24 @@ func (s *SQL) Update(key string, rls *rspb.Release) error {
 		return err
 	}
 
-	query := fmt.Sprintf(
+	query, args, err := s.statementBuilder.
-		"UPDATE %s SET %s=:body, %s=:name, %s=:version, %s=:status, %s=:owner, %s=:modifiedAt WHERE %s=:key AND %s=:namespace",
+		Update(sqlReleaseTableName).
-		sqlReleaseTableName,
+		Set(sqlReleaseTableBodyColumn, body).
-		sqlReleaseTableBodyColumn,
+		Set(sqlReleaseTableNameColumn, rls.Name).
-		sqlReleaseTableNameColumn,
+		Set(sqlReleaseTableVersionColumn, int(rls.Version)).
-		sqlReleaseTableVersionColumn,
+		Set(sqlReleaseTableStatusColumn, rls.Info.Status.String()).
-		sqlReleaseTableStatusColumn,
+		Set(sqlReleaseTableOwnerColumn, sqlReleaseDefaultOwner).
-		sqlReleaseTableOwnerColumn,
+		Set(sqlReleaseTableModifiedAtColumn, int(time.Now().Unix())).
-		sqlReleaseTableModifiedAtColumn,
+		Where(sq.Eq{sqlReleaseTableKeyColumn: key}).
-		sqlReleaseTableKeyColumn,
+		Where(sq.Eq{sqlReleaseTableNamespaceColumn: namespace}).
-		sqlReleaseTableNamespaceColumn,
+		ToSql()
-	)
 
-	if _, err := s.db.NamedExec(query,
+	if err != nil {
-		&SQLReleaseWrapper{
+		s.Log("failed to build update query: %v", err)
-			Key:        key,
+		return err
-			Body:       body,
+	}
-			Name:       rls.Name,
+
-			Namespace:  namespace,
+	if _, err := s.db.Exec(query, args...); err != nil {
-			Version:    int(rls.Version),
-			Status:     rls.Info.Status.String(),
-			Owner:      sqlReleaseDefaultOwner,
-			ModifiedAt: int(time.Now().Unix()),
-		},
-	); err != nil {
 		s.Log("failed to update release %s in SQL database: %v", key, err)
 		return err
 	}
@@ -457,16 +457,19 @@ func (s *SQL) Delete(key string) (*rspb.Release, error) {
 		return nil, fmt.Errorf("error beginning transaction: %v", err)
 	}
 
-	selectQuery := fmt.Sprintf(
+	selectQuery, args, err := s.statementBuilder.
-		"SELECT %s FROM %s WHERE %s=$1 AND %s=$2",
+		Select(sqlReleaseTableBodyColumn).
-		sqlReleaseTableBodyColumn,
+		From(sqlReleaseTableName).
-		sqlReleaseTableName,
+		Where(sq.Eq{sqlReleaseTableKeyColumn: key}).
-		sqlReleaseTableKeyColumn,
+		Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}).
-		sqlReleaseTableNamespaceColumn,
+		ToSql()
-	)
+	if err != nil {
+		s.Log("failed to build select query: %v", err)
+		return nil, err
+	}
 
 	var record SQLReleaseWrapper
-	err = transaction.Get(&record, selectQuery, key, s.namespace)
+	err = transaction.Get(&record, selectQuery, args...)
 	if err != nil {
 		s.Log("release %s not found: %v", key, err)
 		return nil, ErrReleaseNotFound
@@ -480,13 +483,16 @@ func (s *SQL) Delete(key string) (*rspb.Release, error) {
 	}
 	defer transaction.Commit()
 
-	deleteQuery := fmt.Sprintf(
+	deleteQuery, args, err := s.statementBuilder.
-		"DELETE FROM %s WHERE %s = $1 AND %s = $2",
+		Delete(sqlReleaseTableName).
-		sqlReleaseTableName,
+		Where(sq.Eq{sqlReleaseTableKeyColumn: key}).
-		sqlReleaseTableKeyColumn,
+		Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}).
-		sqlReleaseTableNamespaceColumn,
+		ToSql()
-	)
+	if err != nil {
+		s.Log("failed to build select query: %v", err)
+		return nil, err
+	}
 
-	_, err = transaction.Exec(deleteQuery, key, s.namespace)
+	_, err = transaction.Exec(deleteQuery, args...)
 	return release, err
 }

pkg/storage/driver/sql_test.go

@@ -163,7 +163,7 @@ func TestSqlCreate(t *testing.T) {
 	body, _ := encodeRelease(rel)
 
 	query := fmt.Sprintf(
-		"INSERT INTO %s (%s, %s, %s, %s, %s, %s, %s, %s, %s) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)",
+		"INSERT INTO %s (%s,%s,%s,%s,%s,%s,%s,%s,%s) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9)",
 		sqlReleaseTableName,
 		sqlReleaseTableKeyColumn,
 		sqlReleaseTableTypeColumn,
@@ -203,7 +203,7 @@ func TestSqlCreateAlreadyExists(t *testing.T) {
 	body, _ := encodeRelease(rel)
 
 	insertQuery := fmt.Sprintf(
-		"INSERT INTO %s (%s, %s, %s, %s, %s, %s, %s, %s, %s) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)",
+		"INSERT INTO %s (%s,%s,%s,%s,%s,%s,%s,%s,%s) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9)",
 		sqlReleaseTableName,
 		sqlReleaseTableKeyColumn,
 		sqlReleaseTableTypeColumn,
@@ -224,7 +224,7 @@ func TestSqlCreateAlreadyExists(t *testing.T) {
 		WillReturnError(fmt.Errorf("dialect dependent SQL error"))
 
 	selectQuery := fmt.Sprintf(
-		regexp.QuoteMeta("SELECT %s FROM %s WHERE %s = $1 and %s = $2"),
+		regexp.QuoteMeta("SELECT %s FROM %s WHERE %s = $1 AND %s = $2"),
 		sqlReleaseTableKeyColumn,
 		sqlReleaseTableName,
 		sqlReleaseTableKeyColumn,
@@ -264,7 +264,7 @@ func TestSqlUpdate(t *testing.T) {
 	body, _ := encodeRelease(rel)
 
 	query := fmt.Sprintf(
-		"UPDATE %s SET %s=?, %s=?, %s=?, %s=?, %s=?, %s=? WHERE %s=? AND %s=?",
+		"UPDATE %s SET %s = $1, %s = $2, %s = $3, %s = $4, %s = $5, %s = $6 WHERE %s = $7 AND %s = $8",
 		sqlReleaseTableName,
 		sqlReleaseTableBodyColumn,
 		sqlReleaseTableNameColumn,
@@ -311,18 +311,18 @@ func TestSqlQuery(t *testing.T) {
 	sqlDriver, mock := newTestFixtureSQL(t)
 
 	query := fmt.Sprintf(
-		"SELECT %s FROM %s WHERE %s=? AND %s=? AND %s=? AND %s=?",
+		"SELECT %s FROM %s WHERE %s = $1 AND %s = $2 AND %s = $3 AND %s = $4",
 		sqlReleaseTableBodyColumn,
 		sqlReleaseTableName,
 		sqlReleaseTableNameColumn,
-		sqlReleaseTableNamespaceColumn,
 		sqlReleaseTableOwnerColumn,
 		sqlReleaseTableStatusColumn,
+		sqlReleaseTableNamespaceColumn,
 	)
 
 	mock.
 		ExpectQuery(regexp.QuoteMeta(query)).
-		WithArgs("smug-pigeon", "default", sqlReleaseDefaultOwner, "deployed").
+		WithArgs("smug-pigeon", sqlReleaseDefaultOwner, "deployed", "default").
 		WillReturnRows(
 			mock.NewRows([]string{
 				sqlReleaseTableBodyColumn,
@@ -332,17 +332,17 @@ func TestSqlQuery(t *testing.T) {
 		).RowsWillBeClosed()
 
 	query = fmt.Sprintf(
-		"SELECT %s FROM %s WHERE %s=? AND %s=? AND %s=?",
+		"SELECT %s FROM %s WHERE %s = $1 AND %s = $2 AND %s = $3",
 		sqlReleaseTableBodyColumn,
 		sqlReleaseTableName,
 		sqlReleaseTableNameColumn,
-		sqlReleaseTableNamespaceColumn,
 		sqlReleaseTableOwnerColumn,
+		sqlReleaseTableNamespaceColumn,
 	)
 
 	mock.
 		ExpectQuery(regexp.QuoteMeta(query)).
-		WithArgs("smug-pigeon", "default", sqlReleaseDefaultOwner).
+		WithArgs("smug-pigeon", sqlReleaseDefaultOwner, "default").
 		WillReturnRows(
 			mock.NewRows([]string{
 				sqlReleaseTableBodyColumn,