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

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

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



##########
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:
       Organized imports in 564d00922f

##########
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:
       Checking `len(profiles)` in 94272ce568

##########
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:
       Checking `len(cachegroups)` in 94272ce568

##########
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:
       Made comment right in 4fab50ff5e




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