This is an automated email from the ASF dual-hosted git repository.
zrhoffman 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 b491e85 Updating a non existent DS should return a 404, instead of a
500 (#5392)
b491e85 is described below
commit b491e85dac7bf8af404bfe947b30249cd36bfaff
Author: Srijeet Chatterjee <[email protected]>
AuthorDate: Mon Jan 4 12:18:31 2021 -0700
Updating a non existent DS should return a 404, instead of a 500 (#5392)
* Updating a non existent DS should return a 404, instead of a 500
* Changelog modification
* code review fixes
---
CHANGELOG.md | 1 +
.../deliveryservice/deliveryservices.go | 28 +++++++++++++---------
.../deliveryservice/deliveryservices_test.go | 27 +++++++++++++++++++++
3 files changed, 45 insertions(+), 11 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1b7ece9..ca7e3be 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@ The format is based on [Keep a
Changelog](http://keepachangelog.com/en/1.0.0/).
- Traffic Ops: Added validation to ensure that the cachegroups of a delivery
services' assigned ORG servers are present in the topology
### Fixed
+- [#5378](https://github.com/apache/trafficcontrol/issues/5378) - Updating a
non existent DS should return a 404, instead of a 500
- [#5380](https://github.com/apache/trafficcontrol/issues/5380) - Show the
correct servers (including ORGs) when a topology based DS with required
capabilities + ORG servers is queried for the assigned servers
- [#5195](https://github.com/apache/trafficcontrol/issues/5195) - Correctly
show CDN ID in Changelog during Snap
- Fixed Traffic Router logging unnecessary warnings for IPv6-only caches
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
index 1d9a89d..da2b559 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
@@ -848,10 +848,13 @@ func updateV31(w http.ResponseWriter, r *http.Request,
inf *api.APIInfo, dsV31 *
oldHostName := ""
oldCDNName := ""
oldRoutingName := ""
+ errCode := http.StatusOK
+ var userErr error
+ var sysErr error
if dsType.HasSSLKeys() {
- oldHostName, oldCDNName, oldRoutingName, err =
getOldDetails(*ds.ID, tx)
- if err != nil {
- return nil, http.StatusInternalServerError, nil,
errors.New("getting existing delivery service hostname: " + err.Error())
+ oldHostName, oldCDNName, oldRoutingName, userErr, sysErr,
errCode = getOldDetails(*ds.ID, tx)
+ if userErr != nil || sysErr != nil {
+ return nil, errCode, userErr, sysErr
}
}
@@ -861,7 +864,7 @@ func updateV31(w http.ResponseWriter, r *http.Request, inf
*api.APIInfo, dsV31 *
deepCachingType = ds.DeepCachingType.String() // necessary,
because DeepCachingType's default needs to insert the string, not "", and Query
doesn't call .String().
}
- userErr, sysErr, errCode := api.CheckIfUnModified(r.Header, inf.Tx,
*ds.ID, "deliveryservice")
+ userErr, sysErr, errCode = api.CheckIfUnModified(r.Header, inf.Tx,
*ds.ID, "deliveryservice")
if userErr != nil || sysErr != nil {
return nil, errCode, userErr, sysErr
}
@@ -1179,7 +1182,7 @@ func selectMaxLastUpdatedQuery(where string) string {
select max(last_updated) as t from last_deleted l where
l.table_name='deliveryservice') as res`
}
-func getOldDetails(id int, tx *sql.Tx) (string, string, string, error) {
+func getOldDetails(id int, tx *sql.Tx) (string, string, string, error, error,
int) {
q := `
SELECT ds.xml_id, ds.protocol, type.name, ds.routing_name, cdn.domain_name,
cdn.name
FROM deliveryservice as ds
@@ -1194,25 +1197,28 @@ WHERE ds.id=$1
cdnDomain := ""
cdnName := ""
if err := tx.QueryRow(q, id).Scan(&xmlID, &protocol, &dsTypeStr,
&routingName, &cdnDomain, &cdnName); err != nil {
- return "", "", "", fmt.Errorf("querying delivery service %v
host name: "+err.Error()+"\n", id)
+ if err == sql.ErrNoRows {
+ return "", "", "", fmt.Errorf("querying delivery
service %v host name: no such delivery service exists", id), nil,
http.StatusNotFound
+ }
+ return "", "", "", nil, fmt.Errorf("querying delivery service
%v host name: "+err.Error(), id), http.StatusInternalServerError
}
dsType := tc.DSTypeFromString(dsTypeStr)
if dsType == tc.DSTypeInvalid {
- return "", "", "", errors.New("getting delivery services
matchlist: got invalid delivery service type '" + dsTypeStr + "'")
+ return "", "", "", errors.New("getting delivery services
matchlist: got invalid delivery service type '" + dsTypeStr + "'"), nil,
http.StatusBadRequest
}
matchLists, err := GetDeliveryServicesMatchLists([]string{xmlID}, tx)
if err != nil {
- return "", "", "", errors.New("getting delivery services
matchlist: " + err.Error())
+ return "", "", "", nil, errors.New("getting delivery services
matchlist: " + err.Error()), http.StatusInternalServerError
}
matchList, ok := matchLists[xmlID]
if !ok {
- return "", "", "", errors.New("delivery service has no match
lists (is your delivery service missing regexes?)")
+ return "", "", "", errors.New("delivery service has no match
lists (is your delivery service missing regexes?)"), nil, http.StatusBadRequest
}
host, err := getHostName(protocol, dsType, routingName, matchList,
cdnDomain) // protocol defaults to 0: doesn't need to check Valid()
if err != nil {
- return "", "", "", errors.New("getting hostname: " +
err.Error())
+ return "", "", "", nil, errors.New("getting hostname: " +
err.Error()), http.StatusInternalServerError
}
- return host, cdnName, routingName, nil
+ return host, cdnName, routingName, nil, nil, http.StatusOK
}
func getTypeFromID(id int, tx *sql.Tx) (tc.DSType, error) {
diff --git
a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go
b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go
index 8394265..d89c043 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go
@@ -20,6 +20,7 @@ package deliveryservice
*/
import (
+ "net/http"
"reflect"
"testing"
@@ -30,6 +31,32 @@ import (
"gopkg.in/DATA-DOG/go-sqlmock.v1"
)
+func TestGetOldDetails(t *testing.T) {
+ expected := `querying delivery service 1 host name: no such delivery
service exists`
+ mockDB, mock, err := sqlmock.New()
+ if err != nil {
+ t.Fatalf("an error '%s' was not expected when opening a stub
database connection", err)
+ }
+ defer mockDB.Close()
+
+ db := sqlx.NewDb(mockDB, "sqlmock")
+ defer db.Close()
+
+ rows := sqlmock.NewRows([]string{""})
+ mock.ExpectBegin()
+ mock.ExpectQuery("SELECT ds.xml_id, ds.protocol, type.name,
ds.routing_name, cdn.domain_name, cdn.name").WillReturnRows(rows)
+ _, _, _, userErr, _, code := getOldDetails(1, db.MustBegin().Tx)
+ if userErr == nil {
+ t.Fatalf("expected error %v, but got none", expected)
+ }
+ if userErr.Error() != expected {
+ t.Errorf("expected error %v, but got %v", expected,
userErr.Error())
+ }
+ if code != http.StatusNotFound {
+ t.Errorf("expected error code : %d, but got : %d",
http.StatusNotFound, code)
+ }
+}
+
func TestGetDeliveryServicesMatchLists(t *testing.T) {
// test to make sure that the DS matchlists query orders by set_number
mockDB, mock, err := sqlmock.New()
