Remove unnecessary JOIN (#5184)

* Remove unnecessary JOIN

* Fix tests, also some context leaks

* Add to CHANGELOG

(cherry picked from commit 330ac87)

CHANGELOG.md

@@ -109,6 +109,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Fixed #5102 - Python client scripts fail silently on authentication failures
 - Fixed #5103 - Python client scripts crash on connection errors
 - Fixed matching of wildcards in subjectAlternateNames when loading TLS certificates
+- Fixed #5180 - Global Max Mbps and Tps is not send to TM
+- Fixed #3528 - Fix Traffic Ops monitoring.json missing DeliveryServices
 
 ### Changed
 - Changed some Traffic Ops Go Client methods to use `DeliveryServiceNullable` inputs and outputs.

traffic_ops/traffic_ops_golang/monitoring/monitoring.go

@@ -1,3 +1,5 @@
+// Package monitoring contains handlers and supporting logic for the
+// /cdns/{{CDN Name}}/configs/monitoring Traffic Ops API endpoint.
 package monitoring
 
 /*
@@ -27,8 +29,8 @@ import (
 	"strings"
 
 	"github.com/apache/trafficcontrol/lib/go-log"
-
 	"github.com/apache/trafficcontrol/lib/go-tc"
+
 	"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
 
 	"github.com/lib/pq"
@@ -159,7 +161,7 @@ func GetMonitoringJSON(tx *sql.Tx, cdnName string) (*Monitoring, error) {
 		return nil, fmt.Errorf("error getting profiles: %v", err)
 	}
 
-	deliveryServices, err := getDeliveryServices(tx, routers)
+	deliveryServices, err := getDeliveryServices(tx)
 	if err != nil {
 		return nil, fmt.Errorf("error getting deliveryservices: %v", err)
 	}
@@ -201,25 +203,25 @@ WHERE cdn.name = $1
 `
 
 	interfacesQuery := `
-SELECT 
+SELECT
    i.name, i.max_bandwidth, i.mtu, i.monitor, i.server
 FROM interface i
 WHERE i.server in (
-	SELECT 
-		s.id 
-	FROM "server" s 
-	JOIN cdn c 
-		on c.id = s.cdn_id 
+	SELECT
+		s.id
+	FROM "server" s
+	JOIN cdn c
+		on c.id = s.cdn_id
 	WHERE c.name = $1
 )`
 
 	ipAddressQuery := `
-SELECT 
+SELECT
 	ip.address, ip.gateway, ip.service_address, ip.server, ip.interface
 FROM ip_address ip
-JOIN server s 
+JOIN server s
 	ON s.id = ip.server
-JOIN cdn cdn 
+JOIN cdn cdn
 	ON cdn.id = s.cdn_id
 WHERE ip.server = ANY($1)
 AND ip.interface = ANY($2)
@@ -452,20 +454,13 @@ WHERE pr.config_file = $2;
 	return profilesArr, nil
 }
 
-func getDeliveryServices(tx *sql.Tx, routers []Router) ([]DeliveryService, error) {
-	profileNames := []string{}
-	for _, router := range routers {
-		profileNames = append(profileNames, router.Profile)
-	}
-
+func getDeliveryServices(tx *sql.Tx) ([]DeliveryService, error) {
 	query := `
-SELECT ds.xml_id, ds.global_max_tps, ds.global_max_mbps
-FROM deliveryservice ds
-JOIN profile profile ON profile.id = ds.profile
-WHERE profile.name = ANY($1)
-AND ds.active = true
-`
-	rows, err := tx.Query(query, pq.Array(profileNames))
+	SELECT ds.xml_id, ds.global_max_tps, ds.global_max_mbps
+	FROM deliveryservice ds
+	WHERE ds.active = true
+	`
+	rows, err := tx.Query(query)
 	if err != nil {
 		return nil, err
 	}

traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go

@@ -8,14 +8,14 @@ import (
 	"testing"
 	"time"
 
-	"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
-	"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/parameter"
-	"github.com/lib/pq"
-
 	"github.com/apache/trafficcontrol/lib/go-tc"
 	"github.com/apache/trafficcontrol/lib/go-util"
 
+	"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
+	"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/parameter"
+
 	"github.com/jmoiron/sqlx"
+	"github.com/lib/pq"
 	"gopkg.in/DATA-DOG/go-sqlmock.v1"
 )
 
@@ -66,7 +66,8 @@ func TestGetMonitoringServers(t *testing.T) {
 	mock.ExpectBegin()
 	setupMockGetMonitoringServers(mock, monitor, router, []Cache{cache, otherCache, cache3}, []uint64{cacheID, otherCacheID, cache3ID}, cdn)
 
-	dbCtx, _ := context.WithTimeout(context.Background(), time.Duration(10)*time.Second)
+	dbCtx, f := context.WithTimeout(context.Background(), time.Duration(10)*time.Second)
+	defer f()
 	tx, err := db.BeginTx(dbCtx, nil)
 	if err != nil {
 		t.Fatalf("creating transaction: %v", err)
@@ -133,7 +134,8 @@ func TestGetProfileWithParams(t *testing.T) {
 	mockGetParams(mock, expected, cdn)
 	mock.ExpectCommit()
 
-	dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	defer f()
 	tx, err := db.BeginTx(dbCtx, nil)
 	if err != nil {
 		t.Fatalf("creating transaction: %v", err)
@@ -182,7 +184,8 @@ func TestGetCachegroups(t *testing.T) {
 
 	mock.ExpectQuery("SELECT").WithArgs(cdn).WillReturnRows(rows)
 
-	dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	defer f()
 	tx, err := db.BeginTx(dbCtx, nil)
 	if err != nil {
 		t.Fatalf("creating transaction: %v", err)
@@ -253,7 +256,8 @@ func TestGetProfiles(t *testing.T) {
 
 	mock.ExpectQuery("SELECT").WithArgs(pq.Array(profileNames), CacheMonitorConfigFile).WillReturnRows(rows)
 
-	dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	defer f()
 	tx, err := db.BeginTx(dbCtx, nil)
 	if err != nil {
 		t.Fatalf("creating transaction: %v", err)
@@ -301,11 +305,6 @@ func TestGetDeliveryServices(t *testing.T) {
 	db := sqlx.NewDb(mockDB, "sqlmock")
 	defer db.Close()
 
-	router := Router{
-		Type:    RouterType,
-		Profile: "routerProfile",
-	}
-
 	deliveryservice := DeliveryService{
 		XMLID:              "myDsid",
 		TotalTPSThreshold:  42.42,
@@ -314,31 +313,29 @@ func TestGetDeliveryServices(t *testing.T) {
 	}
 
 	deliveryservices := []DeliveryService{deliveryservice}
-	routers := []Router{router}
 
 	mock.ExpectBegin()
 	rows := sqlmock.NewRows([]string{"xml_id", "global_max_tps", "global_max_mbps"})
 	for _, deliveryservice := range deliveryservices {
 		rows = rows.AddRow(deliveryservice.XMLID, deliveryservice.TotalTPSThreshold, deliveryservice.TotalKBPSThreshold/KilobitsPerMegabit)
 	}
 
-	profileNames := []string{router.Profile}
-
-	mock.ExpectQuery("SELECT").WithArgs(pq.Array(profileNames)).WillReturnRows(rows)
+	mock.ExpectQuery("SELECT").WillReturnRows(rows)
 
-	dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	defer f()
 	tx, err := db.BeginTx(dbCtx, nil)
 	if err != nil {
 		t.Fatalf("creating transaction: %v", err)
 	}
 
-	sqlDeliveryservices, err := getDeliveryServices(tx, routers)
+	sqlDeliveryservices, err := getDeliveryServices(tx)
 	if err != nil {
-		t.Errorf("getProfiles expected: nil error, actual: %v", err)
+		t.Errorf("getDeliveryServices expected: nil error, actual: %v", err)
 	}
 
 	if len(deliveryservices) != len(sqlDeliveryservices) {
-		t.Errorf("getProfiles expected: %+v actual: %+v", deliveryservices, sqlDeliveryservices)
+		t.Fatalf("getDeliveryServices expected: %+v actual: %+v", deliveryservices, sqlDeliveryservices)
 	}
 
 	for i, sqlDeliveryservice := range sqlDeliveryservices {
@@ -376,7 +373,8 @@ func TestGetConfig(t *testing.T) {
 
 	mock.ExpectQuery("SELECT").WithArgs(tc.MonitorProfilePrefix+"%%", MonitorConfigFile, "mycdn").WillReturnRows(rows)
 
-	dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	defer f()
 	tx, err := db.BeginTx(dbCtx, nil)
 	if err != nil {
 		t.Fatalf("creating transaction: %v", err)
@@ -482,7 +480,6 @@ func TestGetMonitoringJSON(t *testing.T) {
 		//
 		// getDeliveryServices
 		//
-		router := createMockRouter()
 
 		deliveryservice := DeliveryService{
 			XMLID:              "myDsid",
@@ -499,9 +496,7 @@ func TestGetMonitoringJSON(t *testing.T) {
 			rows = rows.AddRow(deliveryservice.XMLID, deliveryservice.TotalTPSThreshold, deliveryservice.TotalKBPSThreshold/KilobitsPerMegabit)
 		}
 
-		profileNames := []string{router.Profile}
-
-		mock.ExpectQuery("SELECT").WithArgs(pq.Array(profileNames)).WillReturnRows(rows)
+		mock.ExpectQuery("SELECT").WillReturnRows(rows)
 		resp.Response.DeliveryServices = deliveryservices
 	}
 	{
@@ -522,15 +517,16 @@ func TestGetMonitoringJSON(t *testing.T) {
 		resp.Response.Config = config
 	}
 
-	dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second)
+	defer f()
 	tx, err := db.BeginTx(dbCtx, nil)
 	if err != nil {
 		t.Fatalf("creating transaction: %v", err)
 	}
 
 	sqlResp, err := GetMonitoringJSON(tx, cdn)
 	if err != nil {
-		t.Errorf("GetMonitoringJSON expected: nil error, actual: %v", err)
+		t.Fatalf("GetMonitoringJSON expected: nil error, actual: %v", err)
 	}
 	for _, cache := range resp.Response.TrafficServers {
 		cache.Interfaces = sortInterfaces(cache.Interfaces)