[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6064: Fix Internal server error in update cachegroups by already exist cg name

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #6064:
URL: https://github.com/apache/trafficcontrol/pull/6064#discussion_r681827478



##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -414,22 +414,22 @@ func (cg *TOCacheGroup) createCoordinate() (*int, error) {
 	return coordinateID, nil
 }
 
-func (cg *TOCacheGroup) updateCoordinate() error {
+func (cg *TOCacheGroup) updateCoordinate() (*int, error, error, int) {

Review comment:
       It looks like first return value is always `nil` - what's that for?

##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -414,22 +414,22 @@ func (cg *TOCacheGroup) createCoordinate() (*int, error) {
 	return coordinateID, nil
 }
 
-func (cg *TOCacheGroup) updateCoordinate() error {
+func (cg *TOCacheGroup) updateCoordinate() (*int, error, error, int) {
 	if cg.Latitude != nil && cg.Longitude != nil {
 		q := `UPDATE coordinate SET name = $1, latitude = $2, longitude = $3 WHERE id = (SELECT coordinate FROM cachegroup WHERE id = $4)`
 		result, err := cg.ReqInfo.Tx.Tx.Exec(q, tc.CachegroupCoordinateNamePrefix+*cg.Name, *cg.Latitude, *cg.Longitude, *cg.ID)
 		if err != nil {
-			return fmt.Errorf("updating coordinate for cachegroup '%s': %s", *cg.Name, err.Error())
+			return nil, errors.New("cachegroup name already exist, give different name"), tc.DBError, http.StatusBadRequest
 		}
 		rowsAffected, err := result.RowsAffected()
 		if err != nil {
-			return fmt.Errorf("updating coordinate for cachegroup '%s', getting rows affected: %s", *cg.Name, err.Error())
+			return nil, errors.New("updating coordinate for cachegroup , getting rows affected:" + *cg.Name + err.Error()), tc.DBError, http.StatusInternalServerError

Review comment:
       changing this to `errors.New` changes the formatting, so that it needs to be re-ordered or re-worded - I'd just leave it as `fmt.Errorf`, there's no need to change it.

##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -414,22 +414,22 @@ func (cg *TOCacheGroup) createCoordinate() (*int, error) {
 	return coordinateID, nil
 }
 
-func (cg *TOCacheGroup) updateCoordinate() error {
+func (cg *TOCacheGroup) updateCoordinate() (*int, error, error, int) {
 	if cg.Latitude != nil && cg.Longitude != nil {
 		q := `UPDATE coordinate SET name = $1, latitude = $2, longitude = $3 WHERE id = (SELECT coordinate FROM cachegroup WHERE id = $4)`
 		result, err := cg.ReqInfo.Tx.Tx.Exec(q, tc.CachegroupCoordinateNamePrefix+*cg.Name, *cg.Latitude, *cg.Longitude, *cg.ID)
 		if err != nil {
-			return fmt.Errorf("updating coordinate for cachegroup '%s': %s", *cg.Name, err.Error())
+			return nil, errors.New("cachegroup name already exist, give different name"), tc.DBError, http.StatusBadRequest
 		}
 		rowsAffected, err := result.RowsAffected()
 		if err != nil {
-			return fmt.Errorf("updating coordinate for cachegroup '%s', getting rows affected: %s", *cg.Name, err.Error())
+			return nil, errors.New("updating coordinate for cachegroup , getting rows affected:" + *cg.Name + err.Error()), tc.DBError, http.StatusInternalServerError
 		}
 		if rowsAffected == 0 {
-			return fmt.Errorf("updating coordinate for cachegroup '%s', zero rows affected", *cg.Name)
+			return nil, errors.New("updating coordinate for cachegroup - Zero rows affected" + *cg.Name), tc.DBError, http.StatusInternalServerError

Review comment:
       same as above RE: changing `fmt.Errorf` to `errors.New`

##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -414,22 +414,22 @@ func (cg *TOCacheGroup) createCoordinate() (*int, error) {
 	return coordinateID, nil
 }
 
-func (cg *TOCacheGroup) updateCoordinate() error {
+func (cg *TOCacheGroup) updateCoordinate() (*int, error, error, int) {
 	if cg.Latitude != nil && cg.Longitude != nil {
 		q := `UPDATE coordinate SET name = $1, latitude = $2, longitude = $3 WHERE id = (SELECT coordinate FROM cachegroup WHERE id = $4)`
 		result, err := cg.ReqInfo.Tx.Tx.Exec(q, tc.CachegroupCoordinateNamePrefix+*cg.Name, *cg.Latitude, *cg.Longitude, *cg.ID)
 		if err != nil {
-			return fmt.Errorf("updating coordinate for cachegroup '%s': %s", *cg.Name, err.Error())
+			return nil, errors.New("cachegroup name already exist, give different name"), tc.DBError, http.StatusBadRequest
 		}
 		rowsAffected, err := result.RowsAffected()
 		if err != nil {
-			return fmt.Errorf("updating coordinate for cachegroup '%s', getting rows affected: %s", *cg.Name, err.Error())
+			return nil, errors.New("updating coordinate for cachegroup , getting rows affected:" + *cg.Name + err.Error()), tc.DBError, http.StatusInternalServerError
 		}
 		if rowsAffected == 0 {
-			return fmt.Errorf("updating coordinate for cachegroup '%s', zero rows affected", *cg.Name)
+			return nil, errors.New("updating coordinate for cachegroup - Zero rows affected" + *cg.Name), tc.DBError, http.StatusInternalServerError
 		}
 	}
-	return nil
+	return nil, nil, nil, http.StatusBadRequest

Review comment:
       If no error occurred the status code should, by convention, be `http.StatusOK`. I think zero would also be acceptable, since no actual code is prescribed when no errors occur, but people tend to use `http.StatusOK` just in case it bubbles out to the actual response.

##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -687,8 +687,8 @@ func (cg *TOCacheGroup) handleCoordinateUpdate() (*int, error, error, int) {
 		return nil, nil, nil, http.StatusOK
 	}
 
-	if err = cg.updateCoordinate(); err != nil {
-		return nil, nil, tc.DBError, http.StatusInternalServerError
+	if err1, err2, DBError, statusCode := cg.updateCoordinate(); err != nil {
+		return err1, err2, DBError, statusCode

Review comment:
       This isn't checking any of the error values returned by `cg.updatedCoordinate()`, essentially ignoring them and only returning if `cg.getCoordinateID` failed earlier - and if that were true it wouldn't have gotten this far.

##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -414,22 +414,22 @@ func (cg *TOCacheGroup) createCoordinate() (*int, error) {
 	return coordinateID, nil
 }
 
-func (cg *TOCacheGroup) updateCoordinate() error {
+func (cg *TOCacheGroup) updateCoordinate() (*int, error, error, int) {
 	if cg.Latitude != nil && cg.Longitude != nil {
 		q := `UPDATE coordinate SET name = $1, latitude = $2, longitude = $3 WHERE id = (SELECT coordinate FROM cachegroup WHERE id = $4)`
 		result, err := cg.ReqInfo.Tx.Tx.Exec(q, tc.CachegroupCoordinateNamePrefix+*cg.Name, *cg.Latitude, *cg.Longitude, *cg.ID)
 		if err != nil {
-			return fmt.Errorf("updating coordinate for cachegroup '%s': %s", *cg.Name, err.Error())
+			return nil, errors.New("cachegroup name already exist, give different name"), tc.DBError, http.StatusBadRequest

Review comment:
       This error does not always indicate that a CacheGroup by the given name already exists - and since it`s returning `errors.New("constant string")` and `tc.DBError`, the actual error that occurred is obscured so that looking into the logs won't tell you what went wrong.
   
   This error should be returned as a system error, and a check earlier in handling should look for a name collision before handling reaches this point.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org