[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5304: Validate Topology Cache Group non-emptiness on Delivery Services's CDN

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

rawlinp commented on a change in pull request #5304:
URL: https://github.com/apache/trafficcontrol/pull/5304#discussion_r527184669



##########
File path: traffic_ops/testing/api/v3/deliveryservices_test.go
##########
@@ -603,6 +603,110 @@ func UpdateDeliveryServiceWithInvalidTopology(t *testing.T) {
 	if err != nil {
 		t.Errorf("updating DS topology - expected: no error, actual: %v", err)
 	}
+
+	const xmlId = "top-ds-in-cdn2"
+	dses, _, err = TOSession.GetDeliveryServicesV30WithHdr(nil, url.Values{"xmlId": {xmlId}})
+	if err != nil {
+		t.Fatalf("getting Delivery Service %s: %s", xmlId, err.Error())
+	}
+	const expectedSize = 1
+	if len(dses) != expectedSize {
+		t.Fatalf("expected %d Delivery Service with xmlId %s but instead received %d Delivery Services", expectedSize, xmlId, len(dses))
+	}
+	ds = dses[0]
+	dsTopology := ds.Topology
+	ds.Topology = nil
+	ds, _, err = TOSession.UpdateDeliveryServiceV30WithHdr(*ds.ID, ds, nil)
+	if err != nil {
+		t.Fatalf("updating Delivery Service %s: %s", xmlId, err.Error())
+	}
+	const cdn1Name = "cdn1"
+	cdns, _, err := TOSession.GetCDNByNameWithHdr(cdn1Name, nil)
+	if err != nil {
+		t.Fatalf("getting CDN %s: %s", cdn1Name, err.Error())
+	}
+	if len(cdns) != expectedSize {
+		t.Fatalf("expected %d CDN with name %s but instead received %d CDNs", expectedSize, cdn1Name, len(cdns))
+	}
+	cdn1 := cdns[0]
+	const cacheGroupName = "dtrc1"
+	cachegroups, _, err := TOSession.GetCacheGroupsByQueryParamsWithHdr(url.Values{"name": {cacheGroupName}}, nil)
+	if err != nil {
+		t.Fatalf("getting Cache Group %s: %s", cacheGroupName, err.Error())
+	}
+	if len(cdns) != expectedSize {
+		t.Fatalf("expected %d Cache Group with name %s but instead received %d Cache Groups", expectedSize, cacheGroupName, len(cachegroups))
+	}
+	cachegroup := cachegroups[0]
+	params = url.Values{"cdn": {strconv.Itoa(*ds.CDNID)}, "cachegroup": {strconv.Itoa(*cachegroup.ID)}}
+	servers, _, err := TOSession.GetServersWithHdr(&params, nil)
+	if err != nil {
+		t.Fatalf("getting Server with params %v: %s", params, err.Error())
+	}
+	if len(servers.Response) != expectedSize {
+		t.Fatalf("expected %d Server returned for query params %v but instead received %d Servers", expectedSize, params, len(servers.Response))
+	}
+	server := servers.Response[0]
+	*server.CDNID = cdn1.ID
+
+	// A profile specific to CDN 1 is required
+	profileCopy := tc.ProfileCopy{
+		Name:         *server.Profile + "_BUT_IN_CDN1",
+		ExistingID:   *server.ProfileID,
+		ExistingName: *server.Profile,
+		Description:  *server.ProfileDesc,
+	}
+	_, _, err = TOSession.CopyProfile(profileCopy)
+	if err != nil {
+		t.Fatalf("copying Profile %s: %s", *server.Profile, err.Error())
+	}
+
+	profiles, _, err := TOSession.GetProfileByNameWithHdr(profileCopy.Name, nil)
+	if err != nil {
+		t.Fatalf("getting Profile %s: %s", profileCopy.Name, err.Error())
+	}
+	if len(servers.Response) != expectedSize {

Review comment:
       I think you meant to check `len(profiles)` here

##########
File path: traffic_ops/testing/api/v3/deliveryservices_test.go
##########
@@ -603,6 +603,110 @@ func UpdateDeliveryServiceWithInvalidTopology(t *testing.T) {
 	if err != nil {
 		t.Errorf("updating DS topology - expected: no error, actual: %v", err)
 	}
+
+	const xmlId = "top-ds-in-cdn2"
+	dses, _, err = TOSession.GetDeliveryServicesV30WithHdr(nil, url.Values{"xmlId": {xmlId}})
+	if err != nil {
+		t.Fatalf("getting Delivery Service %s: %s", xmlId, err.Error())
+	}
+	const expectedSize = 1
+	if len(dses) != expectedSize {
+		t.Fatalf("expected %d Delivery Service with xmlId %s but instead received %d Delivery Services", expectedSize, xmlId, len(dses))
+	}
+	ds = dses[0]
+	dsTopology := ds.Topology
+	ds.Topology = nil
+	ds, _, err = TOSession.UpdateDeliveryServiceV30WithHdr(*ds.ID, ds, nil)
+	if err != nil {
+		t.Fatalf("updating Delivery Service %s: %s", xmlId, err.Error())
+	}
+	const cdn1Name = "cdn1"
+	cdns, _, err := TOSession.GetCDNByNameWithHdr(cdn1Name, nil)
+	if err != nil {
+		t.Fatalf("getting CDN %s: %s", cdn1Name, err.Error())
+	}
+	if len(cdns) != expectedSize {
+		t.Fatalf("expected %d CDN with name %s but instead received %d CDNs", expectedSize, cdn1Name, len(cdns))
+	}
+	cdn1 := cdns[0]
+	const cacheGroupName = "dtrc1"
+	cachegroups, _, err := TOSession.GetCacheGroupsByQueryParamsWithHdr(url.Values{"name": {cacheGroupName}}, nil)
+	if err != nil {
+		t.Fatalf("getting Cache Group %s: %s", cacheGroupName, err.Error())
+	}
+	if len(cdns) != expectedSize {

Review comment:
       I think you meant to check `len(cachegroups)` here

##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -839,18 +842,50 @@ func TopologyExists(tx *sql.Tx, name string) (bool, error) {
 	return count > 0, err
 }
 
-// GetTopologyCachegroups returns the set of cachegroup names for the given topology.
-func GetTopologyCachegroups(tx *sql.Tx, name string) ([]string, error) {
+// CheckTopology returns an error if the given Topology does not exist or if one of the Topology's Cache Groups is
+// empty with respect to the Delivery Service's CDN.
+func CheckTopology(tx *sqlx.Tx, ds tc.DeliveryServiceNullableV30) (int, error, error) {
+	statusCode, userErr, sysErr := http.StatusOK, error(nil), error(nil)
+
+	if ds.Topology == nil {
+		return statusCode, userErr, sysErr
+	}
+
+	cacheGroupIDs, _, err := GetTopologyCachegroups(tx.Tx, *ds.Topology)
+	if err != nil {
+		return http.StatusInternalServerError, nil, fmt.Errorf("getting topology cachegroups: %v", err)
+	}
+	if len(cacheGroupIDs) == 0 {
+		return http.StatusBadRequest, fmt.Errorf("no such Topology '%s'", *ds.Topology), nil
+	}
+
+	if err = topology_validation.CheckForEmptyCacheGroups(tx, cacheGroupIDs, []int{*ds.CDNID}, true, []int{}); err != nil {
+		return http.StatusBadRequest, fmt.Errorf("empty cachegroups in Topology %s found for CDN %d: %s", *ds.Topology, ds.CDNID, err.Error()), nil
+	}
+
+	return statusCode, userErr, sysErr
+}
+
+// GetTopologyCachegroups returns a map of cachegroup names by cachegroup id for the given topology.

Review comment:
       nit: this comment is wrong

##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -23,6 +23,9 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
+	"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/topology/topology_validation"

Review comment:
       nit: these imports are grouped haphazardly




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