v8(services): refactor instance parameters type

[#175303088](https://www.pivotaltracker.com/story/show/175303088)
cloudfoundry · Oct 29, 2020 · f460dc4 · f460dc4
1 parent e6ffa1e
commit f460dc4
Show file tree

Hide file tree

Showing 12 changed files with 79 additions and 52 deletions.
diff --git a/actor/v7action/cloud_controller_client.go b/actor/v7action/cloud_controller_client.go
@@ -111,7 +111,7 @@ type CloudControllerClient interface {
 	GetServiceBrokers(query ...ccv3.Query) ([]resources.ServiceBroker, ccv3.Warnings, error)
 	GetServiceCredentialBindings(query ...ccv3.Query) ([]resources.ServiceCredentialBinding, ccv3.Warnings, error)
 	GetServiceInstanceByNameAndSpace(name, spaceGUID string, query ...ccv3.Query) (resources.ServiceInstance, ccv3.IncludedResources, ccv3.Warnings, error)
-	GetServiceInstanceParameters(serviceInstanceGUID string) (types.OptionalObject, ccv3.Warnings, error)
+	GetServiceInstanceParameters(serviceInstanceGUID string) (types.JSONObject, ccv3.Warnings, error)
 	GetServiceInstanceSharedSpaces(serviceInstanceGUID string) ([]ccv3.SpaceWithOrganization, ccv3.Warnings, error)
 	GetServiceInstanceUsageSummary(serviceInstanceGUID string) ([]resources.ServiceInstanceUsageSummary, ccv3.Warnings, error)
 	GetServiceInstances(query ...ccv3.Query) ([]resources.ServiceInstance, ccv3.IncludedResources, ccv3.Warnings, error)

diff --git a/actor/v7action/service_instance_details.go b/actor/v7action/service_instance_details.go
@@ -33,7 +33,7 @@ type SharedStatus struct {
 }
 
 type ServiceInstanceParameters struct {
-	Value         types.OptionalObject
+	Value         types.JSONObject
 	MissingReason string
 }
 

diff --git a/actor/v7action/service_instance_details_test.go b/actor/v7action/service_instance_details_test.go
@@ -1,14 +1,15 @@
 package v7action_test
 
 import (
+	"errors"
+
 	"code.cloudfoundry.org/cli/actor/actionerror"
 	. "code.cloudfoundry.org/cli/actor/v7action"
 	"code.cloudfoundry.org/cli/actor/v7action/v7actionfakes"
 	"code.cloudfoundry.org/cli/api/cloudcontroller/ccerror"
 	"code.cloudfoundry.org/cli/api/cloudcontroller/ccv3"
 	"code.cloudfoundry.org/cli/resources"
 	"code.cloudfoundry.org/cli/types"
-	"errors"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
@@ -73,7 +74,7 @@ var _ = Describe("Service Instance Details Action", func() {
 			)
 
 			fakeCloudControllerClient.GetServiceInstanceParametersReturns(
-				types.NewOptionalObject(map[string]interface{}{"foo": "bar"}),
+				types.JSONObject{"foo": "bar"},
 				ccv3.Warnings{"some-parameters-warning"},
 				nil,
 			)
@@ -155,7 +156,7 @@ var _ = Describe("Service Instance Details Action", func() {
 					ServiceBrokerName: serviceBrokerName,
 					SharedStatus:      SharedStatus{},
 					Parameters: ServiceInstanceParameters{
-						Value: types.NewOptionalObject(map[string]interface{}{"foo": "bar"}),
+						Value: types.JSONObject{"foo": "bar"},
 					},
 					BoundApps: []resources.ServiceCredentialBinding{
 						{
@@ -271,7 +272,7 @@ var _ = Describe("Service Instance Details Action", func() {
 			When("getting the parameters fails with an expected error", func() {
 				BeforeEach(func() {
 					fakeCloudControllerClient.GetServiceInstanceParametersReturns(
-						types.OptionalObject{},
+						types.JSONObject{},
 						ccv3.Warnings{"some-parameters-warning"},
 						ccerror.V3UnexpectedResponseError{
 							V3ErrorResponse: ccerror.V3ErrorResponse{
@@ -295,7 +296,7 @@ var _ = Describe("Service Instance Details Action", func() {
 			When("getting the parameters fails with an another error", func() {
 				BeforeEach(func() {
 					fakeCloudControllerClient.GetServiceInstanceParametersReturns(
-						types.OptionalObject{},
+						types.JSONObject{},
 						ccv3.Warnings{"some-parameters-warning"},
 						errors.New("not expected"),
 					)

diff --git a/actor/v7action/v7actionfakes/fake_cloud_controller_client.go b/actor/v7action/v7actionfakes/fake_cloud_controller_client.go
diff --git a/api/cloudcontroller/ccv3/service_instance.go b/api/cloudcontroller/ccv3/service_instance.go
@@ -63,20 +63,14 @@ func (client *Client) GetServiceInstanceByNameAndSpace(name, spaceGUID string, q
 	return instances[0], included, warnings, nil
 }
 
-func (client Client) GetServiceInstanceParameters(serviceInstanceGUID string) (types.OptionalObject, Warnings, error) {
-	var receiver map[string]interface{}
-
-	_, warnings, err := client.MakeRequest(RequestParams{
+func (client Client) GetServiceInstanceParameters(serviceInstanceGUID string) (parameters types.JSONObject, warnings Warnings, err error) {
+	_, warnings, err = client.MakeRequest(RequestParams{
 		RequestName:  internal.GetServiceInstanceParametersRequest,
 		URIParams:    internal.Params{"service_instance_guid": serviceInstanceGUID},
-		ResponseBody: &receiver,
+		ResponseBody: &parameters,
 	})
 
-	if err != nil {
-		return types.OptionalObject{}, warnings, err
-	}
-
-	return types.NewOptionalObject(receiver), warnings, nil
+	return
 }
 
 func (client *Client) CreateServiceInstance(serviceInstance resources.ServiceInstance) (JobURL, Warnings, error) {

diff --git a/api/cloudcontroller/ccv3/service_instance_test.go b/api/cloudcontroller/ccv3/service_instance_test.go
@@ -1,7 +1,6 @@
 package ccv3_test
 
 import (
-	"code.cloudfoundry.org/jsonry"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -13,6 +12,7 @@ import (
 	"code.cloudfoundry.org/cli/api/cloudcontroller/ccv3/internal"
 	"code.cloudfoundry.org/cli/resources"
 	"code.cloudfoundry.org/cli/types"
+	"code.cloudfoundry.org/jsonry"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
@@ -310,22 +310,7 @@ var _ = Describe("Service Instance", func() {
 			params, warnings, err := client.GetServiceInstanceParameters(guid)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(warnings).To(ConsistOf("one", "two"))
-			Expect(params).To(Equal(types.NewOptionalObject(map[string]interface{}{"foo": "bar"})))
-		})
-
-		When("there are no parameters", func() {
-			BeforeEach(func() {
-				requester.MakeRequestCalls(func(params RequestParams) (JobURL, Warnings, error) {
-					json.Unmarshal([]byte(``), params.ResponseBody)
-					return "", nil, nil
-				})
-			})
-
-			It("returns a set empty empty object", func() {
-				params, _, _ := client.GetServiceInstanceParameters(guid)
-				Expect(params.Value).To(BeEmpty())
-				Expect(params.IsSet).To(BeTrue())
-			})
+			Expect(params).To(Equal(types.JSONObject{"foo": "bar"}))
 		})
 
 		When("there is an error getting the parameters", func() {
@@ -337,8 +322,7 @@ var _ = Describe("Service Instance", func() {
 				params, warnings, err := client.GetServiceInstanceParameters(guid)
 				Expect(err).To(MatchError("boom"))
 				Expect(warnings).To(ConsistOf("one", "two"))
-				Expect(params.Value).To(BeEmpty())
-				Expect(params.IsSet).To(BeFalse())
+				Expect(params).To(BeEmpty())
 			})
 		})
 	})

diff --git a/command/v7/service_command.go b/command/v7/service_command.go
@@ -158,7 +158,7 @@ func (cmd ServiceCommand) displayParameters() error {
 	switch {
 	case cmd.serviceInstance.Parameters.MissingReason != "":
 		cmd.displayParametersMissingReason()
-	case len(cmd.serviceInstance.Parameters.Value.Value) > 0:
+	case len(cmd.serviceInstance.Parameters.Value) > 0:
 		cmd.displayParametersData()
 	default:
 		cmd.displayParametersEmpty()

diff --git a/command/v7/service_command_test.go b/command/v7/service_command_test.go
@@ -257,7 +257,7 @@ var _ = Describe("service command", func() {
 					ServicePlan:       resources.ServicePlan{Name: servicePlanName},
 					ServiceBrokerName: serviceBrokerName,
 					Parameters: v7action.ServiceInstanceParameters{
-						Value: types.NewOptionalObject(map[string]interface{}{"foo": "bar"}),
+						Value: types.JSONObject{"foo": "bar"},
 					},
 					SharedStatus: v7action.SharedStatus{
 						IsSharedToOtherSpaces: true,
@@ -653,7 +653,7 @@ var _ = Describe("service command", func() {
 							ServicePlan:       resources.ServicePlan{Name: servicePlanName},
 							ServiceBrokerName: serviceBrokerName,
 							Parameters: v7action.ServiceInstanceParameters{
-								Value: types.NewOptionalObject(map[string]interface{}{}),
+								Value: types.JSONObject{},
 							},
 						},
 						v7action.Warnings{"warning one", "warning two"},
@@ -697,7 +697,7 @@ var _ = Describe("service command", func() {
 						ServicePlan:       resources.ServicePlan{Name: servicePlanName},
 						ServiceBrokerName: serviceBrokerName,
 						Parameters: v7action.ServiceInstanceParameters{
-							Value: types.NewOptionalObject(map[string]interface{}{"foo": "bar"}),
+							Value: types.JSONObject{"foo": "bar"},
 						},
 						SharedStatus: v7action.SharedStatus{
 							IsSharedToOtherSpaces: true,

diff --git a/integration/v7/isolated/service_command_test.go b/integration/v7/isolated/service_command_test.go
@@ -518,8 +518,7 @@ var _ = Describe("service command", func() {
 					bindingName1 = helpers.RandomName()
 					bindingName2 = helpers.RandomName()
 
-					const asyncDelay = time.Minute // Forces bind to be "in progress" for predictable output
-					broker = servicebrokerstub.New().WithAsyncDelay(asyncDelay).EnableServiceAccess()
+					broker = servicebrokerstub.New().EnableServiceAccess()
 
 					helpers.CreateManagedServiceInstance(
 						broker.FirstServiceOfferingName(),
@@ -531,6 +530,9 @@ var _ = Describe("service command", func() {
 						Eventually(helpers.CF("push", appName1, "--no-start", "-p", appDir, "-b", "staticfile_buildpack", "--no-route")).Should(Exit(0))
 						Eventually(helpers.CF("push", appName2, "--no-start", "-p", appDir, "-b", "staticfile_buildpack", "--no-route")).Should(Exit(0))
 					})
+
+					const asyncDelay = time.Minute // Forces bind to be "in progress" for predictable output
+					broker.WithAsyncDelay(asyncDelay).Configure()
 					Eventually(helpers.CF("bind-service", appName1, serviceInstanceName, "--binding-name", bindingName1)).Should(Exit(0))
 					Eventually(helpers.CF("bind-service", appName2, serviceInstanceName, "--binding-name", bindingName2)).Should(Exit(0))
 				})

diff --git a/types/json_object.go b/types/json_object.go
@@ -0,0 +1,16 @@
+package types
+
+import "encoding/json"
+
+// JSONObject represents a JSON object. When not initialized it serializes to `{}`,
+// whereas a `map[string]interface{}` serializes to `null`.
+type JSONObject map[string]interface{}
+
+func (j JSONObject) MarshalJSON() ([]byte, error) {
+	switch len(j) {
+	case 0:
+		return []byte("{}"), nil
+	default:
+		return json.Marshal(map[string]interface{}(j))
+	}
+}
diff --git a/types/json_object_test.go b/types/json_object_test.go
@@ -0,0 +1,26 @@
+package types_test
+
+import (
+	"encoding/json"
+
+	. "code.cloudfoundry.org/cli/types"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("JSONObject", func() {
+	It("marshals as JSON", func() {
+		serialized, err := json.Marshal(JSONObject{"foo": "bar"})
+		Expect(err).NotTo(HaveOccurred())
+		Expect(serialized).To(Equal([]byte(`{"foo":"bar"}`)))
+	})
+
+	When("nil", func() {
+		It("marshals as an empty object (not as null)", func() {
+			var o JSONObject
+			serialized, err := json.Marshal(o)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(serialized).To(Equal([]byte(`{}`)))
+		})
+	})
+})
diff --git a/types/optional_object.go b/types/optional_object.go
@@ -2,6 +2,10 @@ package types
 
 import "encoding/json"
 
+// OptionalObject is for situations where we want to differentiate between an
+// empty object, and the object not having been set. An example would be an
+// optional command line option where we want to tell the difference between
+// it being set to an empty object, and it not being specified at all.
 type OptionalObject struct {
 	IsSet bool
 	Value map[string]interface{}