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:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to