Merge pull request #3311 from cloudfoundry/fix-servicebinding-npe

fix(apiserver) Handle NPE for Custom Metrics Strategy

src/autoscaler/api/broker/broker_test.go

@@ -228,5 +228,4 @@ var _ = Describe("Broker", func() {
 			})
 		})
 	})
-
 })

src/autoscaler/db/sqldb/binding_sqldb.go

@@ -208,7 +208,7 @@ func (bdb *BindingSQLDB) CreateServiceBinding(ctx context.Context, bindingId str
 	query := bdb.sqldb.Rebind("INSERT INTO binding" +
 		"(binding_id, service_instance_id, app_id, created_at, custom_metrics_strategy) " +
 		"VALUES(?, ?, ?, ?,?)")
-	_, err = bdb.sqldb.ExecContext(ctx, query, bindingId, serviceInstanceId, appId, time.Now(), customMetricsStrategy)
+	_, err = bdb.sqldb.ExecContext(ctx, query, bindingId, serviceInstanceId, appId, time.Now(), nullableString(customMetricsStrategy))
 
 	if err != nil {
 		bdb.logger.Error("create-service-binding", err, lager.Data{"query": query, "serviceInstanceId": serviceInstanceId, "bindingId": bindingId, "appId": appId, "customMetricsStrategy": customMetricsStrategy})
@@ -239,23 +239,33 @@ func (bdb *BindingSQLDB) isBindingExists(ctx context.Context, bindingId string,
 	return nil
 }
 
+type dbServiceBinding struct {
+	ServiceBindingID      string         `db:"binding_id"`
+	ServiceInstanceID     string         `db:"service_instance_id"`
+	AppID                 string         `db:"app_id"`
+	CustomMetricsStrategy sql.NullString `db:"custom_metrics_strategy"`
+}
+
 func (bdb *BindingSQLDB) GetServiceBinding(ctx context.Context, serviceBindingId string) (*models.ServiceBinding, error) {
 	logger := bdb.logger.Session("get-service-binding", lager.Data{"serviceBindingId": serviceBindingId})
 
-	serviceBinding := &models.ServiceBinding{}
-
+	dbServiceBinding := &dbServiceBinding{}
 	query := bdb.sqldb.Rebind("SELECT binding_id, service_instance_id, app_id, custom_metrics_strategy FROM binding WHERE binding_id =?")
 
-	err := bdb.sqldb.GetContext(ctx, serviceBinding, query, serviceBindingId)
+	err := bdb.sqldb.GetContext(ctx, dbServiceBinding, query, serviceBindingId)
 	if err != nil {
 		logger.Error("query", err, lager.Data{"query": query})
 		if errors.Is(err, sql.ErrNoRows) {
 			return nil, db.ErrDoesNotExist
 		}
 		return nil, err
 	}
-
-	return serviceBinding, nil
+	return &models.ServiceBinding{
+		ServiceBindingID:      dbServiceBinding.ServiceBindingID,
+		ServiceInstanceID:     dbServiceBinding.ServiceInstanceID,
+		AppID:                 dbServiceBinding.AppID,
+		CustomMetricsStrategy: dbServiceBinding.CustomMetricsStrategy.String,
+	}, nil
 }
 
 func (bdb *BindingSQLDB) DeleteServiceBinding(ctx context.Context, bindingId string) error {

src/autoscaler/db/sqldb/binding_sqldb_test.go

@@ -406,13 +406,21 @@ var _ = Describe("BindingSqldb", func() {
 
 			When("service binding is created with invalid custom metrics strategy", func() {
 				BeforeEach(func() {
-					customMetricsStrategy = ""
+					customMetricsStrategy = "invalid"
 				})
 				It("should throw an error with foreign key violation", func() {
 					Expect(err).To(HaveOccurred())
 					Expect(err.Error()).To(ContainSubstring("foreign key constraint"))
 				})
 			})
+			When("service binding is created with empty/nil custom metrics strategy", func() {
+				BeforeEach(func() {
+					customMetricsStrategy = ""
+				})
+				It("should return custom metrics strategy as null", func() {
+					Expect(hasServiceBindingWithCustomMetricStrategyIsNull(testBindingId, testInstanceId)).To(BeTrue())
+				})
+			})
 		})
 	})
 
@@ -446,6 +454,24 @@ var _ = Describe("BindingSqldb", func() {
 				}))
 			})
 		})
+		Context("with existing custom metrics strategy is null and binding already exists", func() {
+			BeforeEach(func() {
+				customMetricsStrategy = ""
+				err = bdb.CreateServiceInstance(context.Background(), models.ServiceInstance{ServiceInstanceId: testInstanceId, OrgId: testOrgGuid, SpaceId: testSpaceGuid, DefaultPolicy: policyJsonStr, DefaultPolicyGuid: policyGuid})
+				Expect(err).NotTo(HaveOccurred())
+				err = bdb.CreateServiceBinding(context.Background(), testBindingId, testInstanceId, testAppId, "")
+				Expect(err).NotTo(HaveOccurred())
+			})
+			It("should get the custom metrics strategy as empty", func() {
+				Expect(retrievedServiceBinding).To(Equal(&models.ServiceBinding{
+					ServiceBindingID:      testBindingId,
+					ServiceInstanceID:     testInstanceId,
+					AppID:                 testAppId,
+					CustomMetricsStrategy: "",
+				}))
+				Expect(hasServiceBindingWithCustomMetricStrategyIsNull(testBindingId, testInstanceId)).To(BeTrue())
+			})
+		})
 	})
 
 	Describe("DeleteServiceBinding", func() {

src/autoscaler/db/sqldb/sqldb_suite_test.go

@@ -127,6 +127,16 @@ func hasServiceBindingWithCustomMetricStrategy(bindingId string, serviceInstance
 	return item
 }
 
+func hasServiceBindingWithCustomMetricStrategyIsNull(bindingId string, serviceInstanceId string) bool {
+	query := dbHelper.Rebind("SELECT * FROM binding WHERE binding_id = ? AND service_instance_id = ? AND custom_metrics_strategy is NULL")
+	rows, e := dbHelper.Query(query, bindingId, serviceInstanceId)
+	FailOnError("can not query table binding", e)
+	defer func() { _ = rows.Close() }()
+	item := rows.Next()
+	FailOnError("can not query table binding", rows.Err())
+	return item
+}
+
 func cleanPolicyTable() {
 	_, e := dbHelper.Exec("DELETE from policy_json")
 	if e != nil {