Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve the external API of the PropertyBag #1669

Merged
merged 6 commits into from
Aug 7, 2021
Merged

Improve the external API of the PropertyBag #1669

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f451256
Add PropertyBag to std_references
theunrepentantgeek Jul 26, 2021
eb143a4
Add factory to PropertyBag
theunrepentantgeek Jul 26, 2021
5954157
Test PropertyBag.Clone() works with zero bag
theunrepentantgeek Jul 26, 2021
cd54a3a
Revise PropertyBag API
theunrepentantgeek Jul 28, 2021
1ed64b8
Rename tests for clarity
theunrepentantgeek Aug 7, 2021
2523ed7
Merge branch 'master' into improve/propertybag-api
theunrepentantgeek Aug 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 26 additions & 12 deletions hack/generated/pkg/genruntime/property_bag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,39 @@ package genruntime

import (
"encoding/json"

"github.com/pkg/errors"
)

// PropertyBag is used to stash additional information that's not directly supported by storage
// PropertyBag is an unordered set of stashed information that used for properties not directly supported by storage
// resources, allowing for full fidelity round trip conversions
type PropertyBag map[string]string

// Clone creates an independent copy of the property bag
func (bag PropertyBag) Clone() PropertyBag {
// PropertyBag returns a new property bag
// originals is a (potentially empty) sequence of existing property bags who's content will be copied into the new
// property bag. In the case of key overlaps, the bag later in the parameter list overwrites the earlier value.
func NewPropertyBag(originals ...PropertyBag) PropertyBag {
result := make(PropertyBag)
for k, v := range bag {
result[k] = v

for _, orig := range originals {
for k, v := range orig {
result[k] = v
}
}

return result
}

// Contains returns true if the specified name is present in the bag; false otherwise
func (bag PropertyBag) Contains(name string) bool {
_, found := bag[name]
return found
}

// Add is used to add a value into the bag; exact formatting depends on the type.
// Any existing value will be overwritten.
// property is the name of the item to put into the bag
// value is the instance to be stashed away for later
func (bag PropertyBag) Add(property string, value interface{}) error {
switch v := value.(type) {
case string:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: This isn't in the code you actually changed, but I'm curious why string is called out here but not other primitive types?

Feels like you could probably get away without any special casing and just always use json.Marshal? I am assuming that json.Marshal(str) just returns str but I don't know for sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall - might have been to avoid the overhead of calling json.Marshal when not required, or it may have been to handle round trip formatting issues. I do remember it was deliberate though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth leaving a comment there as to the reason? Especially if there was some problem if it wasn't handled specially?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a laundry list of minor code-gardening things to do post codegen-alpha-0, I'll add this to that.

Expand All @@ -43,15 +57,15 @@ func (bag PropertyBag) Add(property string, value interface{}) error {
}

// Pull removes a value from the bag, using it to populate the destination
// property is the name of the item to remove and return
// destination should be a pointer to where the value is to be placed
// Returns (false, nil) if the value isn't found, (true, nil) if the value is successfully stored,
// and (true, error) if something goes wrong during type conversion of the value.
// If an error occurs, the property value is still removed from the bag
func (bag PropertyBag) Pull(property string, destination interface{}) (bool, error) {
// If the item is present and successfully deserialized, returns no error (nil); otherwise returns an error.
// If an error happens deserializing an item from the bag, it is still removed from the bag.
func (bag PropertyBag) Pull(property string, destination interface{}) error {
value, found := bag[property]
if !found {
// Property not found in the bag
return false, nil
return errors.Errorf("property bag does not contain %q", property)
}

// Property found, remove the value
Expand All @@ -64,9 +78,9 @@ func (bag PropertyBag) Pull(property string, destination interface{}) (bool, err
data := []byte(value)
err := json.Unmarshal(data, destination)
if err != nil {
return true, errors.Wrapf(err, "pulling from PropertyBag into %q", property)
return errors.Wrapf(err, "pulling %q from PropertyBag", property)
}
}

return true, nil
return nil
}
151 changes: 117 additions & 34 deletions hack/generated/pkg/genruntime/property_bag_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,113 @@
package genruntime

import (
. "github.com/onsi/gomega"

"testing"

. "github.com/onsi/gomega"
)

/*
* NewPropertyBag() Tests
*/

func TestPropertyBag_NewPropertyBag_WithNoParameters_ReturnsEmptyBag(t *testing.T) {
g := NewWithT(t)
bag := NewPropertyBag()
g.Expect(bag).To(BeEmpty())
}

func TestPropertyBag_NewPropertyBag_WithZeroBag_ReturnsEmptyBag(t *testing.T) {
g := NewWithT(t)
var zeroBag PropertyBag
bag := NewPropertyBag(zeroBag)
g.Expect(bag).To(BeEmpty())
}

func TestPropertyBag_NewPropertyBag_WithEmptyBag_ReturnsEmptyBag(t *testing.T) {
g := NewWithT(t)
emptyBag := NewPropertyBag()
bag := NewPropertyBag(emptyBag)
g.Expect(bag).To(BeEmpty())
}

func TestPropertyBag_NewPropertyBag_WithSingleItemBag_ReturnsBagWithExpectedLength(t *testing.T) {
g := NewWithT(t)
original := NewPropertyBag()
g.Expect(original.Add("Answer", 42)).To(Succeed())

bag := NewPropertyBag(original)
g.Expect(bag).To(HaveLen(1))
}

func TestPropertyBag_NewPropertyBag_WithMultipleItemBag_ReturnsBagWithExpectedContent(t *testing.T) {
g := NewWithT(t)
original := NewPropertyBag()
g.Expect(original.Add("Answer", 42)).To(Succeed())
g.Expect(original.Add("Halloween", "31OCT")).To(Succeed())
g.Expect(original.Add("Christmas", "25DEC")).To(Succeed())

bag := NewPropertyBag(original)
g.Expect(bag).To(HaveKey("Answer"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You aren't asserting that the actual content is correct here, just that it has the key?
Shouldn't you also check to make sure that the key maps to the correct value?

Same comment applies below in a couple other tests, but won't repeat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see in the other tests (that you already had) that you may have those cases covered already.

Maybe this just becomes minor: rather than ReturnsBagWithExpectedContent use ReturnsBagWithExpectedKeys to make it clear you only care about asserting on keys here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other tests check that the values stored can be recalled cleanly, so I'm only worried in this test whether the expected keys are put into the bag. I'll rename the tests as you suggest.

g.Expect(bag).To(HaveKey("Halloween"))
g.Expect(bag).To(HaveKey("Christmas"))
g.Expect(bag).To(HaveLen(3))
}

func TestPropertyBag_NewPropertyBag_WithMultipleSingleItemBags_ReturnsBagWithExpectedContent(t *testing.T) {
g := NewWithT(t)
first := NewPropertyBag()
g.Expect(first.Add("Answer", 42)).To(Succeed())

second := NewPropertyBag()
g.Expect(second.Add("Halloween", "31OCT")).To(Succeed())
g.Expect(second.Add("Christmas", "25DEC")).To(Succeed())

bag := NewPropertyBag(first, second)
g.Expect(bag).To(HaveKey("Answer"))
g.Expect(bag).To(HaveKey("Halloween"))
g.Expect(bag).To(HaveKey("Christmas"))
g.Expect(bag).To(HaveLen(3))
}

func TestPropertyBag_NewPropertyBag_WithOverlappingBags_ReturnsBagWithExpectedContent(t *testing.T) {
g := NewWithT(t)
first := NewPropertyBag()
g.Expect(first.Add("Answer", 42)).To(Succeed())
g.Expect(first.Add("Gift", "Skull")).To(Succeed())

second := NewPropertyBag()
g.Expect(second.Add("Halloween", "31OCT")).To(Succeed())
g.Expect(second.Add("Christmas", "25DEC")).To(Succeed())
g.Expect(second.Add("Gift", "Gold")).To(Succeed())

bag := NewPropertyBag(first, second)
g.Expect(bag).To(HaveKey("Answer"))
g.Expect(bag).To(HaveKey("Halloween"))
g.Expect(bag).To(HaveKey("Christmas"))
g.Expect(bag).To(HaveKey("Gift"))
g.Expect(bag).To(HaveLen(4))

var gift string
err := bag.Pull("Gift", &gift)
g.Expect(err).To(Succeed())
g.Expect(gift).To(Equal("Gold"))
}

/*
* Roundtrip Tests
*/

func TestPropertyBag_CorrectlyRoundTripsIntegers(t *testing.T) {
g := NewWithT(t)
var original int = 42
var actual int

bag := make(PropertyBag)
err := bag.Add("prop", original)
g.Expect(err).To(BeNil())
g.Expect(err).To(Succeed())

found, err := bag.Pull("prop", &actual)
g.Expect(err).To(BeNil())
g.Expect(found).To(BeTrue())
err = bag.Pull("prop", &actual)
g.Expect(err).To(Succeed())
g.Expect(actual).To(Equal(original))
}

Expand All @@ -33,11 +123,10 @@ func TestPropertyBag_CorrectlyRoundTrips64bitIntegers(t *testing.T) {

bag := make(PropertyBag)
err := bag.Add("prop", original)
g.Expect(err).To(BeNil())
g.Expect(err).To(Succeed())

found, err := bag.Pull("prop", &actual)
g.Expect(err).To(BeNil())
g.Expect(found).To(BeTrue())
err = bag.Pull("prop", &actual)
g.Expect(err).To(Succeed())
g.Expect(actual).To(Equal(original))
}

Expand All @@ -48,11 +137,10 @@ func TestPropertyBag_CorrectlyRoundTripsStrings(t *testing.T) {

bag := make(PropertyBag)
err := bag.Add("prop", original)
g.Expect(err).To(BeNil())
g.Expect(err).To(Succeed())

found, err := bag.Pull("prop", &actual)
g.Expect(err).To(BeNil())
g.Expect(found).To(BeTrue())
err = bag.Pull("prop", &actual)
g.Expect(err).To(Succeed())
g.Expect(actual).To(Equal(original))
}

Expand All @@ -63,11 +151,10 @@ func TestPropertyBag_CorrectlyRoundTripsBooleans(t *testing.T) {

bag := make(PropertyBag)
err := bag.Add("prop", original)
g.Expect(err).To(BeNil())
g.Expect(err).To(Succeed())

found, err := bag.Pull("prop", &actual)
g.Expect(err).To(BeNil())
g.Expect(found).To(BeTrue())
err = bag.Pull("prop", &actual)
g.Expect(err).To(Succeed())
g.Expect(actual).To(Equal(original))
}

Expand All @@ -78,11 +165,10 @@ func TestPropertyBag_CorrectlyRoundTripsFloats(t *testing.T) {

bag := make(PropertyBag)
err := bag.Add("prop", original)
g.Expect(err).To(BeNil())
g.Expect(err).To(Succeed())

found, err := bag.Pull("prop", &actual)
g.Expect(err).To(BeNil())
g.Expect(found).To(BeTrue())
err = bag.Pull("prop", &actual)
g.Expect(err).To(Succeed())
g.Expect(actual).To(Equal(original))
}

Expand All @@ -105,11 +191,10 @@ func TestPropertyBag_CorrectlyRoundTripsStructs(t *testing.T) {

bag := make(PropertyBag)
err := bag.Add("prop", original)
g.Expect(err).To(BeNil())
g.Expect(err).To(Succeed())

found, err := bag.Pull("prop", &actual)
g.Expect(err).To(BeNil())
g.Expect(found).To(BeTrue())
err = bag.Pull("prop", &actual)
g.Expect(err).To(Succeed())
g.Expect(actual).To(Equal(original))
}

Expand All @@ -120,11 +205,10 @@ func TestPropertyBag_WhenPropertyNotPresent(t *testing.T) {

bag := make(PropertyBag)
err := bag.Add("prop", original)
g.Expect(err).To(BeNil())
g.Expect(err).To(Succeed())

found, err := bag.Pull("otherProp", &actual)
g.Expect(err).To(BeNil())
g.Expect(found).To(BeFalse())
err = bag.Pull("otherProp", &actual)
g.Expect(err).NotTo(Succeed())
}

func TestPropertyBag_WhenPropertyWrongType(t *testing.T) {
Expand All @@ -134,9 +218,8 @@ func TestPropertyBag_WhenPropertyWrongType(t *testing.T) {

bag := make(PropertyBag)
err := bag.Add("prop", original)
g.Expect(err).To(BeNil())
g.Expect(err).To(Succeed())

found, err := bag.Pull("prop", &actual)
g.Expect(err).NotTo(BeNil())
g.Expect(found).To(BeTrue())
err = bag.Pull("prop", &actual)
g.Expect(err).NotTo(Succeed())
}
1 change: 1 addition & 0 deletions hack/generator/pkg/astmodel/std_references.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ var (
ConvertibleStatusInterfaceType = MakeTypeName(GenRuntimeReference, "ConvertibleStatus")
ResourceReferenceTypeName = MakeTypeName(GenRuntimeReference, "ResourceReference")
KnownResourceReferenceTypeName = MakeTypeName(GenRuntimeReference, "KnownResourceReference")
PropertyBagType = MakeTypeName(GenRuntimeReference, "PropertyBag")
ToARMConverterInterfaceType = MakeTypeName(GenRuntimeReference, "ToARMConverter")

// Type names - API Machinery
Expand Down