support labels with empty value (#722)

* support labels with empty value * support labels with empty value * support labels with empty value * support labels with empty value * support labels with empty value * support labels with empty value * support labels with empty value
Peripli · Nov 28, 2021 · 53c01a9 · 53c01a9
1 parent 7114f8d
commit 53c01a9
Show file tree

Hide file tree

Showing 10 changed files with 155 additions and 21 deletions.
diff --git a/pkg/query/update.go b/pkg/query/update.go
@@ -51,18 +51,11 @@ func ApplyLabelChangesToLabels(changes types.LabelChanges, labels types.Labels)
 		case types.AddLabelOperation:
 			fallthrough
 		case types.AddLabelValuesOperation:
+			if len(change.Values) == 0 {
+				addValue(mergedLabels, change, "", labelsToAdd)
+			}
 			for _, value := range change.Values {
-				found := false
-				for _, currentValue := range mergedLabels[change.Key] {
-					if currentValue == value {
-						found = true
-						break
-					}
-				}
-				if !found {
-					labelsToAdd[change.Key] = append(labelsToAdd[change.Key], value)
-					mergedLabels[change.Key] = append(mergedLabels[change.Key], value)
-				}
+				addValue(mergedLabels, change, value, labelsToAdd)
 			}
 		case types.RemoveLabelOperation:
 			fallthrough
@@ -88,3 +81,17 @@ func ApplyLabelChangesToLabels(changes types.LabelChanges, labels types.Labels)
 
 	return mergedLabels, labelsToAdd, labelsToRemove
 }
+
+func addValue(mergedLabels types.Labels, change *types.LabelChange, value string, labelsToAdd types.Labels) {
+	found := false
+	for _, currentValue := range mergedLabels[change.Key] {
+		if currentValue == value {
+			found = true
+			break
+		}
+	}
+	if !found {
+		labelsToAdd[change.Key] = append(labelsToAdd[change.Key], value)
+		mergedLabels[change.Key] = append(mergedLabels[change.Key], value)
+	}
+}
diff --git a/pkg/query/update_test.go b/pkg/query/update_test.go
@@ -64,8 +64,8 @@ var _ = Describe("Update", func() {
 				body, err := sjson.DeleteBytes(body, "labels.0.values")
 				Expect(err).ToNot(HaveOccurred())
 				changes, err := LabelChangesFromJSON(body)
-				Expect(err).To(HaveOccurred())
-				Expect(changes).To(BeNil())
+				Expect(err).ToNot(HaveOccurred())
+				Expect(changes).ToNot(BeNil())
 			})
 		})
 

diff --git a/pkg/types/labels.go b/pkg/types/labels.go
@@ -55,7 +55,8 @@ const (
 
 // RequiresValues returns true if the operation requires values to be provided
 func (o LabelOperation) RequiresValues() bool {
-	return o != RemoveLabelOperation
+	// add support for empty value
+	return o != RemoveLabelOperation && o != AddLabelOperation
 }
 
 // LabelChange represents the changes that should be performed to a label

diff --git a/storage/postgres/constant.go b/storage/postgres/constant.go
@@ -2,4 +2,4 @@ package postgres
 
 // todo: get automatically from folder
 // change this line with the latest version of the migrations:
-const latestMigrationVersion = "20210808120000"
+const latestMigrationVersion = "20211125100000"
diff --git a/storage/postgres/migrations/20211125100000_alter_label_value.down.sql b/storage/postgres/migrations/20211125100000_alter_label_value.down.sql
@@ -0,0 +1,22 @@
+BEGIN;
+
+ALTER TABLE visibility_labels ALTER COLUMN val SET NOT NULL;
+ALTER TABLE visibility_labels ADD CONSTRAINT visibility_labels_val_check CHECK (val <> '');
+ALTER TABLE broker_labels ALTER COLUMN val SET NOT NULL;
+ALTER TABLE broker_labels ADD CONSTRAINT broker_labels_val_check CHECK (val <> '');
+ALTER TABLE platform_labels ALTER COLUMN val SET NOT NULL;
+ALTER TABLE platform_labels ADD CONSTRAINT platform_labels_val_check CHECK (val <> '');
+ALTER TABLE service_offering_labels ALTER COLUMN val SET NOT NULL;
+ALTER TABLE service_offering_labels ADD CONSTRAINT service_offering_labels_val_check CHECK (val <> '');
+ALTER TABLE service_plan_labels ALTER COLUMN val SET NOT NULL;
+ALTER TABLE service_plan_labels ADD CONSTRAINT service_plan_labels_val_check CHECK (val <> '');
+ALTER TABLE notification_labels ALTER COLUMN val SET NOT NULL;
+ALTER TABLE notification_labels ADD CONSTRAINT notification_labels_val_check CHECK (val <> '');
+ALTER TABLE service_instance_labels ALTER COLUMN val SET NOT NULL;
+ALTER TABLE service_instance_labels ADD CONSTRAINT service_instance_labels_val_check CHECK (val <> '');
+ALTER TABLE service_binding_labels ALTER COLUMN val SET NOT NULL;
+ALTER TABLE service_binding_labels ADD CONSTRAINT service_binding_labels_val_check CHECK (val <> '');
+ALTER TABLE broker_platform_credential_labels ALTER COLUMN val SET NOT NULL;
+ALTER TABLE broker_platform_credential_labels ADD CONSTRAINT broker_platform_credential_labels_val_check CHECK (val <> '');
+
+COMMIT;
diff --git a/storage/postgres/migrations/20211125100000_alter_label_value.up.sql b/storage/postgres/migrations/20211125100000_alter_label_value.up.sql
@@ -0,0 +1,22 @@
+BEGIN;
+
+ALTER TABLE visibility_labels ALTER COLUMN val DROP NOT NULL;
+ALTER TABLE visibility_labels DROP CONSTRAINT visibility_labels_val_check;
+ALTER TABLE broker_labels ALTER COLUMN val DROP NOT NULL;
+ALTER TABLE broker_labels DROP CONSTRAINT broker_labels_val_check;
+ALTER TABLE platform_labels ALTER COLUMN val DROP NOT NULL;
+ALTER TABLE platform_labels DROP CONSTRAINT platform_labels_val_check;
+ALTER TABLE service_offering_labels ALTER COLUMN val DROP NOT NULL;
+ALTER TABLE service_offering_labels DROP CONSTRAINT service_offering_labels_val_check;
+ALTER TABLE service_plan_labels ALTER COLUMN val DROP NOT NULL;
+ALTER TABLE service_plan_labels DROP CONSTRAINT service_plan_labels_val_check;
+ALTER TABLE notification_labels ALTER COLUMN val DROP NOT NULL;
+ALTER TABLE notification_labels DROP CONSTRAINT notification_labels_val_check;
+ALTER TABLE service_instance_labels ALTER COLUMN val DROP NOT NULL;
+ALTER TABLE service_instance_labels DROP CONSTRAINT service_instance_labels_val_check;
+ALTER TABLE service_binding_labels ALTER COLUMN val DROP NOT NULL;
+ALTER TABLE service_binding_labels DROP CONSTRAINT service_binding_labels_val_check;
+ALTER TABLE broker_platform_credential_labels ALTER COLUMN val DROP NOT NULL;
+ALTER TABLE broker_platform_credential_labels DROP CONSTRAINT broker_platform_credential_labels_val_check;
+
+COMMIT;
diff --git a/test/broker_test/broker_test.go b/test/broker_test/broker_test.go
@@ -2571,7 +2571,20 @@ var _ = test.DescribeTestsFor(test.TestCase{
 								Status(http.StatusOK)
 						})
 					})
-
+					Context("Add  label empty value", func() {
+						BeforeEach(func() {
+							operation = types.AddLabelOperation
+							changedLabelKey = "cluster_id"
+							changedLabelValues = []string{}
+						})
+						It("Should return 200", func() {
+							ctx.SMWithOAuth.PATCH(web.ServiceBrokersURL + "/" + id).
+								WithJSON(patchLabelsBody).
+								Expect().
+								Status(http.StatusOK).JSON().
+								Path("$.labels").Object().Keys().Contains(changedLabelKey)
+						})
+					})
 					Context("Remove a label", func() {
 						BeforeEach(func() {
 							operation = types.RemoveLabelOperation

diff --git a/test/common/common.go b/test/common/common.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"github.com/Peripli/service-manager/pkg/multitenancy"
 	"github.com/tidwall/gjson"
+	"reflect"
 	"time"
 
 	"github.com/Peripli/service-manager/pkg/util"
@@ -214,7 +215,8 @@ func MapContains(actual Object, expected Object) {
 		if !ok {
 			Fail(fmt.Sprintf("Missing property '%s'", k), 1)
 		}
-		if value != v {
+
+		if !reflect.DeepEqual(value, v) {
 			Fail(
 				fmt.Sprintf("For property '%s':\nExpected: %s\nActual: %s", k, v, value),
 				1)

diff --git a/test/platform_test/platform_test.go b/test/platform_test/platform_test.go
@@ -292,6 +292,41 @@ var _ = test.DescribeTestsFor(test.TestCase{
 
 						By("Update is persisted")
 
+						reply = ctx.SMWithOAuth.GET(web.PlatformsURL + "/" + id).
+							Expect().
+							Status(http.StatusOK).JSON().Object()
+
+						common.MapContains(reply.Raw(), updatedPlatform)
+					})
+					It("when labels value are empty returns 200", func() {
+						By("Update platform")
+
+						updatedPlatform := common.MakePlatform("", "cf-11", "cff", "descr2")
+						delete(updatedPlatform, "id")
+						changeLabels := []*types.LabelChange{
+							{
+								Operation: types.AddLabelOperation,
+								Key:       "newKey",
+							},
+						}
+						updatedPlatform["labels"] = changeLabels
+						reply := ctx.SMWithOAuth.PATCH(web.PlatformsURL + "/" + id).
+							WithJSON(updatedPlatform).
+							Expect().
+							Status(http.StatusOK).JSON().Object()
+
+						reply.NotContainsKey("credentials")
+						labels := map[string]interface{}{
+							"newKey": []interface{}{""},
+						}
+
+						updatedPlatform["labels"] = labels
+
+						updatedPlatform["id"] = id
+						common.MapContains(reply.Raw(), updatedPlatform)
+
+						By("Update is persisted")
+
 						reply = ctx.SMWithOAuth.GET(web.PlatformsURL + "/" + id).
 							Expect().
 							Status(http.StatusOK).JSON().Object()

diff --git a/test/service_instance_and_binding_test/service_instance_test/service_instance_test.go b/test/service_instance_and_binding_test/service_instance_test/service_instance_test.go
@@ -19,13 +19,14 @@ package service_test
 import (
 	"context"
 	"fmt"
-	"github.com/Peripli/service-manager/operations"
-	"github.com/Peripli/service-manager/pkg/instance_sharing"
-	"github.com/Peripli/service-manager/pkg/query"
 	"strings"
 	"sync/atomic"
 	"time"
 
+	"github.com/Peripli/service-manager/operations"
+	"github.com/Peripli/service-manager/pkg/instance_sharing"
+	"github.com/Peripli/service-manager/pkg/query"
+
 	"github.com/Peripli/service-manager/pkg/env"
 	"github.com/tidwall/gjson"
 	"github.com/tidwall/sjson"
@@ -627,6 +628,18 @@ var _ = DescribeTestsFor(TestCase{
 									WithJSON(postInstanceRequestTLS).
 									Expect().Status(http.StatusAccepted)
 							})
+							It("returns 201", func() {
+								EnsurePlanVisibility(ctx.SMRepository, TenantIdentifier, types.SMPlatform, servicePlanIDWithTLS, TenantIDValue)
+								labels := types.Labels{
+									"organization_guid": []string{},
+								}
+
+								postInstanceRequestTLS["labels"] = labels
+								ctx.SMWithOAuthForTenant.POST(web.ServiceInstancesURL).
+									WithQuery("async", false).
+									WithJSON(postInstanceRequestTLS).
+									Expect().Status(http.StatusCreated)
+							})
 
 							It("returns 201", func() {
 								EnsurePlanVisibility(ctx.SMRepository, TenantIdentifier, types.SMPlatform, servicePlanIDWithTLS, TenantIDValue)
@@ -668,7 +681,6 @@ var _ = DescribeTestsFor(TestCase{
 									JSON().Object().
 									Keys().Contains("error", "description")
 							})
-
 							Context("when request body contains multiple label objects", func() {
 								It("returns 400", func() {
 									ctx.SMWithOAuthForTenant.POST(web.ServiceInstancesURL).
@@ -2092,6 +2104,26 @@ var _ = DescribeTestsFor(TestCase{
 										Expect(method).To(Not(Equal("PATCH")))
 									})
 								})
+								When("only label is provided in request body- value is empty", func() {
+									It("returns success", func() {
+										labels := []*types.LabelChange{
+											{
+												Operation: types.AddLabelOperation,
+												Key:       "labelKey1",
+											},
+										}
+										patchLabelsBody["labels"] = labels
+
+										testCtx.SMWithOAuthForTenant.PATCH(web.ServiceInstancesURL+"/"+SID).
+											WithQuery("async", "false").
+											WithJSON(patchLabelsBody).Expect().Status(http.StatusOK).JSON().Object()
+										// Expect to retrieve the data from the broker of the creat instance and not of the update instance
+										// method should not be patch to the broker, but only post of the previous request
+										method := brokerServer.LastRequest.Method
+										Expect(method).To(Not(Equal("PATCH")))
+
+									})
+								})
 								When("invalid label key", func() {
 									It("returns error", func() {
 										labels := []*types.LabelChange{