GetScalingPolicy => with refactor

src/acceptance/api/api_test.go

@@ -119,8 +119,8 @@ var _ = Describe("AutoScaler Public API", func() {
 		})
 
 		It("should fail to create an invalid custom metrics submission", func() {
-			response, status := createPolicy(GenerateBindingsWithScalingPolicy("invalid-value", 0, 2, "memoryused", 30, 100))
-			Expect(string(response)).Should(ContainSubstring(`custom metrics strategy not supported`))
+			response, status := createPolicy(GenerateBindingsWithScalingPolicy("invalid-value", 1, 2, "memoryused", 30, 100))
+			Expect(string(response)).Should(ContainSubstring(`configuration.custom_metrics.metric_submission_strategy.allow_from must be one of the following: \"bound_app\", \"same_app\""}`))
 			Expect(status).To(Equal(400))
 		})
 
@@ -314,28 +314,31 @@ var _ = Describe("AutoScaler Public API", func() {
 				_, status = deletePolicy()
 				Expect(status).To(Equal(200))
 			})
-			// FIXME
 			It("should succeed to get a custom metrics strategy", func() {
 				actualPolicy, status = getPolicy()
-				Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy), "TODO: should not fail to get the policy")
+				Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy))
 				Expect(status).To(Equal(200))
 			})
 			It("should fail to update an invalid custom metrics strategy", func() {
 				expectedPolicy = GenerateBindingsWithScalingPolicy("invalid-update", 1, 2, "memoryused", 30, 100)
 				actualPolicy, status = createPolicy(expectedPolicy)
-				Expect(string(actualPolicy)).Should(MatchJSON(`{"code":"Bad Request","message":"error: custom metrics strategy not supported"}`))
+				Expect(string(actualPolicy)).Should(ContainSubstring(`configuration.custom_metrics.metric_submission_strategy.allow_from must be one of the following: \"bound_app\", \"same_app\""}`))
 				Expect(status).To(Equal(400))
 			})
 			It("should succeed to update a valid custom metrics strategy", func() {
+
 				Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy))
 				Expect(status).To(Equal(200))
 			})
 
 			When("custom metrics strategy is removed from the existing policy", func() {
 				It("should removed the custom metrics strategy and displays policy only", func() {
+					Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy), "policy and custom metrics strategy should be present")
+
+					By("updating policy without custom metrics strategy")
 					expectedPolicy = GenerateDynamicScaleOutPolicy(1, 2, "memoryused", 30)
 					actualPolicy, status = createPolicy(expectedPolicy)
-					Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy))
+					Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy), "policy should be present only")
 					Expect(status).To(Equal(200))
 				})
 			})

src/autoscaler/api/broker/broker.go

@@ -503,10 +503,13 @@ func (b *Broker) Bind(ctx context.Context, instanceID string, bindingID string,
 		logger.Error("get-binding-configuration-from-request", err)
 		return result, err
 	}
-	bindingConfiguration, err = b.validateOrGetDefaultCustomMetricsStrategy(bindingConfiguration, logger)
+	bindingConfiguration, err = bindingConfiguration.ValidateOrGetDefaultCustomMetricsStrategy(bindingConfiguration)
 	if err != nil {
-		logger.Error("validate-or-get-default-custom-metric-strategy", err)
-		return result, err
+		actionName := "validate-or-get-default-custom-metric-strategy"
+		logger.Error(actionName, err)
+		return result, apiresponses.NewFailureResponseBuilder(
+			err, http.StatusBadRequest, actionName).
+			WithErrorKey(actionName).Build()
 	}
 	policy, err := b.getPolicyFromJsonRawMessage(policyJson, instanceID, details.PlanID)
 	if err != nil {
@@ -593,21 +596,6 @@ func (b *Broker) Bind(ctx context.Context, instanceID string, bindingID string,
 	return result, nil
 }
 
-func (b *Broker) validateOrGetDefaultCustomMetricsStrategy(bindingConfiguration *models.BindingConfig, logger lager.Logger) (*models.BindingConfig, error) {
-	strategy := bindingConfiguration.GetCustomMetricsStrategy()
-	if strategy == "" {
-		bindingConfiguration.SetCustomMetricsStrategy(models.CustomMetricsSameApp)
-	} else if strategy != models.CustomMetricsBoundApp {
-		actionName := "verify-custom-metrics-strategy"
-		return bindingConfiguration, apiresponses.NewFailureResponseBuilder(
-			ErrInvalidCustomMetricsStrategy, http.StatusBadRequest, actionName).
-			WithErrorKey(actionName).
-			Build()
-	}
-	logger.Info("binding-configuration", lager.Data{"bindingConfiguration": bindingConfiguration})
-	return bindingConfiguration, nil
-}
-
 func (b *Broker) getBindingConfigurationFromRequest(policyJson json.RawMessage, logger lager.Logger) (*models.BindingConfig, error) {
 	bindingConfiguration := &models.BindingConfig{}
 	var err error
@@ -739,31 +727,16 @@ func (b *Broker) GetBinding(ctx context.Context, instanceID string, bindingID st
 		b.logger.Error("get-binding", err, lager.Data{"instanceID": instanceID, "bindingID": bindingID, "fetchBindingDetails": details})
 		return domain.GetBindingSpec{}, apiresponses.NewFailureResponse(errors.New("failed to retrieve scaling policy"), http.StatusInternalServerError, "get-policy")
 	}
-	combinedConfig, bindingConfig := b.buildConfigurationIfPresent(serviceBinding.CustomMetricsStrategy)
-	if policy != nil && combinedConfig != nil { //both are present
-		combinedConfig.ScalingPolicy = *policy
-		combinedConfig.BindingConfig = *bindingConfig
-		result.Parameters = combinedConfig
-	} else if policy != nil { // only policy was given
-		result.Parameters = policy
-	} else if combinedConfig != nil { // only configuration was given
-		result.Parameters = bindingConfig
+	if policy != nil {
+		andPolicy, err := models.DetermineBindingConfigAndPolicy(policy, serviceBinding.CustomMetricsStrategy)
+		if err != nil {
+			return domain.GetBindingSpec{}, apiresponses.NewFailureResponse(errors.New("failed to build policy and config response object"), http.StatusInternalServerError, "determine-BindingConfig-and-Policy")
+		}
+		result.Parameters = andPolicy
 	}
 	return result, nil
 }
 
-func (b *Broker) buildConfigurationIfPresent(customMetricsStrategy string) (*models.BindingConfigWithPolicy, *models.BindingConfig) {
-	var combinedConfig *models.BindingConfigWithPolicy
-	var bindingConfig *models.BindingConfig
-
-	if customMetricsStrategy != "" && customMetricsStrategy != models.CustomMetricsSameApp { //if custom metric was given in the binding process
-		combinedConfig = &models.BindingConfigWithPolicy{}
-		bindingConfig = &models.BindingConfig{}
-		bindingConfig.SetCustomMetricsStrategy(customMetricsStrategy)
-		combinedConfig.BindingConfig = *bindingConfig
-	}
-	return combinedConfig, bindingConfig
-}
 func (b *Broker) getServiceBinding(ctx context.Context, bindingID string) (*models.ServiceBinding, error) {
 	logger := b.logger.Session("get-service-binding", lager.Data{"bindingID": bindingID})
 

src/autoscaler/api/broker/broker_test.go

@@ -27,7 +27,6 @@ var _ = Describe("Broker", func() {
 		fakeCredentials          *fakes.FakeCredentials
 		testLogger               = lagertest.NewTestLogger("test")
 		bindingConfigWithScaling *models.BindingConfigWithPolicy
-		bindingConfig            *models.BindingConfig
 	)
 
 	BeforeEach(func() {
@@ -188,7 +187,7 @@ var _ = Describe("Broker", func() {
 			})
 			Context("with configuration and policy", func() {
 				BeforeEach(func() {
-					bindingConfig = &models.BindingConfig{Configuration: models.Configuration{CustomMetrics: models.CustomMetricsConfig{
+					_ = &models.BindingConfig{Configuration: models.Configuration{CustomMetrics: models.CustomMetricsConfig{
 						MetricSubmissionStrategy: models.MetricsSubmissionStrategy{AllowFrom: "bound_app"},
 					},
 					}}
@@ -208,10 +207,6 @@ var _ = Describe("Broker", func() {
 			})
 			Context("with configuration only", func() {
 				BeforeEach(func() {
-					bindingConfig = &models.BindingConfig{Configuration: models.Configuration{CustomMetrics: models.CustomMetricsConfig{
-						MetricSubmissionStrategy: models.MetricsSubmissionStrategy{AllowFrom: "bound_app"},
-					},
-					}}
 					fakeBindingDB.GetServiceBindingReturns(&models.ServiceBinding{ServiceBindingID: testBindingId,
 						ServiceInstanceID: testInstanceId, AppID: testAppId, CustomMetricsStrategy: "bound_app"}, nil)
 					bindingBytes, err := os.ReadFile("testdata/with-configs.json")
@@ -223,7 +218,7 @@ var _ = Describe("Broker", func() {
 				})
 				It("returns the bindings with configs in parameters", func() {
 					Expect(err).To(BeNil())
-					Expect(Binding).To(Equal(domain.GetBindingSpec{Parameters: bindingConfig}))
+					Expect(Binding).To(Equal(domain.GetBindingSpec{Parameters: nil}))
 				})
 			})
 		})

src/autoscaler/api/brokerserver/broker_handler_test.go

@@ -992,7 +992,7 @@ var _ = Describe("BrokerHandler", func() {
 				})
 				It("should fail with 400", func() {
 					Expect(resp.Code).To(Equal(http.StatusBadRequest))
-					Expect(resp.Body.String()).To(MatchJSON(`{"error": "verify-custom-metrics-strategy", "description": "error: custom metrics strategy not supported"}`))
+					Expect(resp.Body.String()).To(MatchJSON(`{"error": "validate-or-get-default-custom-metric-strategy", "description": "error: custom metrics strategy not supported"}`))
 
 				})
 			})

src/autoscaler/api/publicapiserver/public_api_handler.go

@@ -4,13 +4,14 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"github.com/pivotal-cf/brokerapi/v11/domain/apiresponses"
 	"io"
 	"net/http"
 	"net/url"
 	"os"
 	"reflect"
 
+	"github.com/pivotal-cf/brokerapi/v11/domain/apiresponses"
+
 	"code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/config"
 	"code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/policyvalidator"
 	"code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/schedulerclient"
@@ -109,44 +110,15 @@ func (h *PublicApiHandler) GetScalingPolicy(w http.ResponseWriter, r *http.Reque
 		writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving binding policy")
 		return
 	}
-	scalingPolicyResult, err := h.determineBindingConfigAndPolicy(scalingPolicy, customMetricStrategy)
+	scalingPolicyResult, err := models.DetermineBindingConfigAndPolicy(scalingPolicy, customMetricStrategy)
 	if err != nil {
-		logger.Error("Failed to build policy and config response object: %w", err)
+		logger.Error("Failed to build policy and config response object", err)
 		writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving binding policy")
 		return
 	}
 	handlers.WriteJSONResponse(w, http.StatusOK, scalingPolicyResult)
 }
 
-// TODO Move this centrally
-func (h *PublicApiHandler) determineBindingConfigAndPolicy(scalingPolicy *models.ScalingPolicy, customMetricStrategy string) (interface{}, error) {
-	combinedConfig, bindingConfig := h.buildConfigurationIfPresent(customMetricStrategy)
-	if scalingPolicy != nil && combinedConfig != nil { //both are present
-		combinedConfig.ScalingPolicy = *scalingPolicy
-		combinedConfig.BindingConfig = *bindingConfig
-		return combinedConfig, nil
-	} else if scalingPolicy != nil { // only policy was given
-		return scalingPolicy, nil
-	} else if combinedConfig != nil { // only configuration was given //FIXME This should not be possible
-		return bindingConfig, nil
-	}
-	return scalingPolicy, fmt.Errorf("no policy or custom metrics strategy found")
-}
-
-// TODO Move this centrally
-func (h *PublicApiHandler) buildConfigurationIfPresent(customMetricsStrategy string) (*models.BindingConfigWithPolicy, *models.BindingConfig) {
-	var combinedConfig *models.BindingConfigWithPolicy
-	var bindingConfig *models.BindingConfig
-
-	if customMetricsStrategy != "" && customMetricsStrategy != models.CustomMetricsSameApp { //if custom metric was given in the binding process
-		combinedConfig = &models.BindingConfigWithPolicy{}
-		bindingConfig = &models.BindingConfig{}
-		bindingConfig.SetCustomMetricsStrategy(customMetricsStrategy)
-		combinedConfig.BindingConfig = *bindingConfig
-	}
-	return combinedConfig, bindingConfig
-}
-
 func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Request, vars map[string]string) {
 	appId := vars["appId"]
 	if appId == "" {
@@ -171,13 +143,7 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re
 		writeErrorResponse(w, http.StatusInternalServerError, errMessage)
 		return
 	}
-	// FIXME Move this validation code in a central place within api. This is a duplicate in broker.bind
-	bindingConfiguration, err = h.validateOrGetDefaultCustomMetricsStrategy(bindingConfiguration, logger)
-	if err != nil {
-		logger.Error(ErrInvalidConfigurations.Error(), err)
-		writeErrorResponse(w, http.StatusBadRequest, err.Error())
-		return
-	}
+
 	policy, errResults := h.policyValidator.ValidatePolicy(policyBytes)
 	if errResults != nil {
 		logger.Info("Failed to validate policy", lager.Data{"errResults": errResults})
@@ -200,15 +166,23 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re
 		writeErrorResponse(w, http.StatusInternalServerError, err.Error())
 		return
 	}
-	strategy := bindingConfiguration.GetCustomMetricsStrategy()
-	err = h.bindingdb.SetOrUpdateCustomMetricStrategy(r.Context(), appId, strategy, "update")
+
+	validatedBindingConfiguration, err := bindingConfiguration.ValidateOrGetDefaultCustomMetricsStrategy(bindingConfiguration)
+	if err != nil {
+		logger.Error(ErrInvalidConfigurations.Error(), err)
+		writeErrorResponse(w, http.StatusBadRequest, err.Error())
+		return
+	}
+	strategy := validatedBindingConfiguration.GetCustomMetricsStrategy()
+	logger.Info("saving custom metric submission strategy", lager.Data{"strategy": validatedBindingConfiguration.GetCustomMetricsStrategy(), "appId": appId})
+	err = h.bindingdb.SetOrUpdateCustomMetricStrategy(r.Context(), appId, validatedBindingConfiguration.GetCustomMetricsStrategy(), "update")
 	if err != nil {
 		actionName := "failed to save custom metric submission strategy in the database"
 		logger.Error(actionName, err)
 		writeErrorResponse(w, http.StatusInternalServerError, actionName)
 		return
 	}
-	response, err := h.buildResponse(strategy, bindingConfiguration, policy)
+	response, err := h.buildResponse(strategy, validatedBindingConfiguration, policy)
 	if err != nil {
 		logger.Error("Failed to marshal policy", err)
 		writeErrorResponse(w, http.StatusInternalServerError, "Error building response")
@@ -376,21 +350,6 @@ func (h *PublicApiHandler) GetHealth(w http.ResponseWriter, _ *http.Request, _ m
 	}
 }
 
-func (h *PublicApiHandler) validateOrGetDefaultCustomMetricsStrategy(bindingConfiguration *models.BindingConfig, logger lager.Logger) (*models.BindingConfig, error) {
-	strategy := bindingConfiguration.GetCustomMetricsStrategy()
-	if strategy == "" {
-		bindingConfiguration.SetCustomMetricsStrategy(models.CustomMetricsSameApp)
-	} else if strategy != models.CustomMetricsBoundApp {
-		actionName := "verify-custom-metrics-strategy"
-		return bindingConfiguration, apiresponses.NewFailureResponseBuilder(
-			ErrInvalidCustomMetricsStrategy, http.StatusBadRequest, actionName).
-			WithErrorKey(actionName).
-			Build()
-	}
-	logger.Info("binding-configuration", lager.Data{"bindingConfiguration": bindingConfiguration})
-	return bindingConfiguration, nil
-}
-
 func (h *PublicApiHandler) getBindingConfigurationFromRequest(policyJson json.RawMessage, logger lager.Logger) (*models.BindingConfig, error) {
 	bindingConfiguration := &models.BindingConfig{}
 	var err error

src/autoscaler/db/sqldb/binding_sqldb.go

@@ -441,7 +441,6 @@ func (bdb *BindingSQLDB) GetCustomMetricStrategyByAppId(ctx context.Context, app
 }
 
 func (bdb *BindingSQLDB) SetOrUpdateCustomMetricStrategy(ctx context.Context, appId string, customMetricsStrategy string, actionName string) error {
-
 	appBinding, err := bdb.GetAppBindingByAppId(ctx, appId)
 	if err != nil {
 		return err

src/autoscaler/models/binding_configs.go

@@ -1,5 +1,10 @@
 package models
 
+import (
+	"errors"
+	"fmt"
+)
+
 // BindingConfig
 /* The configuration object received as part of the binding parameters. Example config:
 {
@@ -43,3 +48,50 @@ func (b *BindingConfig) GetCustomMetricsStrategy() string {
 func (b *BindingConfig) SetCustomMetricsStrategy(allowFrom string) {
 	b.Configuration.CustomMetrics.MetricSubmissionStrategy.AllowFrom = allowFrom
 }
+
+/**
+ * DetermineBindingConfigAndPolicy determines the binding configuration and policy based on the given parameters.
+ * It establishes the relationship between the scaling policy and the custom metrics strategy.
+ * @param scalingPolicy the scaling policy
+ * @param customMetricStrategy the custom metric strategy
+ * @return the binding configuration and policy if both are present, the scaling policy if only the policy is present,
+* 			the binding configuration if only the configuration is present
+ * @throws an error if no policy or custom metrics strategy is found
+*/
+
+func DetermineBindingConfigAndPolicy(scalingPolicy *ScalingPolicy, customMetricStrategy string) (interface{}, error) {
+	if scalingPolicy == nil {
+		return nil, fmt.Errorf("policy not found")
+	}
+
+	combinedConfig, bindingConfig := buildConfigurationIfPresent(customMetricStrategy)
+	if combinedConfig != nil { //both are present
+		combinedConfig.ScalingPolicy = *scalingPolicy
+		combinedConfig.BindingConfig = *bindingConfig
+		return combinedConfig, nil
+	}
+	return scalingPolicy, nil
+}
+
+func buildConfigurationIfPresent(customMetricsStrategy string) (*BindingConfigWithPolicy, *BindingConfig) {
+	var combinedConfig *BindingConfigWithPolicy
+	var bindingConfig *BindingConfig
+
+	if customMetricsStrategy != "" && customMetricsStrategy != CustomMetricsSameApp { //if custom metric was given in the binding process
+		combinedConfig = &BindingConfigWithPolicy{}
+		bindingConfig = &BindingConfig{}
+		bindingConfig.SetCustomMetricsStrategy(customMetricsStrategy)
+		combinedConfig.BindingConfig = *bindingConfig
+	}
+	return combinedConfig, bindingConfig
+}
+
+func (b *BindingConfig) ValidateOrGetDefaultCustomMetricsStrategy(bindingConfiguration *BindingConfig) (*BindingConfig, error) {
+	strategy := bindingConfiguration.GetCustomMetricsStrategy()
+	if strategy == "" {
+		bindingConfiguration.SetCustomMetricsStrategy(CustomMetricsSameApp)
+	} else if strategy != CustomMetricsBoundApp {
+		return nil, errors.New("error: custom metrics strategy not supported")
+	}
+	return bindingConfiguration, nil
+}