diff --git a/go.mod b/go.mod index d56f5c40c..7731ea9f6 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 3e49d90b7..e84658113 100644 --- a/go.sum +++ b/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= diff --git a/pkg/storage/driver/mock_test.go b/pkg/storage/driver/mock_test.go index 5a5054c0d..c0236ece8 100644 --- a/pkg/storage/driver/mock_test.go +++ b/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" @@ -256,8 +257,9 @@ func newTestFixtureSQL(t *testing.T, releases ...*rspb.Release) (*SQL, sqlmock.S sqlxDB := sqlx.NewDb(sqlDB, "sqlmock") return &SQL{ - db: sqlxDB, - Log: func(a string, b ...interface{}) {}, - namespace: "default", + db: sqlxDB, + Log: func(a string, b ...interface{}) {}, + namespace: "default", + statementBuilder: sq.StatementBuilder.PlaceholderFormat(sq.Dollar), }, mock } diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 56642666e..07f5f8cbc 100644 --- a/pkg/storage/driver/sql.go +++ b/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" @@ -71,8 +72,9 @@ const ( // SQL is the sql storage driver implementation. type SQL struct { - db *sqlx.DB - namespace string + db *sqlx.DB + namespace string + statementBuilder sq.StatementBuilderType Log func(string, ...interface{}) } @@ -192,8 +194,9 @@ func NewSQL(dialect, connectionString string, logger func(string, ...interface{} } driver := &SQL{ - db: db, - Log: logger, + 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( - "SELECT %s FROM %s WHERE %s = $1 AND %s = $2", - sqlReleaseTableBodyColumn, - sqlReleaseTableName, - sqlReleaseTableKeyColumn, - sqlReleaseTableNamespaceColumn, - ) + qb := s.statementBuilder. + Select(sqlReleaseTableBodyColumn). + From(sqlReleaseTableName). + Where(sq.Eq{sqlReleaseTableKeyColumn: key}). + Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}) + + 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( - "SELECT %s FROM %s WHERE %s = $1", - sqlReleaseTableBodyColumn, - sqlReleaseTableName, - sqlReleaseTableOwnerColumn, - ) - - args := []interface{}{sqlReleaseDefaultOwner} + sb := s.statementBuilder. + Select(sqlReleaseTableBodyColumn). + From(sqlReleaseTableName). + Where(sq.Eq{sqlReleaseTableOwnerColumn: 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) - args = append(args, s.namespace) + sb = sb.Where(sq.Eq{sqlReleaseTableNamespaceColumn: 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 - sqlFilter := map[string]interface{}{} - for key, val := range labels { - // Build a slice of where filters e.g - // labels = map[string]string{ "foo": "foo", "bar": "bar" } - // []string{ "foo=?", "bar=?" } + sb := s.statementBuilder. + Select(sqlReleaseTableBodyColumn). + From(sqlReleaseTableName) + + keys := make([]string, 0, len(labels)) + 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}, "")) - sqlFilter[key] = val + sb = sb.Where(sq.Eq{key: labels[key]}) } 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)) - sqlFilter["namespace"] = s.namespace + sb = sb.Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}) } - sort.Strings(sqlFilterKeys) - // Build our query - query := strings.Join([]string{ - fmt.Sprintf("SELECT %s FROM %s", sqlReleaseTableBodyColumn, sqlReleaseTableName), - "WHERE", - strings.Join(sqlFilterKeys, " AND "), - }, " ") - - rows, err := s.db.NamedQuery(query, sqlFilter) + query, args, err := sb.ToSql() 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 - for rows.Next() { - var record SQLReleaseWrapper - if err = rows.StructScan(&record); err != nil { - s.Log("failed to scan record %q: %v", record, err) - return nil, err - } + var records = []SQLReleaseWrapper{} + if err := s.db.Select(&records, query, args...); err != nil { + s.Log("list: failed to query with labels: %v", 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,46 +353,51 @@ func (s *SQL) Create(key string, rls *rspb.Release) error { return fmt.Errorf("error beginning transaction: %v", err) } - query := fmt.Sprintf( - "INSERT INTO %s (%s, %s, %s, %s, %s, %s, %s, %s, %s) VALUES (:key, :type, :body, :name, :namespace, :version, :status, :owner, :createdAt)", - sqlReleaseTableName, - sqlReleaseTableKeyColumn, - sqlReleaseTableTypeColumn, - sqlReleaseTableBodyColumn, - sqlReleaseTableNameColumn, - sqlReleaseTableNamespaceColumn, - sqlReleaseTableVersionColumn, - sqlReleaseTableStatusColumn, - sqlReleaseTableOwnerColumn, - sqlReleaseTableCreatedAtColumn, - ) - - if _, err := transaction.NamedExec(query, - &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( - "SELECT %s FROM %s WHERE %s = $1 and %s = $2", - sqlReleaseTableKeyColumn, - sqlReleaseTableName, + insertQuery, args, err := s.statementBuilder. + Insert(sqlReleaseTableName). + Columns( sqlReleaseTableKeyColumn, + sqlReleaseTableTypeColumn, + sqlReleaseTableBodyColumn, + sqlReleaseTableNameColumn, sqlReleaseTableNamespaceColumn, - ) + sqlReleaseTableVersionColumn, + 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.Exec(insertQuery, args...); err != nil { + defer transaction.Rollback() + + selectQuery, args, buildErr := s.statementBuilder. + Select(sqlReleaseTableKeyColumn). + From(sqlReleaseTableName). + Where(sq.Eq{sqlReleaseTableKeyColumn: key}). + Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}). + 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( - "UPDATE %s SET %s=:body, %s=:name, %s=:version, %s=:status, %s=:owner, %s=:modifiedAt WHERE %s=:key AND %s=:namespace", - sqlReleaseTableName, - sqlReleaseTableBodyColumn, - sqlReleaseTableNameColumn, - sqlReleaseTableVersionColumn, - sqlReleaseTableStatusColumn, - sqlReleaseTableOwnerColumn, - sqlReleaseTableModifiedAtColumn, - sqlReleaseTableKeyColumn, - sqlReleaseTableNamespaceColumn, - ) - - if _, err := s.db.NamedExec(query, - &SQLReleaseWrapper{ - Key: key, - Body: body, - Name: rls.Name, - Namespace: namespace, - Version: int(rls.Version), - Status: rls.Info.Status.String(), - Owner: sqlReleaseDefaultOwner, - ModifiedAt: int(time.Now().Unix()), - }, - ); err != nil { + query, args, err := s.statementBuilder. + Update(sqlReleaseTableName). + Set(sqlReleaseTableBodyColumn, body). + Set(sqlReleaseTableNameColumn, rls.Name). + Set(sqlReleaseTableVersionColumn, int(rls.Version)). + Set(sqlReleaseTableStatusColumn, rls.Info.Status.String()). + Set(sqlReleaseTableOwnerColumn, sqlReleaseDefaultOwner). + Set(sqlReleaseTableModifiedAtColumn, int(time.Now().Unix())). + Where(sq.Eq{sqlReleaseTableKeyColumn: key}). + Where(sq.Eq{sqlReleaseTableNamespaceColumn: namespace}). + ToSql() + + if err != nil { + s.Log("failed to build update query: %v", err) + return err + } + + if _, err := s.db.Exec(query, args...); 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( - "SELECT %s FROM %s WHERE %s=$1 AND %s=$2", - sqlReleaseTableBodyColumn, - sqlReleaseTableName, - sqlReleaseTableKeyColumn, - sqlReleaseTableNamespaceColumn, - ) + selectQuery, args, err := s.statementBuilder. + Select(sqlReleaseTableBodyColumn). + From(sqlReleaseTableName). + Where(sq.Eq{sqlReleaseTableKeyColumn: key}). + Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}). + 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( - "DELETE FROM %s WHERE %s = $1 AND %s = $2", - sqlReleaseTableName, - sqlReleaseTableKeyColumn, - sqlReleaseTableNamespaceColumn, - ) + deleteQuery, args, err := s.statementBuilder. + Delete(sqlReleaseTableName). + Where(sq.Eq{sqlReleaseTableKeyColumn: key}). + Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}). + 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 } diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index 6486e91a8..1562a90aa 100644 --- a/pkg/storage/driver/sql_test.go +++ b/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, @@ -396,7 +396,7 @@ func TestSqlDelete(t *testing.T) { sqlDriver, mock := newTestFixtureSQL(t) selectQuery := fmt.Sprintf( - "SELECT %s FROM %s WHERE %s=$1 AND %s=$2", + "SELECT %s FROM %s WHERE %s = $1 AND %s = $2", sqlReleaseTableBodyColumn, sqlReleaseTableName, sqlReleaseTableKeyColumn,