From dfd049f1cc61f4527a9f49bb18339ef1fc565046 Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Wed, 6 Nov 2024 09:04:34 +0100 Subject: [PATCH] fix-NPE for custom metrics strategy at db level --- src/autoscaler/api/broker/broker_test.go | 1 - src/autoscaler/db/sqldb/binding_sqldb.go | 22 +++++++++++---- src/autoscaler/db/sqldb/binding_sqldb_test.go | 28 ++++++++++++++++++- src/autoscaler/db/sqldb/sqldb_suite_test.go | 10 +++++++ 4 files changed, 53 insertions(+), 8 deletions(-) diff --git a/src/autoscaler/api/broker/broker_test.go b/src/autoscaler/api/broker/broker_test.go index ea0b98c476..2092cb7d92 100644 --- a/src/autoscaler/api/broker/broker_test.go +++ b/src/autoscaler/api/broker/broker_test.go @@ -228,5 +228,4 @@ var _ = Describe("Broker", func() { }) }) }) - }) diff --git a/src/autoscaler/db/sqldb/binding_sqldb.go b/src/autoscaler/db/sqldb/binding_sqldb.go index 8345a1b8da..a8ba4380b6 100644 --- a/src/autoscaler/db/sqldb/binding_sqldb.go +++ b/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,14 +239,20 @@ 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) { @@ -254,8 +260,12 @@ func (bdb *BindingSQLDB) GetServiceBinding(ctx context.Context, serviceBindingId } 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 { diff --git a/src/autoscaler/db/sqldb/binding_sqldb_test.go b/src/autoscaler/db/sqldb/binding_sqldb_test.go index e509d0119b..2c03da0be0 100644 --- a/src/autoscaler/db/sqldb/binding_sqldb_test.go +++ b/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() { diff --git a/src/autoscaler/db/sqldb/sqldb_suite_test.go b/src/autoscaler/db/sqldb/sqldb_suite_test.go index 929d9fe948..38c7cc6da0 100644 --- a/src/autoscaler/db/sqldb/sqldb_suite_test.go +++ b/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 {