[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #5605: Added the ability to change a server capability name

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

srijeet0406 commented on a change in pull request #5605:
URL: https://github.com/apache/trafficcontrol/pull/5605#discussion_r588539667



##########
File path: traffic_ops/app/db/migrations/2021030300000000_server_capability_update_cascade.sql
##########
@@ -0,0 +1,28 @@
+/*
+    Licensed under the Apache License, Version 2.0 (the "License");
+    you may not use this file except in compliance with the License.
+    You may obtain a copy of the License at
+        http://www.apache.org/licenses/LICENSE-2.0
+    Unless required by applicable law or agreed to in writing, software
+    distributed under the License is distributed on an "AS IS" BASIS,
+    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+    See the License for the specific language governing permissions and
+    limitations under the License.
+*/
+
+-- +goose Up
+-- SQL in section 'Up' is executed when this migration is applied
+ALTER TABLE public.server_server_capability DROP CONSTRAINT fk_server_capability;

Review comment:
       Do you need the `public` prefix? 

##########
File path: traffic_ops/testing/api/v4/servercapabilities_test.go
##########
@@ -89,6 +90,41 @@ func ValidationTestServerCapabilities(t *testing.T) {
 	}
 }
 
+func UpdateTestServerCapabilities(t *testing.T) {
+	var header http.Header
+
+	// Get server capability name and edit it to a new name
+	resp, _, err := TOSession.GetServerCapabilitiesWithHdr(header)
+	if err != nil {

Review comment:
       Could you add a check for `len(resp) != 1` here, so that we can avoid chances of a panic on line 101/ 103?

##########
File path: traffic_ops/testing/api/v4/serverservercapability_test.go
##########
@@ -222,6 +223,43 @@ func GetTestServerServerCapabilities(t *testing.T) {
 	}
 }
 
+func UpdateTestServerServerCapabilities(t *testing.T) {
+	var header http.Header
+
+	// Get server capability name and edit it to a new name
+	resp, _, err := TOSession.GetServerCapabilitiesWithHdr(header)
+	if err != nil {

Review comment:
       Same comment about `len(resp)` here.

##########
File path: traffic_ops/app/db/migrations/2021030300000000_server_capability_update_cascade.sql
##########
@@ -0,0 +1,28 @@
+/*
+    Licensed under the Apache License, Version 2.0 (the "License");
+    you may not use this file except in compliance with the License.
+    You may obtain a copy of the License at
+        http://www.apache.org/licenses/LICENSE-2.0
+    Unless required by applicable law or agreed to in writing, software
+    distributed under the License is distributed on an "AS IS" BASIS,
+    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+    See the License for the specific language governing permissions and
+    limitations under the License.
+*/
+
+-- +goose Up
+-- SQL in section 'Up' is executed when this migration is applied
+ALTER TABLE public.server_server_capability DROP CONSTRAINT fk_server_capability;
+ALTER TABLE public.deliveryservices_required_capability DROP CONSTRAINT fk_required_capability;
+ALTER TABLE public.server_server_capability ADD CONSTRAINT fk_server_capability FOREIGN KEY (server_capability) REFERENCES server_capability(name) ON UPDATE CASCADE ON DELETE RESTRICT;
+ALTER TABLE public.deliveryservices_required_capability ADD CONSTRAINT fk_required_capability FOREIGN KEY (required_capability) REFERENCES server_capability(name) ON UPDATE CASCADE ON DELETE RESTRICT;
+
+-- +goose Down
+-- SQL section 'Down' is executed when this migration is rolled back
+ALTER TABLE public.server_server_capability DROP CONSTRAINT fk_server_capability;
+ALTER TABLE public.deliveryservices_required_capability DROP CONSTRAINT fk_required_capability;
+ALTER TABLE public.server_server_capability ADD CONSTRAINT fk_server_capability FOREIGN KEY (server_capability) REFERENCES server_capability(name) ON DELETE RESTRICT;
+ALTER TABLE public.deliveryservices_required_capability ADD CONSTRAINT fk_required_capability FOREIGN KEY (required_capability) REFERENCES server_capability(name) ON DELETE RESTRICT;
+
+

Review comment:
       There are 2 extra blank lines at the end.

##########
File path: traffic_ops/traffic_ops_golang/servercapability/servercapability.go
##########
@@ -105,6 +119,29 @@ func (v *TOServerCapability) Read(h http.Header, useIMS bool) ([]interface{}, er
 	api.DefaultSort(v.APIInfo(), "name")
 	return api.GenericRead(h, v, useIMS)
 }
+
+func (v *TOServerCapability) Update(h http.Header) (error, error, int) {
+	sc, userErr, sysErr, errCode, _ := v.Read(h, false)
+	if userErr != nil || sysErr != nil {
+		return userErr, sysErr, errCode
+	}
+	if len(sc) != 1 {
+		return fmt.Errorf("cannot find exactly one server capability with the query string provided"), nil, http.StatusBadRequest
+	}
+	rows, err := v.ReqInfo.Tx.Query(v.updateQuery(), v.RequestedName, v.Name)
+	if err != nil {
+		return nil, fmt.Errorf("server capability update: error setting the name for server capability %v: %v", v.Name, err.Error()), http.StatusInternalServerError
+	}
+	defer log.Close(rows, "unable to close DB connection")

Review comment:
       You need to add checks for if the capability was already modified by someone else before you updated it. Look at 
   https://github.com/apache/trafficcontrol/blob/ae2ced905d223789c818831ead1c6979c488eaf0/traffic_ops/traffic_ops_golang/origin/origins.go#L303
   to
   https://github.com/apache/trafficcontrol/blob/ae2ced905d223789c818831ead1c6979c488eaf0/traffic_ops/traffic_ops_golang/origin/origins.go#L313 

##########
File path: traffic_ops/testing/api/v4/servercapabilities_test.go
##########
@@ -89,6 +90,41 @@ func ValidationTestServerCapabilities(t *testing.T) {
 	}
 }
 
+func UpdateTestServerCapabilities(t *testing.T) {
+	var header http.Header
+
+	// Get server capability name and edit it to a new name
+	resp, _, err := TOSession.GetServerCapabilitiesWithHdr(header)
+	if err != nil {
+		t.Fatalf("Expected no error, but got %v", err.Error())
+	}
+	origName := resp[0].Name
+	newSCName := "sc-test"
+	resp[0].Name = newSCName
+
+	// Update server capability with new name
+	updateResponse, _, err := TOSession.UpdateServerCapabilityByName(origName, &resp[0])
+	if err != nil {
+		t.Errorf("cannot PUT server capability: %v - %v", err, updateResponse)
+	}
+
+	// Get updated name
+	getResp, _, err := TOSession.GetServerCapabilityWithHdr(newSCName, header)
+	if err != nil {

Review comment:
       Could you add a nil check for `getResp` here?

##########
File path: traffic_ops/testing/api/v4/serverservercapability_test.go
##########
@@ -222,6 +223,43 @@ func GetTestServerServerCapabilities(t *testing.T) {
 	}
 }
 
+func UpdateTestServerServerCapabilities(t *testing.T) {
+	var header http.Header
+
+	// Get server capability name and edit it to a new name
+	resp, _, err := TOSession.GetServerCapabilitiesWithHdr(header)
+	if err != nil {
+		t.Fatalf("Expected no error, but got %v", err.Error())
+	}
+	origName := resp[0].Name
+	newSCName := "sc-test"
+	resp[0].Name = newSCName
+
+	// Update server capability with new name
+	updateResponse, _, err := TOSession.UpdateServerCapabilityByName(origName, &resp[0])
+	if err != nil {
+		t.Errorf("cannot PUT server capability: %v - %v", err, updateResponse)
+	}
+
+	//To check whether the primary key change trickled down to server table
+	ssc, _, err := TOSession.GetServerServerCapabilitiesWithHdr(nil, nil, &newSCName, nil)
+	if err != nil {
+		t.Fatalf("cannot GET server capabilities assigned to servers by server capability name %v: %v", newSCName, err)
+	}
+	for i, s := range ssc {
+		if *s.ServerCapability != *ssc[i].ServerCapability {

Review comment:
       This will always be same. Basically, you're just comparing the element with what you get when you get the element by its index. IMO, the test should be like this:
   1.) You already have a list of servers with assigned server capabilities; Keep a track of which servers are assigned to the capability in question
   2.) Update the name of this server capability
   3.) Get the list of servers associated with this (updated)capability again
   4.) Make sure the list in 1 matches the list in 3; If not, throw error.
    
   Also, missing nil checks before dereferencing pointers.




----------------------------------------------------------------
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.

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