diff --git a/pkg/query/update.go b/pkg/query/update.go index 74eea2a7d..3dae3a900 100644 --- 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 index e073ba78f..b2e599f48 100644 --- 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 index c7bab426a..79b6aa956 100644 --- 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 index 0966cf586..d8c979a15 100644 --- 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 new file mode 100644 index 000000000..def347395 --- /dev/null +++ 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; \ No newline at end of file diff --git a/storage/postgres/migrations/20211125100000_alter_label_value.up.sql b/storage/postgres/migrations/20211125100000_alter_label_value.up.sql new file mode 100644 index 000000000..915b7aade --- /dev/null +++ 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; \ No newline at end of file diff --git a/test/broker_test/broker_test.go b/test/broker_test/broker_test.go index 0ebffeed7..f4a656832 100644 --- 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 index f2454f429..22de36fe1 100644 --- 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 index 5a818abfb..a58c47664 100644 --- 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 index 3e6eeb097..0948704ab 100644 --- 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{