You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2018/02/09 18:37:48 UTC
[GitHub] dangogh closed pull request #1864: fix unclosed rows and error overwriting in cdns.go
dangogh closed pull request #1864: fix unclosed rows and error overwriting in cdns.go
URL: https://github.com/apache/incubator-trafficcontrol/pull/1864
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/traffic_ops/traffic_ops_golang/cdn/cdns.go b/traffic_ops/traffic_ops_golang/cdn/cdns.go
index ca02d09395..5513a23cdc 100644
--- a/traffic_ops/traffic_ops_golang/cdn/cdns.go
+++ b/traffic_ops/traffic_ops_golang/cdn/cdns.go
@@ -128,16 +128,16 @@ FROM cdn c`
//if so, it will return an errorType of DataConflict and the type should be appended to the
//generic error message returned
func (cdn *TOCDN) Update(db *sqlx.DB, user auth.CurrentUser) (error, tc.ApiErrorType) {
+ rollbackTransaction := true
tx, err := db.Beginx()
defer func() {
- if tx == nil {
+ if tx == nil || !rollbackTransaction {
return
}
+ err := tx.Rollback()
if err != nil {
- tx.Rollback()
- return
+ log.Errorln(errors.New("rolling back transaction: " + err.Error()))
}
- tx.Commit()
}()
if err != nil {
@@ -147,8 +147,8 @@ func (cdn *TOCDN) Update(db *sqlx.DB, user auth.CurrentUser) (error, tc.ApiError
log.Debugf("about to run exec query: %s with cdn: %++v", updateCDNQuery(), cdn)
resultRows, err := tx.NamedQuery(updateCDNQuery(), cdn)
if err != nil {
- if err, ok := err.(*pq.Error); ok {
- err, eType := dbhelpers.ParsePQUniqueConstraintError(err)
+ if pqErr, ok := err.(*pq.Error); ok {
+ err, eType := dbhelpers.ParsePQUniqueConstraintError(pqErr)
if eType == tc.DataConflictError {
return errors.New("a cdn with " + err.Error()), eType
}
@@ -158,6 +158,8 @@ func (cdn *TOCDN) Update(db *sqlx.DB, user auth.CurrentUser) (error, tc.ApiError
return tc.DBError, tc.SystemError
}
}
+ defer resultRows.Close()
+
var lastUpdated tc.Time
rowsAffected := 0
for resultRows.Next() {
@@ -176,6 +178,12 @@ func (cdn *TOCDN) Update(db *sqlx.DB, user auth.CurrentUser) (error, tc.ApiError
return fmt.Errorf("this update affected too many rows: %d", rowsAffected), tc.SystemError
}
}
+ err = tx.Commit()
+ if err != nil {
+ log.Errorln("Could not commit transaction: ", err)
+ return tc.DBError, tc.SystemError
+ }
+ rollbackTransaction = false
return nil, tc.NoError
}
@@ -187,16 +195,16 @@ func (cdn *TOCDN) Update(db *sqlx.DB, user auth.CurrentUser) (error, tc.ApiError
//The insert sql returns the id and lastUpdated values of the newly inserted cdn and have
//to be added to the struct
func (cdn *TOCDN) Insert(db *sqlx.DB, user auth.CurrentUser) (error, tc.ApiErrorType) {
+ rollbackTransaction := true
tx, err := db.Beginx()
defer func() {
- if tx == nil {
+ if tx == nil || !rollbackTransaction {
return
}
+ err := tx.Rollback()
if err != nil {
- tx.Rollback()
- return
+ log.Errorln(errors.New("rolling back transaction: " + err.Error()))
}
- tx.Commit()
}()
if err != nil {
@@ -205,14 +213,19 @@ func (cdn *TOCDN) Insert(db *sqlx.DB, user auth.CurrentUser) (error, tc.ApiError
}
resultRows, err := tx.NamedQuery(insertCDNQuery(), cdn)
if err != nil {
- if err, ok := err.(*pq.Error); ok {
- err, eType := dbhelpers.ParsePQUniqueConstraintError(err)
- return errors.New("a cdn with " + err.Error()), eType
+ if pqErr, ok := err.(*pq.Error); ok {
+ err, eType := dbhelpers.ParsePQUniqueConstraintError(pqErr)
+ if eType == tc.DataConflictError {
+ return errors.New("a cdn with " + err.Error()), eType
+ }
+ return err, eType
} else {
log.Errorf("received non pq error: %++v from create execution", err)
return tc.DBError, tc.SystemError
}
}
+ defer resultRows.Close()
+
var id int
var lastUpdated tc.Time
rowsAffected := 0
@@ -234,22 +247,28 @@ func (cdn *TOCDN) Insert(db *sqlx.DB, user auth.CurrentUser) (error, tc.ApiError
}
cdn.SetID(id)
cdn.LastUpdated = lastUpdated
+ err = tx.Commit()
+ if err != nil {
+ log.Errorln("Could not commit transaction: ", err)
+ return tc.DBError, tc.SystemError
+ }
+ rollbackTransaction = false
return nil, tc.NoError
}
//The CDN implementation of the Deleter interface
//all implementations of Deleter should use transactions and return the proper errorType
func (cdn *TOCDN) Delete(db *sqlx.DB, user auth.CurrentUser) (error, tc.ApiErrorType) {
+ rollbackTransaction := true
tx, err := db.Beginx()
defer func() {
- if tx == nil {
+ if tx == nil || !rollbackTransaction {
return
}
+ err := tx.Rollback()
if err != nil {
- tx.Rollback()
- return
+ log.Errorln(errors.New("rolling back transaction: " + err.Error()))
}
- tx.Commit()
}()
if err != nil {
@@ -273,6 +292,12 @@ func (cdn *TOCDN) Delete(db *sqlx.DB, user auth.CurrentUser) (error, tc.ApiError
return fmt.Errorf("this create affected too many rows: %d", rowsAffected), tc.SystemError
}
}
+ err = tx.Commit()
+ if err != nil {
+ log.Errorln("Could not commit transaction: ", err)
+ return tc.DBError, tc.SystemError
+ }
+ rollbackTransaction = false
return nil, tc.NoError
}
diff --git a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
index 24f0f7ce79..0c8774b306 100644
--- a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
+++ b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
@@ -25,6 +25,7 @@ import (
"github.com/apache/incubator-trafficcontrol/lib/go-log"
"github.com/apache/incubator-trafficcontrol/lib/go-tc"
+
"github.com/lib/pq"
)
diff --git a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers_test.go b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers_test.go
index 4cc5729686..5607dfc44e 100644
--- a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers_test.go
+++ b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers_test.go
@@ -35,7 +35,7 @@ func stripAllWhitespace(s string) string {
}
func TestBuildQuery(t *testing.T) {
- v := map[string]string{"param1": "queryParamv1","param2": "queryParamv2"}
+ v := map[string]string{"param1": "queryParamv1", "param2": "queryParamv2"}
selectStmt := `SELECT
t.col1,
@@ -45,8 +45,8 @@ FROM table t
// Query Parameters to Database Query column mappings
// see the fields mapped in the SQL query
queryParamsToSQLCols := map[string]WhereColumnInfo{
- "param1": WhereColumnInfo{"t.col1",nil},
- "param2": WhereColumnInfo{"t.col2",nil},
+ "param1": WhereColumnInfo{"t.col1", nil},
+ "param2": WhereColumnInfo{"t.col2", nil},
}
where, orderBy, queryValues, _ := BuildWhereAndOrderBy(v, queryParamsToSQLCols)
query := selectStmt + where + orderBy
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services