Fix race in custom marshalling of AdditionalMetadata

[#160477667]

Co-authored-by: Kieron Browne <[email protected]>

Gopkg.toml

@@ -44,3 +44,7 @@
 [[constraint]]
  branch = "master"
  name = "github.com/pborman/uuid"
+
+[[constraint]]
+ name = "github.com/pkg/errors"
+ version = "^0.8.0"

catalog.go

@@ -19,6 +19,8 @@ import (
  "encoding/json"
  "reflect"
  "strings"
+
+ "github.com/pkg/errors"
 )
 
 type Service struct {
@@ -112,11 +114,19 @@ const (
 func (spm ServicePlanMetadata) MarshalJSON() ([]byte, error) {
  type Alias ServicePlanMetadata
 
- b, _ := json.Marshal(Alias(spm))
- m := spm.AdditionalMetadata
+ b, err := json.Marshal(Alias(spm))
+ if err != nil {
+ return []byte{}, errors.Wrap(err, "unmarshallable content in AdditionalMetadata")
+ }
+
+ var m map[string]interface{}
  json.Unmarshal(b, &m)
  delete(m, additionalMetadataName)
 
+ for k, v := range spm.AdditionalMetadata {
+ m[k] = v
+ }
+
  return json.Marshal(m)
 }
 
@@ -166,11 +176,18 @@ func GetJsonNames(s reflect.Value) (res []string) {
 func (sm ServiceMetadata) MarshalJSON() ([]byte, error) {
  type Alias ServiceMetadata
 
- b, _ := json.Marshal(Alias(sm))
- m := sm.AdditionalMetadata
+ b, err := json.Marshal(Alias(sm))
+ if err != nil {
+ return []byte{}, errors.Wrap(err, "unmarshallable content in AdditionalMetadata")
+ }
+
+ var m map[string]interface{}
  json.Unmarshal(b, &m)
  delete(m, additionalMetadataName)
 
+ for k, v := range sm.AdditionalMetadata {
+ m[k] = v
+ }
  return json.Marshal(m)
 }
 

catalog_test.go

@@ -18,6 +18,7 @@ package brokerapi_test
 import (
  "encoding/json"
  "reflect"
+ "sync"
 
  . "github.com/onsi/ginkgo"
  . "github.com/onsi/gomega"
@@ -167,6 +168,51 @@ var _ = Describe("Catalog", func() {
  }`
 
  Expect(json.Marshal(metadata)).To(MatchJSON(jsonString))
+
+ By("not mutating the AdditionalMetadata during custom JSON marshalling")
+ Expect(len(metadata.AdditionalMetadata)).To(Equal(2))
+ })
+
+ It("it can marshal same structure in parallel requests", func() {
+ metadata := brokerapi.ServicePlanMetadata{
+ Bullets: []string{"hello", "its me"},
+ DisplayName: "name",
+ AdditionalMetadata: map[string]interface{}{
+ "foo": "bar",
+ "baz": 1,
+ },
+ }
+ jsonString := `{
+ "bullets":["hello", "its me"],
+ "displayName":"name",
+ "foo": "bar",
+ "baz": 1
+ }`
+
+ var wg sync.WaitGroup
+ wg.Add(2)
+
+ for i := 0; i < 2; i++ {
+ go func() {
+ defer wg.Done()
+ defer GinkgoRecover()
+
+ Expect(json.Marshal(metadata)).To(MatchJSON(jsonString))
+ }()
+ }
+ wg.Wait()
+ })
+
+ It("returns an error when additional metadata is not marshallable", func() {
+ metadata := brokerapi.ServicePlanMetadata{
+ Bullets: []string{"hello", "its me"},
+ DisplayName: "name",
+ AdditionalMetadata: map[string]interface{}{
+ "foo": make(chan int, 0),
+ },
+ }
+ _, err := json.Marshal(metadata)
+ Expect(err).To(MatchError(ContainSubstring("unmarshallable content in AdditionalMetadata")))
  })
  })
 
@@ -233,6 +279,48 @@ var _ = Describe("Catalog", func() {
  }`
 
  Expect(json.Marshal(metadata)).To(MatchJSON(jsonString))
+
+ By("not mutating the AdditionalMetadata during custom JSON marshalling")
+ Expect(len(metadata.AdditionalMetadata)).To(Equal(2))
+ })
+
+ It("it can marshal same structure in parallel requests", func() {
+ metadata := brokerapi.ServiceMetadata{
+ DisplayName: "name",
+ AdditionalMetadata: map[string]interface{}{
+ "foo": "bar",
+ "baz": 1,
+ },
+ }
+ jsonString := `{
+ "displayName":"name",
+ "foo": "bar",
+ "baz": 1
+ }`
+
+ var wg sync.WaitGroup
+ wg.Add(2)
+
+ for i := 0; i < 2; i++ {
+ go func() {
+ defer wg.Done()
+ defer GinkgoRecover()
+
+ Expect(json.Marshal(metadata)).To(MatchJSON(jsonString))
+ }()
+ }
+ wg.Wait()
+ })
+
+ It("returns an error when additional metadata is not marshallable", func() {
+ metadata := brokerapi.ServiceMetadata{
+ DisplayName: "name",
+ AdditionalMetadata: map[string]interface{}{
+ "foo": make(chan int),
+ },
+ }
+ _, err := json.Marshal(metadata)
+ Expect(err).To(MatchError(ContainSubstring("unmarshallable content in AdditionalMetadata")))
  })
  })