You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficcontrol.apache.org by ro...@apache.org on 2019/04/29 19:39:58 UTC
[trafficcontrol] branch master updated: TO: Give a better error
message when deleting an object being referenced (#3445)
This is an automated email from the ASF dual-hosted git repository.
rob pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git
The following commit(s) were added to refs/heads/master by this push:
new 80f555b TO: Give a better error message when deleting an object being referenced (#3445)
80f555b is described below
commit 80f555ba38cb073baecf17b43dd86b31b516b9c6
Author: Matthew Allen Moltzau <ma...@moltzau.net>
AuthorDate: Mon Apr 29 13:39:52 2019 -0600
TO: Give a better error message when deleting an object being referenced (#3445)
* Gives a better error message when deleting an object being referenced
Known Examples:
* deleting coordinate with a cachegroup
* deleting profile with a server (Fixes #3410)
* deleting division with assigned region (Fixes #2729)
Any endpoint that uses the generic cruder should be fixed.
Any endpoint that doesn't use the generic cruder can use
api.ParseDBError to get the same result.
* Added api test for division connected to region
(The division endpoint uses the generic delete)
* Updated test to use correct endpoint and test for correct message
---
traffic_ops/testing/api/v14/divisions_test.go | 31 ++++++++++++-
traffic_ops/traffic_ops_golang/api/api.go | 51 +++++++++++++++++++---
traffic_ops/traffic_ops_golang/api/generic_crud.go | 2 +-
3 files changed, 75 insertions(+), 9 deletions(-)
diff --git a/traffic_ops/testing/api/v14/divisions_test.go b/traffic_ops/testing/api/v14/divisions_test.go
index d071ac7..880732f 100644
--- a/traffic_ops/testing/api/v14/divisions_test.go
+++ b/traffic_ops/testing/api/v14/divisions_test.go
@@ -16,6 +16,7 @@ package v14
*/
import (
+ "strings"
"testing"
"github.com/apache/trafficcontrol/lib/go-log"
@@ -23,12 +24,40 @@ import (
)
func TestDivisions(t *testing.T) {
- WithObjs(t, []TCObj{Parameters, Divisions}, func() {
+ WithObjs(t, []TCObj{Parameters, Divisions, Regions}, func() {
+ TryToDeleteDivision(t)
UpdateTestDivisions(t)
GetTestDivisions(t)
})
}
+func TryToDeleteDivision(t *testing.T) {
+ division := testData.Divisions[0]
+
+ resp, _, err := TOSession.GetDivisionByName(division.Name)
+ if err != nil {
+ t.Errorf("cannot GET Division by name: %v - %v\n", division.Name, err)
+ }
+ division = resp[0]
+ _, _, err = TOSession.DeleteDivisionByID(division.ID)
+
+ if err == nil {
+ t.Errorf("should not be able to delete a division prematurely")
+ return
+ }
+
+ if strings.Contains(err.Error(), "Resource not found.") {
+ t.Errorf("division with name %v does not exist", division.Name)
+ return
+ }
+
+ if strings.Contains(err.Error(), "cannot delete division because it is being used by a region") {
+ return
+ }
+
+ t.Errorf("unexpected error occured: %v", err)
+}
+
func CreateTestDivisions(t *testing.T) {
for _, division := range testData.Divisions {
resp, _, err := TOSession.CreateDivision(division)
diff --git a/traffic_ops/traffic_ops_golang/api/api.go b/traffic_ops/traffic_ops_golang/api/api.go
index 597edc9..b103fb9 100644
--- a/traffic_ops/traffic_ops_golang/api/api.go
+++ b/traffic_ops/traffic_ops_golang/api/api.go
@@ -481,7 +481,7 @@ func toCamelCase(str string) string {
}
// parses pq errors for not null constraint
-func parseNotNullConstraintError(err *pq.Error) (error, error, int) {
+func parseNotNullConstraint(err *pq.Error) (error, error, int) {
pattern := regexp.MustCompile(`null value in column "(.+)" violates not-null constraint`)
match := pattern.FindStringSubmatch(err.Message)
if match == nil {
@@ -491,7 +491,7 @@ func parseNotNullConstraintError(err *pq.Error) (error, error, int) {
}
// parses pq errors for violated foreign key constraints
-func parseNotPresentFKConstraintError(err *pq.Error) (error, error, int) {
+func parseNotPresentFKConstraint(err *pq.Error) (error, error, int) {
pattern := regexp.MustCompile(`Key \(.+\)=\(.+\) is not present in table "(.+)"`)
match := pattern.FindStringSubmatch(err.Detail)
if match == nil {
@@ -501,7 +501,7 @@ func parseNotPresentFKConstraintError(err *pq.Error) (error, error, int) {
}
// parses pq errors for uniqueness constraint violations
-func parseUniqueConstraintError(err *pq.Error) (error, error, int) {
+func parseUniqueConstraint(err *pq.Error) (error, error, int) {
pattern := regexp.MustCompile(`Key \((.+)\)=\((.+)\) already exists`)
match := pattern.FindStringSubmatch(err.Detail)
if match == nil {
@@ -510,7 +510,40 @@ func parseUniqueConstraintError(err *pq.Error) (error, error, int) {
return fmt.Errorf("%s %s already exists.", match[1], match[2]), nil, http.StatusBadRequest
}
-// ParseDBError parses pq errors for uniqueness constraint violations, and returns the (userErr, sysErr, httpCode) format expected by the API helpers.
+// parses pq errors for ON DELETE RESTRICT fk constraint violations
+//
+// Note: This method would also catch an ON UPDATE RESTRICT fk constraint,
+// but only an error message appropiate for delete is returned. Currently,
+// no API endpoint can trigger an ON UPDATE RESTRICT fk constraint since
+// no API endpoint updates the primary key of any table.
+//
+// ATM I'm not sure if there is significance in restricting either of the table
+// names that are captured in the regex to not contain any underscores.
+// This function fixes issues like #3410. If an error message needs to be made
+// for tables with underscores in particular, it should be made into an issue
+// and this function should be udated then. At the moment, there are no documented
+// issues for this case, so I won't include it.
+//
+// It may be helpful to look at constraints for api_capability, role_capability,
+// and user_role for examples.
+//
+func parseRestrictFKConstraint(err *pq.Error) (error, error, int) {
+ pattern := regexp.MustCompile(`update or delete on table "([a-z]+)" violates foreign key constraint ".+" on table "([a-z]+)"`)
+ match := pattern.FindStringSubmatch(err.Message)
+ if match == nil {
+ return nil, nil, http.StatusOK
+ }
+
+ // small heuristic for grammar
+ article := "a"
+ switch match[2][0] {
+ case 'a', 'e', 'i', 'o':
+ article = "an"
+ }
+ return fmt.Errorf("cannot delete %s because it is being used by %s %s", match[1], article, match[2]), nil, http.StatusBadRequest
+}
+
+// ParseDBError parses pq errors for database constraint violations, and returns the (userErr, sysErr, httpCode) format expected by the API helpers.
func ParseDBError(ierr error) (error, error, int) {
err, ok := ierr.(*pq.Error)
@@ -518,15 +551,19 @@ func ParseDBError(ierr error) (error, error, int) {
return nil, errors.New("database returned non pq error: " + err.Error()), http.StatusInternalServerError
}
- if usrErr, sysErr, errCode := parseNotNullConstraintError(err); errCode != http.StatusOK {
+ if usrErr, sysErr, errCode := parseNotNullConstraint(err); errCode != http.StatusOK {
+ return usrErr, sysErr, errCode
+ }
+
+ if usrErr, sysErr, errCode := parseNotPresentFKConstraint(err); errCode != http.StatusOK {
return usrErr, sysErr, errCode
}
- if usrErr, sysErr, errCode := parseNotPresentFKConstraintError(err); errCode != http.StatusOK {
+ if usrErr, sysErr, errCode := parseUniqueConstraint(err); errCode != http.StatusOK {
return usrErr, sysErr, errCode
}
- if usrErr, sysErr, errCode := parseUniqueConstraintError(err); errCode != http.StatusOK {
+ if usrErr, sysErr, errCode := parseRestrictFKConstraint(err); errCode != http.StatusOK {
return usrErr, sysErr, errCode
}
diff --git a/traffic_ops/traffic_ops_golang/api/generic_crud.go b/traffic_ops/traffic_ops_golang/api/generic_crud.go
index 5c0dd35..f059c6f 100644
--- a/traffic_ops/traffic_ops_golang/api/generic_crud.go
+++ b/traffic_ops/traffic_ops_golang/api/generic_crud.go
@@ -135,7 +135,7 @@ func GenericUpdate(val GenericUpdater) (error, error, int) {
func GenericDelete(val GenericDeleter) (error, error, int) {
result, err := val.APIInfo().Tx.NamedExec(val.DeleteQuery(), val)
if err != nil {
- return nil, errors.New("deleting " + val.GetType() + ": " + err.Error()), http.StatusInternalServerError
+ return ParseDBError(err)
}
if rowsAffected, err := result.RowsAffected(); err != nil {